Re: [PATCH v3] Bluetooth: hci_conn: Fix null ptr deref in hci_abort_conn()

["Siwei Zhang" <oss@fourdim.xyz>]

Hi Luiz,

On Thu, Jun 11, 2026, at 11:36 AM, Luiz Augusto von Dentz wrote:
> Hi Siwei,
>
> On Thu, Jun 11, 2026 at 11:20 AM Siwei Zhang <oss@fourdim.xyz> wrote:
>>
>> hci_abort_conn() read hci_skb_event(hdev->sent_cmd) when a connection
>> was pending, but hdev->sent_cmd can be NULL while req_status is still
>> HCI_REQ_PEND, leading to a NULL pointer dereference and a general
>> protection fault from the hci_rx_work() receive path.
>>
>> Instead of inspecting hdev->sent_cmd, track the in-flight create
>> connection command with a new per-connection HCI_CONN_CREATE flag and
>> route all cancellation through hci_cancel_connect_sync(), which dequeues
>> the command if still queued, or cancels the pending request if this
>> connection owns the in-flight create command. CIS uses the same path
>> via the existing HCI_CONN_CREATE_CIS flag.
>>
>> Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude:claude-opus-4-8
>> Signed-off-by: Siwei Zhang <oss@fourdim.xyz>
>> ---
>>  include/net/bluetooth/hci_core.h |  1 +
>>  net/bluetooth/hci_conn.c         | 21 ++--------
>>  net/bluetooth/hci_sync.c         | 66 ++++++++++++++++++++++++++------
>>  3 files changed, 58 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index aa600fbf9a53..aa554c34f9ec 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -988,6 +988,7 @@ enum {
>>         HCI_CONN_AUTH_FAILURE,
>>         HCI_CONN_PER_ADV,
>>         HCI_CONN_BIG_CREATED,
>> +       HCI_CONN_CREATE,
>>         HCI_CONN_CREATE_CIS,
>>         HCI_CONN_CREATE_BIG_SYNC,
>>         HCI_CONN_BIG_SYNC,
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 54eabaa46960..eba4a548bef5 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -3181,26 +3181,11 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
>>
>>         conn->abort_reason = reason;
>>
>> -       /* If the connection is pending check the command opcode since that
>> -        * might be blocking on hci_cmd_sync_work while waiting its respective
>> -        * event so we need to hci_cmd_sync_cancel to cancel it.
>> -        *
>> -        * hci_connect_le serializes the connection attempts so only one
>> -        * connection can be in BT_CONNECT at time.
>> +       /* Cancel the connect attempt. A return of 0 means the create command
>> +        * was still queued and got dequeued, so there is nothing to disconnect.
>>          */
>> -       if (conn->state == BT_CONNECT && READ_ONCE(hdev->req_status) == HCI_REQ_PEND) {
>> -               switch (hci_skb_event(hdev->sent_cmd)) {
>> -               case HCI_EV_CONN_COMPLETE:
>> -               case HCI_EV_LE_CONN_COMPLETE:
>> -               case HCI_EV_LE_ENHANCED_CONN_COMPLETE:
>> -               case HCI_EVT_LE_CIS_ESTABLISHED:
>> -                       hci_cmd_sync_cancel(hdev, ECANCELED);
>> -                       break;
>> -               }
>> -       /* Cancel connect attempt if still queued/pending */
>> -       } else if (!hci_cancel_connect_sync(hdev, conn)) {
>> +       if (!hci_cancel_connect_sync(hdev, conn))
>>                 return 0;
>> -       }
>>
>>         /* Run immediately if on cmd_sync_work since this may be called
>>          * as a result to MGMT_OP_DISCONNECT/MGMT_OP_UNPAIR which does
>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>> index df23245d6ccd..1fe19ddbbc2c 100644
>> --- a/net/bluetooth/hci_sync.c
>> +++ b/net/bluetooth/hci_sync.c
>> @@ -6668,6 +6668,12 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>>                                              &own_addr_type);
>>         if (err)
>>                 goto done;
>> +
>> +       /* Mark create connection in flight so hci_cancel_connect_sync() can
>> +        * cancel it while blocking on the connection complete event.
>> +        */
>> +       set_bit(HCI_CONN_CREATE, &conn->flags);
>> +
>>         /* Send command LE Extended Create Connection if supported */
>>         if (use_ext_conn(hdev)) {
>>                 err = hci_le_ext_create_conn_sync(hdev, conn, own_addr_type);
>> @@ -6703,6 +6709,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>>                                        conn->conn_timeout, NULL);
>>
>>  done:
>> +       clear_bit(HCI_CONN_CREATE, &conn->flags);
>> +
>>         if (err == -ETIMEDOUT)
>>                 hci_le_connect_cancel_sync(hdev, conn, 0x00);
>>
>> @@ -6982,10 +6990,19 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
>>         else
>>                 cp.role_switch = 0x00;
>>
>> -       return __hci_cmd_sync_status_sk(hdev, HCI_OP_CREATE_CONN,
>> -                                       sizeof(cp), &cp,
>> -                                       HCI_EV_CONN_COMPLETE,
>> -                                       conn->conn_timeout, NULL);
>> +       /* Mark create connection in flight so hci_cancel_connect_sync() can
>> +        * cancel it while blocking on the connection complete event.
>> +        */
>> +       set_bit(HCI_CONN_CREATE, &conn->flags);
>> +
>> +       err = __hci_cmd_sync_status_sk(hdev, HCI_OP_CREATE_CONN,
>> +                                      sizeof(cp), &cp,
>> +                                      HCI_EV_CONN_COMPLETE,
>> +                                      conn->conn_timeout, NULL);
>> +
>> +       clear_bit(HCI_CONN_CREATE, &conn->flags);
>> +
>> +       return err;
>>  }
>>
>>  int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
>> @@ -7039,20 +7056,45 @@ int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
>>
>>  int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
>>  {
>> -       if (conn->state != BT_OPEN)
>> -               return -EINVAL;
>> +       hci_cmd_sync_work_func_t func = NULL;
>> +       hci_cmd_sync_work_destroy_t destroy = NULL;
>> +       int create_flag = -1;
>>
>>         switch (conn->type) {
>>         case ACL_LINK:
>> -               return !hci_cmd_sync_dequeue_once(hdev,
>> -                                                 hci_acl_create_conn_sync,
>> -                                                 conn, NULL);
>> +               func = hci_acl_create_conn_sync;
>> +               create_flag = HCI_CONN_CREATE;
>> +               break;
>>         case LE_LINK:
>> -               return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync,
>> -                                                 conn, create_le_conn_complete);
>> +               func = hci_le_create_conn_sync;
>> +               destroy = create_le_conn_complete;
>> +               create_flag = HCI_CONN_CREATE;
>> +               break;
>> +       case CIS_LINK:
>> +               /* LE Create CIS is shared by the whole CIG and cannot be
>> +                * dequeued per-connection; only cancel it in-flight below.
>> +                */
>> +               create_flag = HCI_CONN_CREATE_CIS;
>> +               break;
>> +       default:
>> +               return -ENOENT;
>>         }
>>
>> -       return -ENOENT;
>> +       /* The create command is in exactly one of two states: still queued, or
>> +        * in flight. The create flag is only set once the worker has dequeued
>> +        * the entry and started running it, so the two states are mutually
>> +        * exclusive. Try to dequeue first: if it succeeds the command had not
>> +        * started yet and we are done. Otherwise the worker already pulled it
>> +        * off the queue (the dequeue is a no-op here),
>
> This is where I disagree, `dequeue` is not a no-op. It must traverse
> the list of entries in cmd_sync_work_list, so it is cheaper to check
> the flag first and then attempt to dequeue.
>

If we test the flag first and read 0 while the entry is still queued, the worker
can advance it to in-flight before our dequeue runs.
In that case we will return without cancelling.

I can update the comment saying that it has no effect instead of no-op.

>> +        * owns the in-flight create command, cancel the pending request.
>> +        */
>> +       if (func && hci_cmd_sync_dequeue_once(hdev, func, conn, destroy))
>> +               return 0;
>> +
>> +       if (create_flag >= 0 && test_bit(create_flag, &conn->flags))
>> +               hci_cmd_sync_cancel(hdev, ECANCELED);
>> +
>> +       return -EBUSY;
>>  }
>>
>>  int hci_le_conn_update_sync(struct hci_dev *hdev, struct hci_conn *conn,
>> --
>> 2.54.0
>>
>
>
> -- 
> Luiz Augusto von Dentz

Best,
Siwei