[bpf-next] sock_map: fix a potential use-after-free in sock_map_close()

Commit Message

Cong Wang April 8, 2021, 3:05 a.m. UTC

From: Cong Wang <[e-mail]>

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: [e-mail]
Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
Cc: John Fastabend <[e-mail]>
Cc: Daniel Borkmann <[e-mail]>
Cc: Jakub Sitnicki <[e-mail]>
Cc: Lorenz Bauer <[e-mail]>
Signed-off-by: Cong Wang <[e-mail]>
---
 net/core/sock_map.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

John Fastabend April 9, 2021, 12:26 a.m. UTC | #1

Cong Wang wrote:
> From: Cong Wang <[e-mail]>
> 
> 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: [e-mail]
> Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
> Cc: John Fastabend <[e-mail]>
> Cc: Daniel Borkmann <[e-mail]>
> Cc: Jakub Sitnicki <[e-mail]>
> Cc: Lorenz Bauer <[e-mail]>
> Signed-off-by: Cong Wang <[e-mail]>
> ---
>  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
>

Cong Wang April 9, 2021, 4:08 a.m. UTC | #2

On Thu, Apr 8, 2021 at 5:26 PM John Fastabend <[e-mail]> wrote:
>
> Cong Wang wrote:
> > From: Cong Wang <[e-mail]>
> >
> > 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: [e-mail]
> > Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
> > Cc: John Fastabend <[e-mail]>
> > Cc: Daniel Borkmann <[e-mail]>
> > Cc: Jakub Sitnicki <[e-mail]>
> > Cc: Lorenz Bauer <[e-mail]>
> > Signed-off-by: Cong Wang <[e-mail]>
> > ---
> >  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.

John Fastabend April 9, 2021, 7:42 p.m. UTC | #3

Cong Wang wrote:
> On Thu, Apr 8, 2021 at 5:26 PM John Fastabend <[e-mail]> wrote:
> >
> > Cong Wang wrote:
> > > From: Cong Wang <[e-mail]>
> > >
> > > 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: [e-mail]
> > > Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
> > > Cc: John Fastabend <[e-mail]>
> > > Cc: Daniel Borkmann <[e-mail]>
> > > Cc: Jakub Sitnicki <[e-mail]>
> > > Cc: Lorenz Bauer <[e-mail]>
> > > Signed-off-by: Cong Wang <[e-mail]>
> > > ---
> > >  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 <[e-mail]>

Jakub Sitnicki April 12, 2021, 8:56 a.m. UTC | #4

On Thu, Apr 08, 2021 at 05:05 AM CEST, Cong Wang wrote:
> From: Cong Wang <[e-mail]>
>
> 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: [e-mail]
> Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
> Cc: John Fastabend <[e-mail]>
> Cc: Daniel Borkmann <[e-mail]>
> Cc: Jakub Sitnicki <[e-mail]>
> Cc: Lorenz Bauer <[e-mail]>
> Signed-off-by: Cong Wang <[e-mail]>
> ---

Acked-by: Jakub Sitnicki <[e-mail]>

[e-mail] April 12, 2021, 3:40 p.m. UTC | #5

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 <[e-mail]>
> 
> 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

Patch

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