Re: Is commit 4d94f0555827 safe?

[Takashi Iwai <tiwai@suse.de>]

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?


Takashi

> 
> On Sun, Mar 2, 2025 at 4:59 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > [ I posted without the subject line mistakenly, so resent again;
> >   sorry if you have seen already read it ]
> >
> > Hi Luiz,
> >
> > due to the CVE assignment, I stumbled on the recent fix for BT
> > hci_core, the commit 4d94f0555827 ("Bluetooth: hci_core: Fix sleeping
> > function called from invalid context"), and wonder whether it's really
> > safe.
> >
> > As already asked question at the patch review:
> >   https://patchwork.kernel.org/comment/26147087/
> > the code allows the callbacks to be called even after
> > hci_unregister_cb() returns.
> >
> > Your assumption was that it's never called without the module removal,
> > but isn't hci_unregister_cb() also called from iso_exit() which can be
> > triggered via set_iso_socket_func() in mgmt.c?  Also, any 3rd party
> > module could call hci_unregister_cb() in a wild way, too -- even if
> > the function still remains, it doesn't mean that you can call it
> > safely if the caller already assumes it being unregistered.
> >
> > In addition to that, I feel what the patch does as a bit too
> > heavy-lifting: it does kmalloc() and copy the whole hci_cb object,
> > which isn't quite small for each.  If the callback is still safe to
> > call after RCU protection, you may just keep the hci_cb pointer
> > instead of copying the whole content, too?
> >
> > I couldn't find v1 patch in the patchwork, so not sure whether this
> > has been already discussed.  If so, let me know.
> >
> >
> > Thanks!
> >
> > Takashi
> >
> 
> 
> -- 
> Luiz Augusto von Dentz