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 11:33 AM, Kuniyuki Iwashima wrote:
> From: Shoaib Rao <rao.shoaib@oracle.com>
> Date: Tue, 10 Sep 2024 11:16:59 -0700
>> On 9/10/2024 10:57 AM, Kuniyuki Iwashima wrote:
>>> From: Shoaib Rao <rao.shoaib@oracle.com>
>>> Date: Tue, 10 Sep 2024 09:55:03 -0700
>>>> On 9/9/2024 5:48 PM, Kuniyuki Iwashima wrote:
>>>>> From: Shoaib Rao <rao.shoaib@oracle.com>
>>>>> Date: Mon, 9 Sep 2024 17:29:04 -0700
>>>>>> I have some more time investigating the issue. The sequence of packet
>>>>>> arrival and consumption definitely points to an issue with OOB handling
>>>>>> and I will be submitting a patch for that.
>>>>>
>>>>> It seems a bit late.
>>>>> My patches were applied few minutes before this mail was sent.
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/netdev/172592764315.3964840.16480083161244716649.git-patchwork-notify@kernel.org/__;!!ACWV5N9M2RV99hQ!M806VrqNEGFgGXEoWG85msKAdFPXup7RzHy9Kt4q_HOfpPWsjNHn75KyFK3a3jWvOb9EEQuFGOjpqgk$
>>>>>
>>>>
>>>> That is a subpar fix. I am not sure why the maintainers accepted the fix
>>>> when it was clear that I was still looking into the issue.
>>>
>>> Just because it's not a subpar fix and you were slow and wrong,
>>> clining to triggering the KASAN splat without thinking much.
>>>
>>>
>>>> Plus the
>>>> claim that it fixes the panic is absolutely wrong.
>>>
>>> The _root_ cause of the splat is mishandling of OOB in manage_oob()
>>> which causes UAF later in another recvmsg().
>>>
>>> Honestly your patch is rather a subpar fix to me, few points:
>>>
>>>     1. The change conflicts with net-next as we have already removed
>>>        the additional unnecessary refcnt for OOB skb that has caused
>>>        so many issue reported by syzkaller
>>>
>>>     2. Removing OOB skb in queue_oob() relies on the unneeded refcnt
>>>        but it's not mentioned; if merge was done wrongly, another UAF
>>>        will be introduced in recvmsg()
>>>
>>>     3. Even the removing logic is completely unnecessary if manage_oob()
>>>        is changed
>>>
>>>     4. The scan_again: label is misplaced; two consecutive empty OOB skbs
>>>        never exist at the head of recvq
>>>
>>>     5. ioctl() is not fixed
>>>
>>>     6. No test added
>>>
>>>     7. Fixes: tag is bogus
>>>
>>>     8. Subject lacks target tree and af_unix prefix
>>
>> If you want to nit pick, nit pick away, Just because the patch email
>> lacks proper formatting does not make the patch technically inferior.
> 
> Ironically you just nit picked 8.
> 
> 

I have no idea what you mean. I am more worried about technical 
correctness than formatting -- That does not mean formatting is not 
necessary.

>> My
>> fix is a proper fix not a hack. The change in queue_oob is sufficient to
>> fix all issues including SIOCATMARK. The fix in manage_oob is just for
>> correctness.
> 
> Then, it should be WARN_ON_ONCE() not to confuse future readers.
> 
> 
>> In your fix I specifically did not like the change made to
>> fix SIOCATMARK.
> 
> I don't like that part too, but it's needed to avoid the additional refcnt
> that is much worse as syzbot has been demonstrating.
> 

syzbot has nothing to do with doing a proper fix. One has to understand 
the code though to do the fix at the proper location.

> 
>>
>> What is most worrying is claim to fixing a panic when it can not even
>> happen with the bug.
> 
> It's only on your setup.  syzbot and I were able to trigger that with
> the bug.
> 

Really, what is so special about my setup that kasan does not like? Can 
you point me to the exact location where the access is made?

I am at least glad that you have backed off your assertion that my 
change does not fix the ioctl. I am sure if I keep pressing you, you 
will back off the panic claim as well. You yourself admitted you did not 
know why kasan was not panicing, Has anyone else hit the same panic?

If you can pin point the exact location where the illegal access is 
made, please do so and I will accept that I am wrong, other than that I 
am not interested in this constant back and forth with no technical 
details just fluff.

Shoaib

> 
>> Please note I am not pushing that my patch be
>> accepted, I have done what I am suppose to do, it is upto the
>> maintainers to decide what is best for the code.