On Tue, Oct 24, 2023 at 02:39 PM -07, John Fastabend wrote:
> Jakub Sitnicki wrote:
>> On Mon, Oct 16, 2023 at 12:08 PM -07, John Fastabend wrote:
>> > AF_UNIX sockets are a paired socket. So sending on one of the pairs
>> > will lookup the paired socket as part of the send operation. It is
>> > possible however to put just one of the pairs in a BPF map. This
>> > currently increments the refcnt on the sock in the sockmap to
>> > ensure it is not free'd by the stack before sockmap cleans up its
>> > state and stops any skbs being sent/recv'd to that socket.
>> >
>> > But we missed a case. If the peer socket is closed it will be
>> > free'd by the stack. However, the paired socket can still be
>> > referenced from BPF sockmap side because we hold a reference
>> > there. Then if we are sending traffic through BPF sockmap to
>> > that socket it will try to dereference the free'd pair in its
>> > send logic creating a use after free. And following splat,
>> >
>> > [59.900375] BUG: KASAN: slab-use-after-free in sk_wake_async+0x31/0x1b0
>> > [59.901211] Read of size 8 at addr ffff88811acbf060 by task kworker/1:2/954
>> > [...]
>> > [59.905468] Call Trace:
>> > [59.905787] <TASK>
>> > [59.906066] dump_stack_lvl+0x130/0x1d0
>> > [59.908877] print_report+0x16f/0x740
>> > [59.910629] kasan_report+0x118/0x160
>> > [59.912576] sk_wake_async+0x31/0x1b0
>> > [59.913554] sock_def_readable+0x156/0x2a0
>> > [59.914060] unix_stream_sendmsg+0x3f9/0x12a0
>> > [59.916398] sock_sendmsg+0x20e/0x250
>> > [59.916854] skb_send_sock+0x236/0xac0
>> > [59.920527] sk_psock_backlog+0x287/0xaa0
>>
>> Isn't the problem here that unix_stream_sendmsg doesn't grab a ref to
>> peer sock? Unlike unix_dgram_sendmsg which uses the unix_peer_get
>> helper.
>
> It does by my read. In unix_stream_connect we have,
>
> sock_hold(sk);
> unix_peer(newsk) = sk;
> newsk->sk_state = TCP_ESTABLISHED;
>
> where it assigns the peer sock. unix_dgram_connect() also calls
> sock_hold() but through the path that does the socket lookup, such as
> unix_find_other().
>
> The problem I see is before the socket does the kfree on the
> sock we need to be sure the backlog is canceled and the skb list
> ingress_skb is purged. If we don't ensure this then the redirect
> will
>
> My model is this,
>
> s1 c1
> refcnt 1 1
> connect 2 2
> psock 3 3
> send(s1) ...
> close(s1) 2 1 <- close drops psock count also
> close(c1) 0 0
>
> The important bit here is the psock has a refcnt on the
> underlying sock (psock->sk) and wont dec that until after
> cancel_delayed_work_sync() completes. This ensures the
> backlog wont try to sendmsg() on that sock after we free
> it. We also check for SOCK_DEAD and abort to avoid sending
> over a socket that has been marked DEAD.
>
> So... After close(s1) the only thing keeping that sock
> around is c1. Then we close(c1) that call path is
>
> unix_release
> close()
> unix_release_sock()
> skpair = unix_peer(sk);
> ...
> sock_put(skpair); <- trouble here
>
> The release will call sock_put() on the pair socket and
> dec it to 0 where it gets free'd through sk_free(). But
> now the trouble is we haven't waited for cancel_delayed_work_sync()
> on the c1 socket yet so backlog can still run. When it does
> run it may try to send a pkg over socket s1. OK right up until
> the sendmsg(s1, ...) does a peer lookup and derefs the peer
> socket. The peer socket was free'd earlier so use after free.
>
> The question I had originally was this is odd, we are allowing
> a sendmsg(s1) over a socket while its in unix_release(). We
> used to take the sock lock from the backlog that was dropped
> in the name of performance, but it creates these races.
>
> Other fixes I considered. First adding sock lock back to
> backlog. But that punishes the UDP and TCP cases that don't
> have this problem. Set the SOCK_DEAD flag earlier or check
> later but this just makes the race smaller doesn't really
> eliminate it.
>
> So this patch is what I came up with.
What I was getting at is that we could make it safe to call sendmsg on a
unix stream sock while its peer is being release. And not just for
sockmap. I expect io_uring might have the same problem. But I didn't
actually check yet.
For that we could keep a ref to peer for the duration of sendmsg call,
like unix dgram does. Then 'other' doesn't become a stale pointer before
we're done with it.
Bumping ref count on each sendmsg is not free, but maybe its
acceptable. Unix dgram sockets live with it.
With a patch like below, I'm no longer able to trigger an UAF splat.
WDYT?
---8<---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 3e8a04a13668..48cf19ea9294 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2198,7 +2198,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
goto out_err;
} else {
err = -ENOTCONN;
- other = unix_peer(sk);
+ other = unix_peer_get(sk);
if (!other)
goto out_err;
}
@@ -2282,6 +2282,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
}
#endif
+ sock_put(other);
scm_destroy(&scm);
return sent;
@@ -2294,6 +2295,8 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
send_sig(SIGPIPE, current, 0);
err = -EPIPE;
out_err:
+ if (other)
+ sock_put(other);
scm_destroy(&scm);
return sent ? : err;
}
