| Message ID | 20210408030556.45134-1-xiyou.wangcong@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next] sock_map: fix a potential use-after-free in sock_map_close() | expand |
Cong Wang wrote: > From: Cong Wang <cong.wang@bytedance.com> > > The last refcnt of the psock can be gone right after > sock_map_remove_links(), so sk_psock_stop() could trigger a UAF. > The reason why I placed sk_psock_stop() there is to avoid RCU read > critical section, and more importantly, some callee of > sock_map_remove_links() is supposed to be called with RCU read lock, > we can not simply get rid of RCU read lock here. Therefore, the only > choice we have is to grab an additional refcnt with sk_psock_get() > and put it back after sk_psock_stop(). > > Reported-by: syzbot+7b6548ae483d6f4c64ae@syzkaller.appspotmail.com > Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()") > Cc: John Fastabend <john.fastabend@gmail.com> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Jakub Sitnicki <jakub@cloudflare.com> > Cc: Lorenz Bauer <lmb@cloudflare.com> > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > --- > net/core/sock_map.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > index f473c51cbc4b..6f1b82b8ad49 100644 > --- a/net/core/sock_map.c > +++ b/net/core/sock_map.c > @@ -1521,7 +1521,7 @@ void sock_map_close(struct sock *sk, long timeout) > > lock_sock(sk); > rcu_read_lock(); It looks like we can drop the rcu_read_lock()/unlock() section then if we take a reference on the psock? Before it was there to ensure we didn't lose the psock from some other context, but with a reference held this can not happen. > - psock = sk_psock(sk); > + psock = sk_psock_get(sk); > if (unlikely(!psock)) { > rcu_read_unlock(); > release_sock(sk); > @@ -1532,6 +1532,7 @@ void sock_map_close(struct sock *sk, long timeout) > sock_map_remove_links(sk, psock); > rcu_read_unlock(); > sk_psock_stop(psock, true); > + sk_psock_put(sk, psock); > release_sock(sk); > saved_close(sk, timeout); > } > -- > 2.25.1 >
On Thu, Apr 8, 2021 at 5:26 PM John Fastabend <john.fastabend@gmail.com> wrote: > > Cong Wang wrote: > > From: Cong Wang <cong.wang@bytedance.com> > > > > The last refcnt of the psock can be gone right after > > sock_map_remove_links(), so sk_psock_stop() could trigger a UAF. > > The reason why I placed sk_psock_stop() there is to avoid RCU read > > critical section, and more importantly, some callee of > > sock_map_remove_links() is supposed to be called with RCU read lock, > > we can not simply get rid of RCU read lock here. Therefore, the only > > choice we have is to grab an additional refcnt with sk_psock_get() > > and put it back after sk_psock_stop(). > > > > Reported-by: syzbot+7b6548ae483d6f4c64ae@syzkaller.appspotmail.com > > Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()") > > Cc: John Fastabend <john.fastabend@gmail.com> > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > Cc: Jakub Sitnicki <jakub@cloudflare.com> > > Cc: Lorenz Bauer <lmb@cloudflare.com> > > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > > --- > > net/core/sock_map.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > > index f473c51cbc4b..6f1b82b8ad49 100644 > > --- a/net/core/sock_map.c > > +++ b/net/core/sock_map.c > > @@ -1521,7 +1521,7 @@ void sock_map_close(struct sock *sk, long timeout) > > > > lock_sock(sk); > > rcu_read_lock(); > > It looks like we can drop the rcu_read_lock()/unlock() section then if we > take a reference on the psock? Before it was there to ensure we didn't > lose the psock from some other context, but with a reference held this > can not happen. Some callees under sock_map_remove_links() still assert RCU read lock, so we can not simply drop the RCU read lock here. Some additional efforts are needed to take care of those assertions, which can be a separate patch. Thanks.
Cong Wang wrote: > On Thu, Apr 8, 2021 at 5:26 PM John Fastabend <john.fastabend@gmail.com> wrote: > > > > Cong Wang wrote: > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > The last refcnt of the psock can be gone right after > > > sock_map_remove_links(), so sk_psock_stop() could trigger a UAF. > > > The reason why I placed sk_psock_stop() there is to avoid RCU read > > > critical section, and more importantly, some callee of > > > sock_map_remove_links() is supposed to be called with RCU read lock, > > > we can not simply get rid of RCU read lock here. Therefore, the only > > > choice we have is to grab an additional refcnt with sk_psock_get() > > > and put it back after sk_psock_stop(). > > > > > > Reported-by: syzbot+7b6548ae483d6f4c64ae@syzkaller.appspotmail.com > > > Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()") > > > Cc: John Fastabend <john.fastabend@gmail.com> > > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > > Cc: Jakub Sitnicki <jakub@cloudflare.com> > > > Cc: Lorenz Bauer <lmb@cloudflare.com> > > > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > > > --- > > > net/core/sock_map.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > > > index f473c51cbc4b..6f1b82b8ad49 100644 > > > --- a/net/core/sock_map.c > > > +++ b/net/core/sock_map.c > > > @@ -1521,7 +1521,7 @@ void sock_map_close(struct sock *sk, long timeout) > > > > > > lock_sock(sk); > > > rcu_read_lock(); > > > > It looks like we can drop the rcu_read_lock()/unlock() section then if we > > take a reference on the psock? Before it was there to ensure we didn't > > lose the psock from some other context, but with a reference held this > > can not happen. > > Some callees under sock_map_remove_links() still assert RCU read > lock, so we can not simply drop the RCU read lock here. Some > additional efforts are needed to take care of those assertions, which > can be a separate patch. > > Thanks. OK at least this case exists, sock_map_close sock_map_remove_links sock_map_unlink sock_hash_delete_from_link WARN_ON_ONCE(!rcu_read_lock_held()); also calls into sock_map_unref through similar path use sk_psock(sk) depending on rcu critical section. Its certainly non-trivial to remove. I don't really like taking a ref here but seems necessary for now. Acked-by: John Fastabend <john.fastabend@gmail.com>
On Thu, Apr 08, 2021 at 05:05 AM CEST, Cong Wang wrote: > From: Cong Wang <cong.wang@bytedance.com> > > The last refcnt of the psock can be gone right after > sock_map_remove_links(), so sk_psock_stop() could trigger a UAF. > The reason why I placed sk_psock_stop() there is to avoid RCU read > critical section, and more importantly, some callee of > sock_map_remove_links() is supposed to be called with RCU read lock, > we can not simply get rid of RCU read lock here. Therefore, the only > choice we have is to grab an additional refcnt with sk_psock_get() > and put it back after sk_psock_stop(). > > Reported-by: syzbot+7b6548ae483d6f4c64ae@syzkaller.appspotmail.com > Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()") > Cc: John Fastabend <john.fastabend@gmail.com> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Jakub Sitnicki <jakub@cloudflare.com> > Cc: Lorenz Bauer <lmb@cloudflare.com> > Signed-off-by: Cong Wang <cong.wang@bytedance.com> > --- Acked-by: Jakub Sitnicki <jakub@cloudflare.com>
Hello: This patch was applied to bpf/bpf-next.git (refs/heads/master): On Wed, 7 Apr 2021 20:05:56 -0700 you wrote: > From: Cong Wang <cong.wang@bytedance.com> > > The last refcnt of the psock can be gone right after > sock_map_remove_links(), so sk_psock_stop() could trigger a UAF. > The reason why I placed sk_psock_stop() there is to avoid RCU read > critical section, and more importantly, some callee of > sock_map_remove_links() is supposed to be called with RCU read lock, > we can not simply get rid of RCU read lock here. Therefore, the only > choice we have is to grab an additional refcnt with sk_psock_get() > and put it back after sk_psock_stop(). > > [...] Here is the summary with links: - [bpf-next] sock_map: fix a potential use-after-free in sock_map_close() https://git.kernel.org/bpf/bpf-next/c/aadb2bb83ff7 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/core/sock_map.c b/net/core/sock_map.c index f473c51cbc4b..6f1b82b8ad49 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -1521,7 +1521,7 @@ void sock_map_close(struct sock *sk, long timeout) lock_sock(sk); rcu_read_lock(); - psock = sk_psock(sk); + psock = sk_psock_get(sk); if (unlikely(!psock)) { rcu_read_unlock(); release_sock(sk); @@ -1532,6 +1532,7 @@ void sock_map_close(struct sock *sk, long timeout) sock_map_remove_links(sk, psock); rcu_read_unlock(); sk_psock_stop(psock, true); + sk_psock_put(sk, psock); release_sock(sk); saved_close(sk, timeout); }