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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120309144804.GF4497@localhost>
Date:	Fri, 9 Mar 2012 15:48:04 +0100
From:	Johan Hovold <jhovold@...il.com>
To:	David Herrmann <dh.herrmann@...glemail.com>
Cc:	Marcel Holtmann <marcel@...tmann.org>,
	"Gustavo F. Padovan" <padovan@...fusion.mobi>,
	"David S. Miller" <davem@...emloft.net>,
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, stable <stable@...r.kernel.org>
Subject: Re: [PATCH 2/2 v2] bluetooth: hci_core: fix NULL-pointer dereference
 at unregister

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@...il.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/