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