Re: [syzbot] [net?] KASAN: slab-use-after-free Read in unix_stream_read_actor (2)
From: Shoaib Rao
Date: Tue Sep 10 2024 - 14:49:48 EST
On 9/10/2024 11:33 AM, Kuniyuki Iwashima wrote:
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.
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.