Hi David,
On Fri, Mar 09, 2012 at 02:44:30PM +0100, David Herrmann wrote:
> On Wed, Mar 7, 2012 at 5:01 PM, Johan Hovold <jhovold@xxxxxxxxx> wrote:
> > Do not close protocol driver until device has been unregistered.
> >
> > This fixes a race between tty_close and hci_dev_open which can result in
> > a NULL-pointer dereference.
> >
> > The line discipline closes the protocol driver while we may still have
> > hci_dev_open sleeping on the req_lock mutex resulting in a NULL-pointer
> > dereference when lock is acquired and hci_init_req called.
[...]
> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> > index 0711448..6946081 100644
> > --- a/drivers/bluetooth/hci_ldisc.c
> > +++ b/drivers/bluetooth/hci_ldisc.c
> > @@ -310,11 +310,11 @@ static void hci_uart_tty_close(struct tty_struct *tty)
> > hci_uart_close(hdev);
> >
> > if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> > - hu->proto->close(hu);
> > if (hdev) {
> > hci_unregister_dev(hdev);
> > hci_free_dev(hdev);
> > }
> > + hu->proto->close(hu);
> > }
> > }
> > }
>
> I can confirm this. hci_uart_set_proto() opens the proto before it
> registers the hci device. Hence, we should also unregister the hci
> device before closing the proto. I also looked whether this introduces
> other race conditions but no proto-callback can be called here as they
> are all protected by the tty-layer which synchronizes all
> tty-callbacks. Therefore, I think this is the correct fix.
>
> We can apply this to stable even without the "destruct"-fixes from me
> as hu->proto->$cb$() doesn't care whether hdev is valid or not. I
> don't think the destruct-fixes are important enough to send them to
> stable.
Unfortunately hu is is not valid once hci_unregister returns as it will
call the destruct callback. So my patch depends on changing this
behaviour first. (I could also store a pointer to the protocol before
calling unregister in my patch.)
Secondly, I must disagree with you regarding whether the memory leak you
found is critical enough to be added to the stable trees. We're leaking
kernel memory in a deterministic and easily triggered way which could be
exploited by a malicious user.
> Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxxxxxxx>
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
