Re: [syzbot] [net?] KASAN: slab-use-after-free Read in unix_stream_read_actor (2)

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Shoaib Rao <rao.shoaib@oracle.com>
Date: Tue, 10 Sep 2024 15:30:08 -0700
> My fellow engineer let's first take a breath and calm down. We both are 
> trying to do the right thing. Now read my comments below and if I still 
> don't get it, please be patient, maybe I am not as smart as you are.
> 
> On 9/10/2024 2:53 PM, Kuniyuki Iwashima wrote:
> > From: Shoaib Rao <rao.shoaib@oracle.com>
> > Date: Tue, 10 Sep 2024 13:57:04 -0700
> >> The commit Message:
> >>
> >> syzbot reported use-after-free in unix_stream_recv_urg(). [0]
> >>
> >> The scenario is
> >>
> >>     1. send(MSG_OOB)
> >>     2. recv(MSG_OOB)
> >>        -> The consumed OOB remains in recv queue
> >>     3. send(MSG_OOB)
> >>     4. recv()
> >>        -> manage_oob() returns the next skb of the consumed OOB
> >>        -> This is also OOB, but unix_sk(sk)->oob_skb is not cleared
> >>     5. recv(MSG_OOB)
> >>        -> unix_sk(sk)->oob_skb is used but already freed
> > 
> > How did you miss this ?
> > 
> > Again, please read my patch and mails **carefully**.
> > 
> > unix_sk(sk)->oob_sk wasn't cleared properly and illegal access happens
> > in unix_stream_recv_urg(), where ->oob_skb is dereferenced.
> > 
> > Here's _technical_ thing that you want.
> 
> This is exactly what I am trying to point out to you.
> The skb has proper references and is NOT de-referenced because 
> __skb_datagram_iter() detects that the length is zero and returns EFAULT.

It's dereferenced as UNIXCB(skb).consumed first in
unix_stream_read_actor().

Then, 1 byte of data is copied without -EFAULT because
unix_stream_recv_urg() always passes 1 as chunk (size) to
recv_actor().

That's why I said KASAN should be working on your setup and suggested
running the repro with/without KASAN.  If KASAN is turned off, single
byte garbage is copied from the freed area.

See the last returned values below

Without KASAN:

---8<---
write(1, "executing program\n", 18
executing program
)     = 18
socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\333", iov_len=1}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, MSG_OOB|MSG_DONTWAIT
[   15.657345] queued OOB: ff1100000442c700
) = 1
recvmsg(3,
[   15.657793] reading OOB: ff1100000442c700
{msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0, msg_controllen=0, msg_flags=MSG_OOB}, MSG_OOB|MSG_WAITFORONE) = 1
sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\21", iov_len=1}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, MSG_OOB|MSG_NOSIGNAL|MSG_MORE
[   15.659830] queued OOB: ff1100000442c300
) = 1
recvfrom(3,
[   15.660272] free skb: ff1100000442c300
"\21", 125, MSG_DONTROUTE|MSG_TRUNC, NULL, NULL) = 1
recvmsg(3,
[   15.661014] reading OOB: ff1100000442c300
{msg_namelen=0, MSG_OOB|MSG_ERRQUEUE) = 1
---8<---


With KASAN:

---8<---
socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\333", iov_len=1}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, MSG_OOB|MSG_DONTWAIT
[  134.735962] queued OOB: ff110000099f0b40
) = 1
recvmsg(3,
[  134.736460] reading OOB: ff110000099f0b40
{msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0, msg_controllen=0, msg_flags=MSG_OOB}, MSG_OOB|MSG_WAITFORONE) = 1
sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\21", iov_len=1}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, MSG_OOB|MSG_NOSIGNAL|MSG_MORE
[  134.738554] queued OOB: ff110000099f0c80
) = 1
recvfrom(3,
[  134.739086] free skb: ff110000099f0c80
"\21", 125, MSG_DONTROUTE|MSG_TRUNC, NULL, NULL) = 1
recvmsg(3,
[  134.739792] reading OOB: ff110000099f0c80
 {msg_namelen=0}, MSG_OOB|MSG_ERRQUEUE) = -1 EFAULT (Bad address)
---8<---


> 
> See more below
> > 
> > ---8<---
> > # ./oob
> > executing program
> > [   25.773750] queued OOB: ff1100000947ba40
> > [   25.774110] reading OOB: ff1100000947ba40
> > [   25.774401] queued OOB: ff1100000947bb80
> > [   25.774669] free skb: ff1100000947bb80
> > [   25.774919] reading OOB: ff1100000947bb80
> > [   25.775172] ==================================================================
> > [   25.775654] BUG: KASAN: slab-use-after-free in unix_stream_read_actor+0x86/0xb0
> > ---8<---
> > 
> > ---8<---
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index a1894019ebd5..ccd9c47160a5 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -2230,6 +2230,7 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
> >   	__skb_queue_tail(&other->sk_receive_queue, skb);
> >   	spin_unlock(&other->sk_receive_queue.lock);
> >   
> > +	printk(KERN_ERR "queued OOB: %px\n", skb);
> >   	sk_send_sigurg(other);
> >   	unix_state_unlock(other);
> >   	other->sk_data_ready(other);
> > @@ -2637,6 +2638,7 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
> >   	spin_unlock(&sk->sk_receive_queue.lock);
> >   	unix_state_unlock(sk);
> >   
> > +	printk(KERN_ERR "reading OOB: %px\n", oob_skb);
> >   	chunk = state->recv_actor(oob_skb, 0, chunk, state);
> >   
> >   	if (!(state->flags & MSG_PEEK))
> > @@ -2915,7 +2917,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> >   
> >   			skb_unlink(skb, &sk->sk_receive_queue);
> >   			consume_skb(skb);
> > -
> > +			printk(KERN_ERR "free skb: %px\n", skb);
> 
> This printk is wrongly placed

It's not, because this just prints the address of OOB skb just after
it's illegally consumed without MSG_OOB.  The code is only called
for the illegal OOB during the repro.


> because the skb has been freed above, but 
> since it is just printing the pointer it should be OK, access to any skb 
> field will be an issue. You should move this printk to before 
> unix_stream_read_generic and print the refcnt on the skb and the length 
> of the data and verify what I am saying, that the skb has one refcnt and 
> zero length.

Note this is on top of net-next where no additional refcnt is taken
for OOB, so no need to print skb's refcnt.  Also the length is not
related because chunk is always 1.


> This is the kind of interaction I was looking for. If I have missed 
> something please be patient and let me know.
> 
> Regards,
> 
> Shoaib