On Fri, Mar 09, 2012 at 03:35:46PM +0100, David Herrmann wrote:

On Fri, Mar 9, 2012 at 3:29 PM, Johan Hovold [off-list ref] wrote:

quoted

On Fri, Mar 09, 2012 at 02:44:30PM +0100, David Herrmann wrote:

quoted

On Wed, Mar 7, 2012 at 5:01 PM, Johan Hovold [off-list ref] wrote:

quoted

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.

[...]

quoted

quoted

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.)

Right, I missed that, sorry.

quoted

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.

Are you planning on sending a patch to stable-ML or should I do so? How about
my proposal in the other mail? Could you include this fix when resending this?

This needs to go in through the bluetooth/networking trees (or their
maintainers at least) so that it gets in to 3.3, otherwise stable will
not pick it up for earlier trees.

I'll post a revised series which includes the minimal fix to the memory
leak so that all three patches can go to Linus and hopefully make it in
before 3.3 is out. 

Sounds good?

Thanks,
Johan