From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
Date: Mon, 16 Jun 2025 14:56:26 -0400
> Hi Pauli, Kuniyuki,
>
> On Mon, Jun 16, 2025 at 2:12 PM Pauli Virtanen <pav@xxxxxx> wrote:
> >
> > Hi,
> >
> > ma, 2025-06-16 kello 10:37 -0700, Kuniyuki Iwashima kirjoitti:
> > > From: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> > >
> > > syzbot reported use-after-free in vhci_flush() without repro. [0]
> > >
> > [clip]
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index 07a8b4281a39..d648b514e2df 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -64,9 +64,9 @@ static DEFINE_IDA(hci_index_ida);
> > >
> > > /* Get HCI device by index.
> > > * Device is held on return. */
> > > -struct hci_dev *hci_dev_get(int index)
> > > +static struct hci_dev *__hci_dev_get(int index, int *srcu_index)
> > > {
> > > - struct hci_dev *hdev = NULL, *d;
> > > + struct hci_dev *hdev = NULL;
> > >
> > > BT_DBG("%d", index);
> > >
> > > @@ -74,9 +74,11 @@ struct hci_dev *hci_dev_get(int index)
> > > return NULL;
> > >
> > > read_lock(&hci_dev_list_lock);
> > > - list_for_each_entry(d, &hci_dev_list, list) {
> > > - if (d->id == index) {
> > > - hdev = hci_dev_hold(d);
> > > + list_for_each_entry(hdev, &hci_dev_list, list) {
> > > + if (hdev->id == index) {
> > > + hci_dev_hold(hdev);
> > > + if (srcu_index)
> > > + *srcu_index = srcu_read_lock(&hdev->srcu);
> > > break;
> > > }
> > > }
> > > @@ -84,6 +86,22 @@ struct hci_dev *hci_dev_get(int index)
> > > return hdev;
> > > }
> >
> > If no list item has `hdev->id == index`, doesn't this now return the
> > list head -> crash?
>
> Seems wrong to me as well, *d was used to hold the current entry but
> that has been removed so hdev would be used instead which may return a
> valid/non-NULL entry even when its index doesn't match. Btw, are there
> any documentation regarding the usage of SRCU in such cases where
> there are still references?
I think that's the main use case of SRCU, and basically the use case
is similar to the normal RCU. There are a bunch of doc under
Documentations/ and I'm not sure what's the best one, but LWN article
would be a nice one to start.
https://lwn.net/Articles/202847/
Also, one good example would be rtnl_link_ops_get(), rtnl_link_ops_put(),
and rtnl_link_unregister().
>
> Usually the hci_unregister_dev is called by the driver to inform the
> hardware has been unplugged from the system, so we do want to be able
> to abort any ongoing usage of the hci_dev so in this particular case
> perhaps it is easier to just check if HCI_UNREGISTER has been set
> before attempting to flush.
Maybe like below ?
I think the downside of this approach would be we need to apply this
change to each driver that has the same issue.
Note that we need to hold mutex here otherwise there is no guarantee
that another thread does not complete kfree(). I'm not sure if touching
unregister_dev in non-bluetooth-core code is something we should avoid.
What do you think ?
---8<---
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 59f4d7bdffdc..3dea1292da2d 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -66,7 +66,10 @@ static int vhci_flush(struct hci_dev *hdev)
{
struct vhci_data *data = hci_get_drvdata(hdev);
- skb_queue_purge(&data->readq);
+ mutex_lock(&hdev->unregister_lock);
+ if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
+ skb_queue_purge(&data->readq);
+ mutex_unlock(&hdev->unregister_lock);
return 0;
}
---8<---
