[PATCH v2] Bluetooth: hci_serdev: Fix use-after-free in hci_uart_unregister_device()
ZhaoJinming <[email protected]>
| Newsgroups | org.kernel.vger.linux-bluetooth,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <D747A53C08BD2456+20260810-bluetooth-hci-serdev-uart-unregister-v2-1-690493a6ab95@uniontech.com> |
hci_uart_unregister_device() frees the HCI device (hci_free_dev) before cancelling write_work via cancel_work_sync(). If write_work is executing concurrently on another CPU, it can access hu->hdev and write to hdev->stat after the memory has been freed. Additionally, HCI_UART_PROTO_READY is not cleared until after cancel_work_sync, so the write_wakeup serdev callback can still schedule write_work via hci_uart_tx_wakeup() even after hci_free_dev has freed the device. Fix this by mirroring the same ordering used in the tty/ldisc path (hci_uart_tty_close, hci_ldisc.c:565-593): 1. Save the PROTO_READY state and clear it under the write lock so a concurrent hci_uart_tx_wakeup() cannot re-schedule write_work 2. Cancel write_work (no new work can be scheduled and no work is in flight) 3. Unregister the HCI device 4. Close the protocol (may access hu->hdev and the serdev device) 5. Close the serdev port (safe now that write_work is quiesced and protocol is done) 6. Free the HCI device Also free any partially transmitted frame (hu->tx_skb) left over by write_work once the transmit path is quiesced, since hci_uart_close() would skip hci_uart_flush() because HCI_UART_PROTO_READY is cleared. Signed-off-by: ZhaoJinming <[email protected]> --- Fix a use-after-free in hci_uart_unregister_device() where the HCI device could be freed (hci_free_dev) before write_work was cancelled, allowing a concurrently running write_work to access hu->hdev after the memory had been freed. The teardown sequence is reordered to mirror the tty/ldisc path (hci_uart_tty_close) and to account for the serdev-specific teardown: the serdev port is closed only after write_work and the protocol are fully torn down, and any partially transmitted frame is freed once the transmit path is quiesced. Changes in v2: - Clear HCI_UART_PROTO_READY under percpu_down_write() to prevent a concurrent hci_uart_tx_wakeup() from re-scheduling write_work via the write_wakeup callback once the device is torn down. - Cancel write_work before closing the serdev device to avoid a use-after-free in the serdev/TTY backend. - Close the serdev device after the protocol close, since some protocol close handlers (e.g. qca_close) still access the serdev device. - Free any partially transmitted frame (hu->tx_skb) after write_work is quiesced. --- drivers/bluetooth/hci_serdev.c | 46 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c index 593d9cefbbf925b4d3f8d37a12420a99e08a9477..13346c205591056eaca6e4f0dae53db398064230 100644 --- a/drivers/bluetooth/hci_serdev.c +++ b/drivers/bluetooth/hci_serdev.c @@ -395,20 +395,54 @@ EXPORT_SYMBOL_GPL(hci_uart_register_device_priv); void hci_uart_unregister_device(struct hci_uart *hu) { struct hci_dev *hdev = hu->hdev; + bool proto_ready; + /* Wait for init_ready to finish to prevent registration races */ cancel_work_sync(&hu->init_ready); - if (test_bit(HCI_UART_REGISTERED, &hu->flags)) - hci_unregister_dev(hdev); - hci_free_dev(hdev); + proto_ready = test_bit(HCI_UART_PROTO_READY, &hu->flags); + if (proto_ready) { + /* Clear HCI_UART_PROTO_READY under the write lock so a + * concurrent hci_uart_tx_wakeup() cannot re-schedule + * write_work via the write_wakeup callback once the device + * is torn down. + */ + percpu_down_write(&hu->proto_lock); + clear_bit(HCI_UART_PROTO_READY, &hu->flags); + percpu_up_write(&hu->proto_lock); + } + + /* Unconditionally cancel write_work AFTER clearing PROTO_READY. + * This ensures that concurrent protocol timers cannot requeue + * write_work, permanently preventing double-free races and UAFs, + * and guarantees no write_work is in flight before the serdev + * device is closed. + */ cancel_work_sync(&hu->write_work); + /* Free any partially transmitted frame left over by write_work now + * that the transmit path is fully quiesced. hci_uart_close() would + * skip hci_uart_flush() because HCI_UART_PROTO_READY is cleared. + */ + if (hu->tx_skb) { + kfree_skb(hu->tx_skb); + hu->tx_skb = NULL; + } + + if (test_bit(HCI_UART_REGISTERED, &hu->flags)) + hci_unregister_dev(hdev); + + /* Close the protocol before freeing hdev (intrinsically purges queues). + * Some protocol close handlers (e.g. qca_close) may still access the + * serdev device, so keep the serdev port open until this completes. + */ hu->proto->close(hu); - if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) { - clear_bit(HCI_UART_PROTO_READY, &hu->flags); + if (proto_ready) serdev_device_close(hu->serdev); - } + + hci_free_dev(hdev); + percpu_free_rwsem(&hu->proto_lock); } EXPORT_SYMBOL_GPL(hci_uart_unregister_device); --- base-commit: db2ddb87143519e20a95aa36c60b36107b736a58 change-id: 20260810-bluetooth-hci-serdev-uart-unregister-ce37ebb29283 Best regards, -- ZhaoJinming <[email protected]>