linux-kernel - Re: Is commit 4d94f0555827 safe?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <877c55ci1r.wl-tiwai@suse.de>
Date: Tue, 04 Mar 2025 09:32:32 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc: Pauli Virtanen <pav@....fi>,
	Takashi Iwai <tiwai@...e.de>,
	Luiz Augusto von Dentz <luiz.von.dentz@...el.com>,
	linux-bluetooth@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: Is commit 4d94f0555827 safe?

On Mon, 03 Mar 2025 19:19:47 +0100,
Luiz Augusto von Dentz wrote:
> 
> Hi Pauli, Takashi,
> 
> On Mon, Mar 3, 2025 at 12:47 PM Pauli Virtanen <pav@....fi> wrote:
> >
> > Hi,
> >
> > ma, 2025-03-03 kello 17:38 +0100, Takashi Iwai kirjoitti:
> > > On Mon, 03 Mar 2025 17:29:58 +0100,
> > > Luiz Augusto von Dentz wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Mon, Mar 3, 2025 at 10:56 AM Takashi Iwai <tiwai@...e.de> wrote:
> > > > >
> > > > > On Mon, 03 Mar 2025 16:50:37 +0100,
> > > > > Luiz Augusto von Dentz wrote:
> > > > > >
> > > > > > Hi Takashi,
> > > > > >
> > > > > > On Mon, Mar 3, 2025 at 10:10 AM Takashi Iwai <tiwai@...e.de> wrote:
> > > > > > >
> > > > > > > On Mon, 03 Mar 2025 15:57:16 +0100,
> > > > > > > Luiz Augusto von Dentz wrote:
> > > > > > > >
> > > > > > > > Hi Takashi,
> > > > > > > >
> > > > > > > > Well the assumption was that because we are doing a copy of the struct
> > > > > > > > being unregistered/freed would never cause any errors, so to trigger
> > > > > > > > something like UAF like the comment was suggesting the function
> > > > > > > > callback would need to be unmapped so even if the likes of iso_exit is
> > > > > > > > called it function (e.g. iso_connect_cfm) remains in memory.
> > > > > > >
> > > > > > > But it doesn't guarantee that the callback function would really
> > > > > > > work.  e.g. if the callback accesses some memory that was immediately
> > > > > > > freed after the unregister call, it will lead to a UAF, even though
> > > > > > > the function itself is still present on the memory.
> > > > > > >
> > > > > > > That said, the current situation makes hard to judge the object life
> > > > > > > time.
> > > > > > >
> > > > > > > > You can find the previous version here:
> > > > > > > >
> > > > > > > > https://syzkaller.appspot.com/text?tag=Patch&x=100c0de8580000
> > > > > > > >
> > > > > > > > Problem with it was that it is invalid to unlock and relock like that.
> > > > > > >
> > > > > > > Thanks for the pointer!
> > > > > > >
> > > > > > >
> > > > > > > BTW, I saw another patch posted to replace the mutex with spinlock
> > > > > > > (and you replied later on that it's been already fixed).
> > > > > > > Is it an acceptable approach at all?
> > > > > >
> > > > > > I don't remember if I saw that, but yeah anything that makes the issue
> > > > > > go away, and doesn't create new problems, would probably be
> > > > > > acceptable.
> > > > >
> > > > > I saw this one:
> > > > >   https://lore.kernel.org/all/20230907122234.146449-1-william.xuanziyang@huawei.com/
> > > >
> > > > Ive might have missed it, we will probably need to rebase it but other
> > > > than that it should be acceptable.
> > >
> > > Does it mean that you'd revert the change and apply the above one
> > > (with rebase or modification)?  Or would you keep a part of the
> > > current change (e.g. match callback looks neat) while applying the
> > > similar fix using the spinlock?
> >
> > My current understanding of this is that the actual problem for
> > 4d94f0555827 was incorrect RCU use at the callsite in
> > hci_le_create_big_complete_evt(). That part was rewritten in
> >
> > commit 581dd2dc168f ("Bluetooth: hci_event: Fix using rcu_read_(un)lock
> > while iterating")
> 
> In that case maybe we can just revert the 4d94f0555827 ("Bluetooth:
> hci_core: Fix sleeping
> function called from invalid context") and see if that works, might
> need to trigger syzbot just to confirm we don't introduce the original
> problem.

Fair enough, it sounds reasonable.


thanks,

Takashi