BPF Archive on lore.kernel.org
 help / color / Atom feed
* [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close()
@ 2021-04-08  3:05 Cong Wang
  2021-04-09  0:26 ` John Fastabend
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Cong Wang @ 2021-04-08  3:05 UTC (permalink / raw)
  To: netdev
  Cc: bpf, Cong Wang, syzbot+7b6548ae483d6f4c64ae, John Fastabend,
	Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer

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();
-	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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close()
  2021-04-08  3:05 [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close() Cong Wang
@ 2021-04-09  0:26 ` John Fastabend
  2021-04-09  4:08   ` Cong Wang
  2021-04-12  8:56 ` Jakub Sitnicki
  2021-04-12 15:40 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2021-04-09  0:26 UTC (permalink / raw)
  To: Cong Wang, netdev
  Cc: bpf, Cong Wang, syzbot+7b6548ae483d6f4c64ae, John Fastabend,
	Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer

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
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close()
  2021-04-09  0:26 ` John Fastabend
@ 2021-04-09  4:08   ` Cong Wang
  2021-04-09 19:42     ` John Fastabend
  0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2021-04-09  4:08 UTC (permalink / raw)
  To: John Fastabend
  Cc: Linux Kernel Network Developers, bpf, Cong Wang, syzbot,
	Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close()
  2021-04-09  4:08   ` Cong Wang
@ 2021-04-09 19:42     ` John Fastabend
  0 siblings, 0 replies; 6+ messages in thread
From: John Fastabend @ 2021-04-09 19:42 UTC (permalink / raw)
  To: Cong Wang, John Fastabend
  Cc: Linux Kernel Network Developers, bpf, Cong Wang, syzbot,
	Daniel Borkmann, Jakub Sitnicki, Lorenz Bauer

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close()
  2021-04-08  3:05 [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close() Cong Wang
  2021-04-09  0:26 ` John Fastabend
@ 2021-04-12  8:56 ` Jakub Sitnicki
  2021-04-12 15:40 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 6+ messages in thread
From: Jakub Sitnicki @ 2021-04-12  8:56 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, bpf, Cong Wang, syzbot+7b6548ae483d6f4c64ae,
	John Fastabend, Daniel Borkmann, Lorenz Bauer

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close()
  2021-04-08  3:05 [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close() Cong Wang
  2021-04-09  0:26 ` John Fastabend
  2021-04-12  8:56 ` Jakub Sitnicki
@ 2021-04-12 15:40 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-04-12 15:40 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, bpf, cong.wang, syzbot+7b6548ae483d6f4c64ae,
	john.fastabend, daniel, jakub, lmb

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08  3:05 [Patch bpf-next] sock_map: fix a potential use-after-free in sock_map_close() Cong Wang
2021-04-09  0:26 ` John Fastabend
2021-04-09  4:08   ` Cong Wang
2021-04-09 19:42     ` John Fastabend
2021-04-12  8:56 ` Jakub Sitnicki
2021-04-12 15:40 ` patchwork-bot+netdevbpf

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git