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

[Shoaib Rao <rao.shoaib@oracle.com>]

On 9/10/2024 3:59 PM, Kuniyuki Iwashima wrote:
> 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().
> 

That does not matter as the skb still has a refernce. That is why I 
asked you to print the reference count.

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

Can you verify this because IIRC it is not de-refernced. AFAIK, KASAN 
does nothing that would cause returning EFAULT and if KASAN does spot 
this illegal access why is it not pancing the system or producing a report.

This is where we disagree.

Shoaib

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