Date: Fri, 14 Jun 2024 10:32:44 -0400
From: Ignat Korchagin <ignat@...udflare.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsa@...ulusnetworks.com, dsahern@...nel.org, 
	edumazet@...gle.com, kernel-team@...udflare.com, kraig@...gle.com, 
	kuba@...nel.org, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	pabeni@...hat.com, stable@...r.kernel.org
Subject: Re: [PATCH] net: do not leave dangling sk pointer in inet_create()/inet6_create()

On Thu, Jun 13, 2024 at 3:41 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Ignat Korchagin <ignat@...udflare.com>
> Date: Wed, 12 Jun 2024 14:22:36 -0400
> > > And curious if bpf_get_socket_cookie() can be called any socket
> > > family to trigger the splat.  e.g. ieee802154_create() seems to
> > > have the same bug.
> >
> > Just judging from the code - yes, indeed.
> >
> > > If so, how about clearing sock->sk in sk_common_release() ?
> >
> > This was my first thought, but I was a bit put off by the fact that
> > sk_common_release() is called from many places and the sk object
> > itself is reference counted. So not every call to sk_common_release()
> > seems to actually free the sk object.
>
> sk_common_release() is called
>
>   1. when we fail to create a socket (socket() or accept() syscall)
>   2. when we release the last refcount of the socket's file descriptor
>      (basically close() syscall)
>
> The issue only happens at 1. because we clear sock->sk at 2. in
> __sock_release() after calling sock->ops->release().
>
> So, we need not take care of these callers of sk_common_release().
>
>   - inet_release
>     - ->close()
>       - udp_lib_close
>       - ping_close
>       - raw_close
>       - rawv6_close
>       - l2tp_ip_close
>       - l2tp_ip6_close
>       - sctp_close
>   - ieee802154_sock_release
>     - ->close()
>       - raw_close
>       - dgram_close
>   - mctp_release
>     - ->close()
>       - mctp_sk_close
>   - pn_socket_release
>     - ->close()
>       - pn_sock_close
>       - pep_sock_close
>
> Then, the rest of the callers are:
>
>   - __sock_create
>     - pf->create()
>       - inet_create
>       - inet6_create
>       - ieee802154_create
>       - smc_create
>         - __smc_create
>
>   - setsockopt(TCP_ULP)
>     - smc_ulp_init
>       - __smc_create
>
>   - sctp_accept
>     - sctp_v4_create_accept_sk
>     - sctp_v6_create_accept_sk
>
> we need not care about sctp_v[46]_create_accept_sk() because they don't set
> sock->sk for the socket; we don't pass sock to sock_init_data(NULL, newsk)
> before calling sk_common_release().
>
> __sock_create() path and SMC's ULP path have the same issue, and
> sk_common_release() releases the last refcount of struct sock there.
>
> So, I think we can set NULL to sock->sk in sk_common_release().

Thanks for the explanation. Makes sense. I'll spin up a v2 with this
(and try to test it as well).

>
> > Secondly, I was put off by this
> > comment (which I don't fully understand TBH) [1]
> >
> > On the other hand - in inet/inet6_create() we definitely know that the
> > object would be freed, because we just created that.
> >
> > But if someone more familiar with the code confirms it is
> > better/possible to do in sk_common_release(), I'm happy to adjust and
> > it would be cleaner indeed.
> >
> > > ---8<---
> > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > index 8629f9aecf91..bbc94954d9bf 100644
> > > --- a/net/core/sock.c
> > > +++ b/net/core/sock.c
> > > @@ -3754,6 +3754,9 @@ void sk_common_release(struct sock *sk)
> > >          * until the last reference will be released.
> > >          */
> > >
> > > +       if (sk->sk_socket)
> > > +               sk->sk_socket->sk = NULL;
> > > +
> > >         sock_orphan(sk);
> > >
> > >         xfrm_sk_free_policy(sk);
> > > ---8<---
> >
> > [1]: https://elixir.bootlin.com/linux/v6.10-rc3/source/include/net/sock.h#L1985
>

Powered by blists - more mailing lists