[PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Commit 9060cb719e61 ("net: crypto set sk to NULL when af_alg_release.")
fixed a use-after-free in sockfs_setattr() when an AF_ALG socket is
closed concurrently with fchownat().  However, it ignored that many
other proto_ops::release() methods don't set sock->sk to NULL and
therefore allow the same use-after-free:

    - base_sock_release
    - bnep_sock_release
    - cmtp_sock_release
    - data_sock_release
    - dn_release
    - hci_sock_release
    - hidp_sock_release
    - iucv_sock_release
    - l2cap_sock_release
    - llcp_sock_release
    - llc_ui_release
    - rawsock_release
    - rfcomm_sock_release
    - sco_sock_release
    - svc_release
    - vcc_release
    - x25_release

Rather than fixing all these and relying on every socket type to get
this right forever, just make __sock_release() set sock->sk to NULL
itself after calling proto_ops::release().

Reproducer that produces the KASAN splat when any of these socket types
are configured into the kernel:

    #include <pthread.h>
    #include <stdlib.h>
    #include <sys/socket.h>
    #include <unistd.h>

    pthread_t t;
    volatile int fd;

    void *close_thread(void *arg)
    {
        for (;;) {
            usleep(rand() % 100);
            close(fd);
        }
    }

    int main()
    {
        pthread_create(&t, NULL, close_thread, NULL);
        for (;;) {
            fd = socket(rand() % 50, rand() % 11, 0);
            fchownat(fd, "", 1000, 1000, 0x1000);
            close(fd);
        }
    }

Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.")
Cc: <stable@vger.kernel.org> # v4.10+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---

NOTE: I am not an expert in the networking code, so please carefully
check that I haven't missed some reason why this simple fix won't do.

 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/socket.c b/net/socket.c
index d80d87a395ea..320f51b22b19 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -577,6 +577,7 @@ static void __sock_release(struct socket *sock, struct inode *inode)
 		if (inode)
 			inode_lock(inode);
 		sock->ops->release(sock);
+		sock->sk = NULL;
 		if (inode)
 			inode_unlock(inode);
 		sock->ops = NULL;
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Al Viro]

On Thu, Feb 21, 2019 at 02:13:56PM -0800, Eric Biggers wrote:

> Rather than fixing all these and relying on every socket type to get
> this right forever, just make __sock_release() set sock->sk to NULL
> itself after calling proto_ops::release().

	Is there any case when we would want sock->sk non-NULL when
sock->sk->sk_socket is NULL?

	IOW, why not make sock_orphan() take care of that, at the same
time we dissociate sock from socket?  After all, on the other end
of things we have both sock_graft() and sock_init_data() set both
sock->sk and sk->sk_socket...

	Looking at the assignments to sock->sk, I see
* atalk_release(), bcm_release(), raw_release(), pfkey_release(),
pppol2tp_release(), packet_release(), smc_release(), xsk_release() -
directly after sock_orphan()
* rxrpc_release() - directly *before* sock_orphan()

* ax25_release() - somewhat after sock_orphan(); no idea if anything
done in between needs it still set.  Doesn't looks like there could
be - readers of sock->sk in there are all in methods that can't
overlap with ->release().
* kcm_release() - similar.
* rds_release() - similar.
* tipc_release() - similar.
* unix_accept() - similar, and there I'm fairly sure it could be
done right after sock_orphan()
* netrom_release() - similar.  BTW, why the hell does nr_accept()
check for NULL sock->sk, while other methods (callable for exact
same sockets) assume that it's non-NULL?  AFAICS, the check in
nr_accept() is pointless - nr_create() can leave sock->sk NULL
only if it fails, in which case it's going to be immediately
hit by ->release() and freed by iput() in sock_release().
* rose_release() - similar, complete with ->accept() oddity.

* caif_release() - done *before* sock_orphan(), with other bit of
the latter duplicated just prior.  NFI why; it messes with
debugfs, so there's a whole kettle of copulating slugs involved ;-/

* ieee802154_sock_release(), inet_release(), pn_socket_release() -
done before calliing into protocol's ->close(), which ends up
calling sock_orphan(), either directly or via sk_common_release().
I suspect that we would be find with zeroing sock->sk delayed
until sock_orphan() in all of those, but that needs more RTFS
than I want to go into at the moment.

* netlink_release() - somewhat after sock_orphan(); not sure,
calls of ->netlink_unbind() done inbetween might want it still
set.

* qrtr_release() - lacks sock_orphan(), might be broken

* vsock_releae() - NFI.

* vcc_create() - clears sock->sk in the very beginning; AFAICS
that's pointless.

Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Eric Dumazet]

On 02/21/2019 02:13 PM, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 9060cb719e61 ("net: crypto set sk to NULL when af_alg_release.")
> fixed a use-after-free in sockfs_setattr() when an AF_ALG socket is
> closed concurrently with fchownat().  However, it ignored that many
> other proto_ops::release() methods don't set sock->sk to NULL and
> therefore allow the same use-after-free:
> 

I fail to see how setting a pointer to NULL can avoid races.


We lack some kind of protection, rcu or something, if another thread can change sock->sk at anytime
while sockfs_setattr() is used.

sockfs_setattr()
...
     if (sock->sk)

// even if sock->sk was not NULL for the if (...).

// it can be NULL right now, compiler could read sock->sk a second time and catch a NULL.

        sock->sk->sk_uid = iattr->ia_uid;



>     - base_sock_release
>     - bnep_sock_release
>     - cmtp_sock_release
>     - data_sock_release
>     - dn_release
>     - hci_sock_release
>     - hidp_sock_release
>     - iucv_sock_release
>     - l2cap_sock_release
>     - llcp_sock_release
>     - llc_ui_release
>     - rawsock_release
>     - rfcomm_sock_release
>     - sco_sock_release
>     - svc_release
>     - vcc_release
>     - x25_release
> 
> Rather than fixing all these and relying on every socket type to get
> this right forever, just make __sock_release() set sock->sk to NULL
> itself after calling proto_ops::release().
> 
> Reproducer that produces the KASAN splat when any of these socket types
> are configured into the kernel:
> 
>     #include <pthread.h>
>     #include <stdlib.h>
>     #include <sys/socket.h>
>     #include <unistd.h>
> 
>     pthread_t t;
>     volatile int fd;
> 
>     void *close_thread(void *arg)
>     {
>         for (;;) {
>             usleep(rand() % 100);
>             close(fd);
>         }
>     }
> 
>     int main()
>     {
>         pthread_create(&t, NULL, close_thread, NULL);
>         for (;;) {
>             fd = socket(rand() % 50, rand() % 11, 0);
>             fchownat(fd, "", 1000, 1000, 0x1000);
>             close(fd);
>         }
>     }
> 
> Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.")
> Cc: <stable@vger.kernel.org> # v4.10+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> 
> NOTE: I am not an expert in the networking code, so please carefully
> check that I haven't missed some reason why this simple fix won't do.
> 
>  net/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/socket.c b/net/socket.c
> index d80d87a395ea..320f51b22b19 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -577,6 +577,7 @@ static void __sock_release(struct socket *sock, struct inode *inode)
>  		if (inode)
>  			inode_lock(inode);
>  		sock->ops->release(sock);
> +		sock->sk = NULL;
>  		if (inode)
>  			inode_unlock(inode);
>  		sock->ops = NULL;
> 

Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Eric Biggers]

Hi Eric,

On Fri, Feb 22, 2019 at 09:45:35AM -0800, Eric Dumazet wrote:
> 
> 
> On 02/21/2019 02:13 PM, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Commit 9060cb719e61 ("net: crypto set sk to NULL when af_alg_release.")
> > fixed a use-after-free in sockfs_setattr() when an AF_ALG socket is
> > closed concurrently with fchownat().  However, it ignored that many
> > other proto_ops::release() methods don't set sock->sk to NULL and
> > therefore allow the same use-after-free:
> > 
> 
> I fail to see how setting a pointer to NULL can avoid races.
> 
> 
> We lack some kind of protection, rcu or something, if another thread can change sock->sk at anytime
> while sockfs_setattr() is used.
> 
> sockfs_setattr()
> ...
>      if (sock->sk)
> 
> // even if sock->sk was not NULL for the if (...).
> 
> // it can be NULL right now, compiler could read sock->sk a second time and catch a NULL.
> 
>         sock->sk->sk_uid = iattr->ia_uid;
> 
> 

->setattr() is called under inode_lock(), which __sock_release() also takes.  So
the uses of sock->sk are serialized.  See commit 6d8c50dcb029 ("socket: close
race condition between sock_close() and sockfs_setattr()").

The issue now is that if ->setattr() happens *after* __sock_release() (which is
possible if fchownat() gets the reference to the file's 'struct path', then the
file is close()d by another thread, then fchownat() continues), it will see
stale sock->sk because for many socket types it wasn't set to NULL earlier.

- Eric

Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Cong Wang]

On Thu, Feb 21, 2019 at 2:14 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> Commit 9060cb719e61 ("net: crypto set sk to NULL when af_alg_release.")
> fixed a use-after-free in sockfs_setattr() when an AF_ALG socket is
> closed concurrently with fchownat().  However, it ignored that many
> other proto_ops::release() methods don't set sock->sk to NULL and
> therefore allow the same use-after-free:
>
>     - base_sock_release
>     - bnep_sock_release
>     - cmtp_sock_release
>     - data_sock_release
>     - dn_release
>     - hci_sock_release
>     - hidp_sock_release
>     - iucv_sock_release
>     - l2cap_sock_release
>     - llcp_sock_release
>     - llc_ui_release
>     - rawsock_release
>     - rfcomm_sock_release
>     - sco_sock_release
>     - svc_release
>     - vcc_release
>     - x25_release
>
> Rather than fixing all these and relying on every socket type to get
> this right forever, just make __sock_release() set sock->sk to NULL
> itself after calling proto_ops::release().
>
> Reproducer that produces the KASAN splat when any of these socket types
> are configured into the kernel:
>
>     #include <pthread.h>
>     #include <stdlib.h>
>     #include <sys/socket.h>
>     #include <unistd.h>
>
>     pthread_t t;
>     volatile int fd;
>
>     void *close_thread(void *arg)
>     {
>         for (;;) {
>             usleep(rand() % 100);
>             close(fd);
>         }
>     }
>
>     int main()
>     {
>         pthread_create(&t, NULL, close_thread, NULL);
>         for (;;) {
>             fd = socket(rand() % 50, rand() % 11, 0);
>             fchownat(fd, "", 1000, 1000, 0x1000);
>             close(fd);
>         }
>     }
>
> Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.")
> Cc: <stable@vger.kernel.org> # v4.10+
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Eric Dumazet]

On 02/22/2019 09:57 AM, Eric Biggers wrote:

> ->setattr() is called under inode_lock(), which __sock_release() also takes.  So
> the uses of sock->sk are serialized.  See commit 6d8c50dcb029 ("socket: close
> race condition between sock_close() and sockfs_setattr()").

Oh right, we added another inode_lock()/inode_unlock() for sock_close()


> 
> The issue now is that if ->setattr() happens *after* __sock_release() (which is
> possible if fchownat() gets the reference to the file's 'struct path', then the
> file is close()d by another thread, then fchownat() continues), it will see
> stale sock->sk because for many socket types it wasn't set to NULL earlier.
> 
> - Eric
> 

Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[Al Viro]

On Fri, Feb 22, 2019 at 10:25:09AM -0800, Eric Dumazet wrote:
> 
> 
> On 02/22/2019 09:57 AM, Eric Biggers wrote:
> 
> > ->setattr() is called under inode_lock(), which __sock_release() also takes.  So
> > the uses of sock->sk are serialized.  See commit 6d8c50dcb029 ("socket: close
> > race condition between sock_close() and sockfs_setattr()").
> 
> Oh right, we added another inode_lock()/inode_unlock() for sock_close()

An interesting question is whether anything else will be confused by
	sock->sk && sock->sk->sk_socket != sock

I'd still like to figure out if we could simply make sock_orphan()
do something like
	if (likely(sk->sk_socket))
		sk->sk_socket->sk = NULL;
just before sk_set_socket(sk, NULL);

That would make for much easier rules; the question is whether anything
relies upon the windows when linkage between socket and sock is not
symmetrical...

Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()

[David Miller]

From: Eric Biggers <ebiggers@kernel.org>
Date: Thu, 21 Feb 2019 14:13:56 -0800

> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 9060cb719e61 ("net: crypto set sk to NULL when af_alg_release.")
> fixed a use-after-free in sockfs_setattr() when an AF_ALG socket is
> closed concurrently with fchownat().  However, it ignored that many
> other proto_ops::release() methods don't set sock->sk to NULL and
> therefore allow the same use-after-free:
 ...
> Rather than fixing all these and relying on every socket type to get
> this right forever, just make __sock_release() set sock->sk to NULL
> itself after calling proto_ops::release().
> 
> Reproducer that produces the KASAN splat when any of these socket types
> are configured into the kernel:
 ...
> Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Applied and queued up for -stable, thanks Eric.