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

[Jann Horn]

On Sun, May 17, 2026 at 9:21 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> 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<---

Ah, thanks, I will integrate that context in the commit message.

> > 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 ?

Will do.