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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANq1E4TzcQCuKomPv1bNTdkJ7LDTYjKiMqUwV8ZaoYG6FJO9Qw@mail.gmail.com>
Date:	Fri, 9 Mar 2012 15:35:46 +0100
From:	David Herrmann <dh.herrmann@...glemail.com>
To:	Johan Hovold <jhovold@...il.com>
Cc:	Marcel Holtmann <marcel@...tmann.org>,
	"Gustavo F. Padovan" <padovan@...fusion.mobi>,
	"David S. Miller" <davem@...emloft.net>,
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, stable <stable@...r.kernel.org>
Subject: Re: [PATCH 1/2] bluetooth: hci_ldisc: fix NULL-pointer dereference on tty_close

On Fri, Mar 9, 2012 at 3:29 PM, Johan Hovold <jhovold@...il.com> wrote:
> 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@...il.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.)

Right, I missed that, sorry.

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

>> Reviewed-by: David Herrmann <dh.herrmann@...glemail.com>
>
> Thanks,
> Johan

Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/