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

[Johan Hovold <jhovold-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Fri, Mar 09, 2012 at 04:02:21PM +0100, David Herrmann wrote:
> On Fri, Mar 9, 2012 at 3:40 PM, Johan Hovold <jhovold-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

Agreed.

> Or simply link to the patches that remove the destruct callback in the
> -next tree.

Yes, I would definitely mention those patches.

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

Ouch. I should have realised it was a typo, sorry.

Thanks,
Johan