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

[Kuniyuki Iwashima]

From: Shoaib Rao <rao.shoaib@xxxxxxxxxx>
Date: Tue, 10 Sep 2024 11:16:59 -0700
> On 9/10/2024 10:57 AM, Kuniyuki Iwashima wrote:
> > From: Shoaib Rao <rao.shoaib@xxxxxxxxxx>
> > Date: Tue, 10 Sep 2024 09:55:03 -0700
> >> On 9/9/2024 5:48 PM, Kuniyuki Iwashima wrote:
> >>> From: Shoaib Rao <rao.shoaib@xxxxxxxxxx>
> >>> 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@xxxxxxxxxx/__;!!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.


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


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


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