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

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Mingyu,


Thank you for the patch, and your work on the Linux kernel.

Am 13.05.26 um 08:45 schrieb w15303746062@163.com:
> 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.

Please elaborate, in what setup it was observed, and please add an 
excerpt of the trace.

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

Please add a Fixes: tag, so it gets backported.

Also, please add a Link: tag with a URL to the test case, or include it 
in the commit message.

> 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);
> +
>   	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);


Kind regards,

Paul


PS: If you resend, and don’t know yet (you have commits in the Linux 
kernel already), please add v2 to the tag. (`git format-patch -2 …` or 
an equivalent option to your tooling.

PPS: sashiko.dev did not pick this patch up yet [1].


[1]: https://sashiko.dev/#/?list=org.kernel.vger.linux-bluetooth