Re:Re: [PATCH] Bluetooth: hci_uart: fix UAF in hci_uart_tty_close()

[w15303746062 <w15303746062@163.com>]

Hi Luiz,

Thank you for the review.

That is an excellent suggestion. You are absolutely right. Since the
`hu` structure is being torn down and freed immediately afterward, 
using `disable_work_sync()` provides a much stronger guarantee by 
preventing any concurrent threads from re-queuing the works, thus 
eliminating the risk of a lingering UAF.

Both `init_ready` and `write_work` are standard `struct work_struct`,
so `disable_work_sync()` applies perfectly here.

I will send out a v4 patch shortly adopting this change. 
Thank you for pointing this out!

Best regards,
Mingyu


At 2026-05-15 20:37:57, "Luiz Augusto von Dentz" <luiz.dentz@gmail.com> wrote:
>Hi,
>
>On Wed, May 13, 2026 at 2:46 AM <w15303746062@163.com> wrote:
>>
>> From: Mingyu Wang <25181214217@stu.xidian.edu.cn>
>>
>> A Use-After-Free (UAF) vulnerability and a subsequent General Protection
>> Fault (GPF) were observed in h5_recv() due to a race condition between
>> the initialization of the HCI UART line discipline and concurrent TTY
>> hangup via TIOCVHANGUP.
>>
>> The issue arises because the workqueues (init_ready and write_work) are
>> only cancelled if the HCI_UART_PROTO_READY flag is set. However, during
>> the protocol initialization phase (HCI_UART_PROTO_INIT), the underlying
>> protocol (e.g., H5) may schedule work (such as sending sync/config
>> packets). If a hangup occurs before the setup completes and the READY
>> flag is set, hci_uart_tty_close() skips the cancel_work_sync() calls
>> and proceeds to free the `hu` struct.
>>
>> When the delayed workqueue finally executes, it blindly dereferences
>> the freed `hu` struct, causing ODEBUG warnings and kernel panics.
>>
>> Fix this by moving the cancel_work_sync() calls outside the
>> HCI_UART_PROTO_READY check, ensuring that any pending works are
>> unconditionally cancelled before the hci_uart structure is freed.
>>
>> Signed-off-by: Mingyu Wang <25181214217@stu.xidian.edu.cn>
>> ---
>>  drivers/bluetooth/hci_ldisc.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index 275ea865bc29..566e1c525ee2 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -544,14 +544,18 @@ static void hci_uart_tty_close(struct tty_struct *tty)
>>         if (hdev)
>>                 hci_uart_close(hdev);
>>
>> +       /*
>> +        * Always cancel workqueues unconditionally before freeing the hu
>> +        * struct, as they might be active during the PROTO_INIT phase.
>> +        */
>> +       cancel_work_sync(&hu->init_ready);
>> +       cancel_work_sync(&hu->write_work);
>
>Can't we use disable_work_sync? If it frees up at the end, it's
>probably best to disable it so it doesn't allow new submissions.
>
>>         if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
>>                 percpu_down_write(&hu->proto_lock);
>>                 clear_bit(HCI_UART_PROTO_READY, &hu->flags);
>>                 percpu_up_write(&hu->proto_lock);
>>
>> -               cancel_work_sync(&hu->init_ready);
>> -               cancel_work_sync(&hu->write_work);
>> -
>>                 if (hdev) {
>>                         if (test_bit(HCI_UART_REGISTERED, &hu->flags))
>>                                 hci_unregister_dev(hdev);
>> --
>> 2.34.1
>>
>>
>
>
>-- 
>Luiz Augusto von Dentz