Messages in this thread | | | Date | Fri, 9 Mar 2012 15:29:39 +0100 | From | Johan Hovold <> | Subject | Re: [PATCH 1/2] bluetooth: hci_ldisc: fix NULL-pointer dereference on tty_close |
| |
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@gmail.com> 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@googlemail.com>
Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |