Re: [PATCH] Bluetooth: hci_h4: Fix race during initialization
From: Jonathan Rissanen
Date: Tue Mar 24 2026 - 06:06:15 EST
Hello Luiz,
On 3/20/26 21:04, Luiz Augusto von Dentz wrote:
[You don't often get email from luiz.dentz@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Hi Jonathan,
On Fri, Mar 20, 2026 at 8:10 AM Jonathan Rissanen
<jonathan.rissanen@xxxxxxxx> wrote:
Commit 5df5dafc171b ("Bluetooth: hci_uart: Fix another race during
initialization") fixed a race for hci commands sent during initialization.
However, there is still a race that happens if an hci event from one of
these commands is received before HCI_UART_REGISTERED has been set at
the end of hci_uart_register_dev(). The event will be ignored which
causes the command to fail with a timeout in the log:
"Bluetooth: hci0: command 0x1003 tx timeout"
This is because the hci event receive path (hci_uart_tty_receive ->
h4_recv) requires HCI_UART_REGISTERED to be set in h4_recv(), while the
hci command transmit path (hci_uart_send_frame -> h4_enqueue) only
requires HCI_UART_PROTO_INIT to be set in hci_uart_send_frame().
The check for HCI_UART_REGISTERED was originally added in commit
c2578202919a ("Bluetooth: Fix H4 crash from incoming UART packets")
to fix a crash caused by hu->hdev being null dereferenced. That can no
longer happen: once HCI_UART_PROTO_INIT is set in hci_uart_register_dev()
all pointers (hu, hu->priv and hu->hdev) are valid, and
hci_uart_tty_receive() already calls h4_recv() on HCI_UART_PROTO_INIT
or HCI_UART_PROTO_READY.
Remove the check for HCI_UART_REGISTERED in h4_recv() to fix the race
condition.
Signed-off-by: Jonathan Rissanen <jonathan.rissanen@xxxxxxxx>
---
drivers/bluetooth/hci_h4.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index ec017df8572c..1e9e2cad9ddf 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -109,9 +109,6 @@ static int h4_recv(struct hci_uart *hu, const void *data, int count)
{
struct h4_struct *h4 = hu->priv;
- if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
- return -EUNATCH;
-
h4->rx_skb = h4_recv_buf(hu, h4->rx_skb, data, count,
h4_recv_pkts, ARRAY_SIZE(h4_recv_pkts));
if (IS_ERR(h4->rx_skb)) {
---
There is some interesting comments on:
https://sashiko.dev/#/patchset/20260320-hci-init-fix-v1-1-e1960a41baf2%40axis.com
I see. Yeah there seem to be some valid comments:
If hci_register_dev() fails, the error path calls hu->proto->close(hu)
which frees the protocol-private data in hu->priv and sets it to NULL.
However, the error path fails to clear the HCI_UART_PROTO_INIT flag.
I think regardless of this patch it makes sense to clear HCI_UART_PROTO_INIT if hci_register_dev() fails, since we're no longer in the initializing state. It becomes more important with this patch since it will lead to a null pointer dereference if h4_recv() is called after hci_register_dev() fails.
Additionally, since hci_uart_register_dev() is called without holding the
write-side of hu->proto_lock, can concurrent incoming UART data during
the failure window trigger a use-after-free while hu->priv is actively
being freed by h4_close()?
I believe adding a write lock and clearing the bit would solve these issues:
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 2b28515de92c..5455990ab211 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -692,6 +692,9 @@ static int hci_uart_register_dev(struct hci_uart *hu)
if (hci_register_dev(hdev) < 0) {
BT_ERR("Can't register HCI device");
+ percpu_down_write(&hu->proto_lock);
+ clear_bit(HCI_UART_PROTO_INIT, &hu->flags);
+ percpu_up_write(&hu->proto_lock);
hu->proto->close(hu);
hu->hdev = NULL;
hci_free_dev(hdev);
It seems the issues pointed out there are unrelated to this change,
but I guess it is worth double checking just in case.
I think it's best to fix it before applying this change as it can cause null pointer dereference in error path. I can send a new patchset with the fix.
--
Best regards, Jonathan