Re: [PATCH 2/2 v2] bluetooth: hci_core: fix NULL-pointer dereference at unregister

[Johan Hovold <jhovold-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi David,

On Fri, Mar 09, 2012 at 03:04:11PM +0100, David Herrmann wrote:
> On Fri, Mar 9, 2012 at 1:53 PM, Johan Hovold <jhovold-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Make sure hci_dev_open returns immediately if hci_dev_unregister has
> > been called.
> >
> > This fixes a race between hci_dev_open and hci_dev_unregister which can
> > lead to a NULL-pointer dereference.
> >
> > Bug is 100% reproducible using hciattach and a disconnected serial port:
> >
> > 0. # hciattach -n /dev/ttyO1 any noflow
> >
> > 1. hci_dev_open called from hci_power_on grabs req lock
> > 2. hci_init_req executes but device fails to initialise (times out
> >   eventually)
> > 3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
> > 4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
> >   hci_dev_do_close
> > 5. hci_dev_open (1) releases req lock
> > 6. hci_dev_do_close grabs req lock and returns as device is not up
> > 7. hci_dev_unregister sleeps in destroy_workqueue
> > 8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
> > 9. hci_dev_unregister finishes, while hci_dev_open is still running...

[...]

> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 00596e8..e8879b9 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -93,6 +93,8 @@ enum {
> >  * states from the controller.
> >  */
> >  enum {
> > +       HCI_UNREGISTER,
> > +
> >        HCI_LE_SCAN,
> >  };
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index d6448f0..22b6781 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)
> >
> >        hci_req_lock(hdev);
> >
> > +       if (test_bit(HCI_UNREGISTER, &hdev->dev_flags)) {
> > +               ret = -ENODEV;
> > +               goto done;
> > +       }
> > +
> 
> Isn't it enough to check for HCI_RUNNING here? We obviously have a
> race here as we take the device with hci_dev_get(), then sleep and
> then we do not check whether the device is still alive. However,
> drivers are required to reset HCI_RUNNING before calling
> hci_unregister_dev() (which is bogus anyway, but its the way we
> handled it in the past) therefore it should be enough for us to check
> for HCI_RUNNING.

I'm afraid this won't work as hci_dev_open is responsible for setting
HCI_RUNNING in the first place (set in hdev->open(hdev) called from
hci_dev_open).

Thanks,
Johan