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@xxxxxx> 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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxx/ > > > > > > > > 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
