net: unix: fix use-after-free

The unix_dgram_poll() routine calls sock_poll_wait() not only for the wait queue associated with the socket s that we are poll'ing against, but also calls sock_poll_wait() for a remote peer socket p, if it is connected. Thus, if we call poll()/select()/epoll() for the socket s, there are then a couple of code paths in which the remote peer socket p and its associated peer_wait queue can be freed before poll()/select()/epoll() have a chance to remove themselves from the remote peer socket.

The way that remote peer socket can be freed are:

1. If s calls connect() to a connect to a new socket other than p, it will drop its reference on p, and thus a close() on p will free it.

2. If we call close on p(), then a subsequent sendmsg() from s, will drop the final reference to p, allowing it to be freed.

Address this issue, by reverting unix_dgram_poll() to only register with the wait queue associated with s and register a callback with the remote peer socket on connect() that will wake up the wait queue associated with s. If scenarios 1 or 2 occur above we then simply remove the callback from the remote peer. This then presents the expected semantics to poll()/select()/ epoll().

I've implemented this for sock-type, SOCK_RAW, SOCK_DGRAM, and SOCK_SEQPACKET but not for SOCK_STREAM, since SOCK_STREAM does not use unix_dgram_poll().

Introduced in commit ec0d215f9420 ("af_unix: fix 'poll for write'/connected DGRAM sockets").

Tested-by Mathias Krause <[email protected]>

Signed-off-by Jason Baron <[email protected]>

Quoted 26 lines · Jason Baron

The unix_dgram_poll() routine calls sock_poll_wait() not only for the wait queue associated with the socket s that we are poll'ing against, but also calls sock_poll_wait() for a remote peer socket p, if it is connected. Thus, if we call poll()/select()/epoll() for the socket s, there are then a couple of code paths in which the remote peer socket p and its associated peer_wait queue can be freed before poll()/select()/epoll() have a chance to remove themselves from the remote peer socket.

The way that remote peer socket can be freed are:

1. If s calls connect() to a connect to a new socket other than p, it will drop its reference on p, and thus a close() on p will free it.

2. If we call close on p(), then a subsequent sendmsg() from s, will drop the final reference to p, allowing it to be freed.

Address this issue, by reverting unix_dgram_poll() to only register with the wait queue associated with s and register a callback with the remote peer socket on connect() that will wake up the wait queue associated with s. If scenarios 1 or 2 occur above we then simply remove the callback from the remote peer. This then presents the expected semantics to poll()/select()/ epoll().

I've implemented this for sock-type, SOCK_RAW, SOCK_DGRAM, and SOCK_SEQPACKET but not for SOCK_STREAM, since SOCK_STREAM does not use unix_dgram_poll().

Quoted 5 lines

Introduced in commit ec0d215f9420 ("af_unix: fix 'poll for write'/connected DGRAM sockets").

Tested-by Mathias Krause <[email protected]>

Signed-off-by Jason Baron <[email protected]>

While I think this approach works, I haven't seen where the current code leaks a reference.

Hi,

Quoted 23 lines · Jason Baron

The unix_dgram_poll() routine calls sock_poll_wait() not only for the wait queue associated with the socket s that we are poll'ing against, but also calls sock_poll_wait() for a remote peer socket p, if it is connected. Thus, if we call poll()/select()/epoll() for the socket s, there are then a couple of code paths in which the remote peer socket p and its associated peer_wait queue can be freed before poll()/select()/epoll() have a chance to remove themselves from the remote peer socket.

The way that remote peer socket can be freed are:

1. If s calls connect() to a connect to a new socket other than p, it will drop its reference on p, and thus a close() on p will free it.

2. If we call close on p(), then a subsequent sendmsg() from s, will drop the final reference to p, allowing it to be freed.

Address this issue, by reverting unix_dgram_poll() to only register with the wait queue associated with s and register a callback with the remote peer socket on connect() that will wake up the wait queue associated with s. If scenarios 1 or 2 occur above we then simply remove the callback from the remote peer. This then presents the expected semantics to poll()/select()/ epoll().

Quoted 8 lines

I've implemented this for sock-type, SOCK_RAW, SOCK_DGRAM, and SOCK_SEQPACKET but not for SOCK_STREAM, since SOCK_STREAM does not use unix_dgram_poll().

Introduced in commit ec0d215f9420 ("af_unix: fix 'poll for write'/connected DGRAM sockets").

Tested-by Mathias Krause <[email protected]>

Signed-off-by Jason Baron <[email protected]>

While I think this approach works, I haven't seen where the current code leaks a reference. Assignment to unix_peer(sk) in general take spin_lock and increment refcount. Are there bugs at the two places you referred to?

Is an easier fix just to use atomic_inc_not_zero(&sk->sk_refcnt) in unix_peer_get() which could also help other places?

Quoted 36 lines · Hannes Frederic Sowa

Hi,

Quoted 23 lines · Jason Baron

The unix_dgram_poll() routine calls sock_poll_wait() not only for the wait queue associated with the socket s that we are poll'ing against, but also calls sock_poll_wait() for a remote peer socket p, if it is connected. Thus, if we call poll()/select()/epoll() for the socket s, there are then a couple of code paths in which the remote peer socket p and its associated peer_wait queue can be freed before poll()/select()/epoll() have a chance to remove themselves from the remote peer socket.

The way that remote peer socket can be freed are:

1. If s calls connect() to a connect to a new socket other than p, it will drop its reference on p, and thus a close() on p will free it.

2. If we call close on p(), then a subsequent sendmsg() from s, will drop the final reference to p, allowing it to be freed.

Address this issue, by reverting unix_dgram_poll() to only register with the wait queue associated with s and register a callback with the remote peer socket on connect() that will wake up the wait queue associated with s. If scenarios 1 or 2 occur above we then simply remove the callback from the remote peer. This then presents the expected semantics to poll()/select()/ epoll().

Quoted 8 lines

I've implemented this for sock-type, SOCK_RAW, SOCK_DGRAM, and SOCK_SEQPACKET but not for SOCK_STREAM, since SOCK_STREAM does not use unix_dgram_poll().

Introduced in commit ec0d215f9420 ("af_unix: fix 'poll for write'/connected DGRAM sockets").

Tested-by Mathias Krause <[email protected]>

Signed-off-by Jason Baron <[email protected]>

Quoted 8 lines

While I think this approach works, I haven't seen where the current code leaks a reference. Assignment to unix_peer(sk) in general take spin_lock and increment refcount. Are there bugs at the two places you referred to?

Is an easier fix just to use atomic_inc_not_zero(&sk->sk_refcnt) in unix_peer_get() which could also help other places?

Hi,

So we could potentially inc the refcnt on the remote peer such that the remote peer does not free before the socket that has connected to it. However, then the socket that has taken the reference against the peer socket has to potentially record a number of remote sockets (all the ones that it has connected to over its lifetime), and then drop all of their refcnt's when it finally closes.

The reason for this is that with the current code when we do poll()/select()/epoll() on a socket with a peer socket, those calls take reference on the peer socket. Specifically, they record the remote peer whead, such that they can remove their callbacks when they return. So its not safe to just drop a reference on the remote peer when it closes because their might be outstanding poll()/select()/epoll() references pending.

Normally, poll()/select()/epoll() are waiting on a whead associated directly with the fd/file that they are waiting for.

The other point here is that the way this patch structures things is that when the socket connects to a new remote and hence disconnects from an existing remote, POLLOUT events will continue to be correctly delivered. That was not possible with the current structure of things b/c there was no way to inform poll to re-register with the remote peer whead. So, that means that the first test case here now works:

https://lkml.org/lkml/2015/10/4/154

Whereas with the old code test case would just hang for ever.

So yes there is a bit of code churn here, but I think it moves the code-base in a direction that not only solves this issue, but corrects additional poll() behaviors as well.

Hi Jason,

[auto build test ERROR on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-i0-201540 (attached as .config) reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

net/unix/af_unix.c: In function 'unix_dgram_writable':

Quoted 1 line

Quoted 1 line

net/unix/af_unix.c:2465:3: error: 'other_full' undeclared (first use in this function)

     *other_full = false;
      ^

net/unix/af_unix.c:2465:3: note: each undeclared identifier is reported only once for each function it appears in

These patches are against mainline, I can re-base to net-next, please let me know.

They have been tested against: https://lkml.org/lkml/2015/9/13/195,
which causes the use-after-free quite quickly and here:
https://lkml.org/lkml/2015/10/2/693.

From: Jason Baron <[email protected]>
Date: Fri, 9 Oct 2015 00:15:59 -0400

Quoted 6 lines

These patches are against mainline, I can re-base to net-next, please let me know.

They have been tested against: https://lkml.org/lkml/2015/9/13/195,
which causes the use-after-free quite quickly and here:
https://lkml.org/lkml/2015/10/2/693.

I'd like to understand how patches that don't even compile can be "tested"?

net/unix/af_unix.c: In function ‘unix_dgram_writable’:
net/unix/af_unix.c:2480:3: error: ‘other_full’ undeclared (first use in this function) net/unix/af_unix.c:2480:3: note: each undeclared identifier is reported only once for each function it appears in

Could you explain how that works, I'm having a hard time understanding this?

Quoted 13 lines · David Miller

From: Jason Baron <[email protected]>
Date: Fri, 9 Oct 2015 00:15:59 -0400

Quoted 6 lines

These patches are against mainline, I can re-base to net-next, please let me know.

They have been tested against: https://lkml.org/lkml/2015/9/13/195,
which causes the use-after-free quite quickly and here:
https://lkml.org/lkml/2015/10/2/693.

I'd like to understand how patches that don't even compile can be "tested"?

Quoted 6 lines

net/unix/af_unix.c: In function ‘unix_dgram_writable’:
net/unix/af_unix.c:2480:3: error: ‘other_full’ undeclared (first use in this function) net/unix/af_unix.c:2480:3: note: each undeclared identifier is reported only once for each function it appears in

Could you explain how that works, I'm having a hard time understanding this?

This is basicallly a workaround for the problem that it's not possible to tell epoll to let go of a certain wait queue: Instead of registering the peer_wait queue via sock_poll_wait, a wait_queue_t under control of the af_unix.c code is linked onto it which relays a wake up on the peer_wait queue to the 'ordinary' wait queue associated with the polled socket via custom wake function. But (at least the code I looked it) it enqueues a unix socket on connect which has certain side effects (in particular, /dev/log will have a seriously large wait queue of entirely uninterested peers) and in many cases, this is simply not necessary, as the additional peer_wait event is only interesting in case a peer of a fan-in socket (like /dev/log) happens to be waiting for writeabilty via poll/ select/ epoll/ ...

Since the wait queue handling code is now under control of the af_unix.c code, it can remove itself from the peer_wait queue prior to dropping its reference to a peer on disconnect or on detecting a dead peer in unix_dgram_sendmsg.
--

From: Jason Baron <[email protected]>
Date: Fri, 9 Oct 2015 00:15:59 -0400

Quoted 6 lines

These patches are against mainline, I can re-base to net-next, please let me know.

They have been tested against: https://lkml.org/lkml/2015/9/13/195,
which causes the use-after-free quite quickly and here:
https://lkml.org/lkml/2015/10/2/693.

I'd like to understand how patches that don't even compile can be "tested"?

net/unix/af_unix.c: In function ‘unix_dgram_writable’:
net/unix/af_unix.c:2480:3: error: ‘other_full’ undeclared (first use in this function) net/unix/af_unix.c:2480:3: note: each undeclared identifier is reported only once for each function it appears in

Could you explain how that works, I'm having a hard time understanding this?

Also please address Hannes's feedback, thanks.

I'm happy to re-post the series or whatever makes things easiest for you.