Re: [PATCH 1/3] af_unix: Fix UAF read of tail->len in unix_stream_data_wait()

[Kuniyuki Iwashima]

On Fri, May 15, 2026 at 11:54 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
>
> unix_stream_data_wait() does skb_peek_tail(&sk->sk_receive_queue) without
> holding any lock that prevents SKBs on that queue from being dequeued and
> freed.
> This has been the case since commit 79f632c71bea ("unix/stream: fix
> peeking with an offset larger than data in queue").
> The first consequence of this is that the pointer comparison
> `tail != last` can be false even if `last` semantically refers to an
> already-freed SKB while `tail` is a new SKB allocated at the same address;
> which can cause unix_stream_data_wait() to wrongly keep blocking after new
> data has arrived, but only in a weird scenario where a peeking recv() and
> a normal recv() on the same socket are racing, which is probably not a
> real problem.
>
> But since commit 2b514574f7e8 ("net: af_unix: implement splice for stream
> af_unix sockets"), `tail` is actually dereferenced, which can cause UAF in
> the following race scenario (where test_setup() runs single-threaded,
> and afterwards, test_thread1() and test_thread2() run concurrently in
> two threads:
> ```
> static int socks[2];
> void test_setup(void) {
>   socketpair(AF_UNIX, SOCK_STREAM, 0, socks);
>   send(socks[1], "A", 1, 0);
>   int peekoff = 1;
>   setsockopt(socks[0], SOL_SOCKET, SO_PEEK_OFF, &peekoff, sizeof(peekoff));
> }
> void test_thread1(void) {
>   char dummy;
>   recv(socks[0], &dummy, 1, MSG_PEEK);
> }
> void test_thread2(void) {
>   char dummy;
>   recv(socks[0], &dummy, 1, 0);
>   shutdown(socks[1], SHUT_WR);
> }
> ```
>
> when racing like this:
> ```
> thread1                       thread2
> unix_stream_read_generic
>   mutex_lock(&u->iolock)
>   skb_peek(&sk->sk_receive_queue)
>   skb_peek_next(skb, &sk->sk_receive_queue)
>   mutex_unlock(&u->iolock)
>                               unix_stream_read_generic
>                                 unix_state_lock(sk)
>                                 skb_peek(&sk->sk_receive_queue)
>                                 unix_state_unlock(sk)
>   unix_stream_data_wait
>     unix_state_lock(sk)
>     tail = skb_peek_tail(&sk->sk_receive_queue)
>                                 spin_lock(&sk->sk_receive_queue.lock)
>                                 __skb_unlink(skb, &sk->sk_receive_queue)
>                                 spin_unlock(&sk->sk_receive_queue.lock)
>                                 consume_skb(skb) [frees the SKB]
>     `tail != last`: false
>     `tail`: true
>     `tail->len != last_len` ***UAF***
> ```
>
> Fix the UAF by removing the read of tail->len; checking tail->len would
> only make sense if SKBs in the receive queue of a UNIX socket could grow,
> which AFAIK is not supposed to happen.

I posted the same patch 2 years ago (and forgot to respin),
which has the historical context.
https://lore.kernel.org/netdev/20240530164256.40223-1-kuniyu@xxxxxxxxxx/

---8<---
When commit 869e7c62486e ("net: af_unix: implement stream sendpage
support") added sendpage() support, data could be appended to the last
skb in the receiver's queue.

That's why we needed to check if the length of the last skb was changed
while waiting for new data in unix_stream_data_wait().

However, commit a0dbf5f818f9 ("af_unix: Support MSG_SPLICE_PAGES") and
commit 57d44a354a43 ("unix: Convert unix_stream_sendpage() to use
MSG_SPLICE_PAGES") refactored sendmsg(), and now data is always added
to a new skb.
---8<---


>
> Fixes: 2b514574f7e8 ("net: af_unix: implement splice for stream af_unix sockets")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>

Can you post this patch separately to net.git by specifying
[PATCH net v2] in Subject ?

The later patches are net-next material.