Re: [PATCH 1/2] bluetooth: hci_ldisc: fix NULL-pointer dereference on tty_close

[David Herrmann]

On Fri, Mar 9, 2012 at 3:40 PM, Johan Hovold <jhovold@xxxxxxxxx> wrote:
> On Fri, Mar 09, 2012 at 02:52:00PM +0100, David Herrmann wrote:
>> On Fri, Mar 9, 2012 at 2:04 PM, Johan Hovold <jhovold@xxxxxxxxx> wrote:
>> > On Thu, Mar 08, 2012 at 09:45:22AM -0800, Marcel Holtmann 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.
>> >> >
>> >> > [...]
>> >> >
>> >> > > what kernel version is this against? Our changes in bluetooth-next fixed
>> >> > > some of the destruct handling.
>> >> >
>> >> > This is against the latest rc as it needs to be fixed in 3.3, but I
>> >> > missed a dependency to bluetooth-next as you point out below.
>> >> >
>> >> > > Also hci_unregister_dev should be calling the destruct handler and thus
>> >> > > your change is now accessing hu but it got freed already.
>> >> >
>> >> > You're right, my patch depends on 010666a126fc ("Bluetooth: Make
>> >> > hci-destruct callback optional") and 797fe796c4 ("Bluetooth: uart-ldisc:
>> >> > Fix memory leak and remove destruct cb") from bluetooth-next.
>> >> >
>> >> > But since the latter one fixes a memory leak it should have been marked
>> >> > for stable as well as pushed to Linus for 3.3, right?
>> >>
>> >> we need to look into this and propose patches for -stable. Is your
>> >> problem still present with bluetooth-next or not?
>> >
>> > Yes, both races are present in bluetooth-next of today (b8622cbd58f34)
>> > and only takes an additional manual step to trigger (as the core no
>> > longer tries to open the device twice automatically).
>> >
>> > My two patches on top of either the two patches by David Herrmann
>> > mentioned above or the following minimal fix of the same memory leak
>> > would be sufficient to fix both races in 3.3:
>> >
>> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> > index 0711448..97c5faa 100644
>> > --- a/drivers/bluetooth/hci_ldisc.c
>> > +++ b/drivers/bluetooth/hci_ldisc.c
>> > @@ -237,7 +237,6 @@ static void hci_uart_destruct(struct hci_dev *hdev)
>> >                return;
>> >
>> >        BT_DBG("%s", hdev->name);
>> > -       kfree(hdev->driver_data);
>> >  }
>> >
>> >  /* ------ LDISC part ------ */
>> > @@ -316,6 +315,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
>> >                                hci_free_dev(hdev);
>> >                        }
>> >                }
>> > +               kfree(hu);
>> >        }
>> >  }
>>
>> The "destruct"-callback was broken in many ways but working around it
>> without removing it seems wrong.
>
> The reason for not doing so would be to keep the fixes minimal and thus
> more appropriate for the stable trees.
>
> Furthermore, according to you patch own description "Several drivers
> already provide an empty callback" so I didn't consider it to be
> a problem.

It's just a proposal, feel free to keep your patch. But please include
a comment in your commit-message that you explicitly avoid using the
destruct-callback as it is, and always was, broken. Otherwise, it looks
wrong seeing such a commit.
Or simply link to the patches that remove the destruct callback in the
-next tree.

>> This memory-leak occurs only if a
>> tty-device uses the uart-ldisc without a protocol bound to it.
>> Therefore, I didn't consider it important enough for stable.
>
> See my answer to you previous mail regarding this.
>
>> However,
>> if you want to fix this, leave the kfree() inside the destruct
>> callback but add another kfree() into the hci_uart_close() in an
>> "else"-clause like this:
>>
>> if (test_and_clear_bit(...)) {
>> } else {
>> +   kfree(...);
>> }
>
> You really don't want to free the hci_uart in it's own close method...
>
> The hci_uart is allocated in tty_open and should be freed in tty_close.

Oops, I obviously meant hci_uart_tty_close(), sorry.

>> This will still keep the bogus ref-counts inside hci_dev with the
>> destruct() callback but will also free the ldisc if no protocol is
>> set.
>
> Thanks,
> Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/