[PATCH v5 1/2] Bluetooth: hci_conn: Fix null ptr deref in hci_abort_conn()

[PATCH v5 1/2] Bluetooth: hci_conn: Fix null ptr deref in hci_abort_conn()

[Siwei Zhang]

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(). The create
command is in exactly one of two states: still queued, or in flight.
hci_cancel_connect_sync() holds cmd_sync_work_lock across the whole
decision: the worker takes this lock to dequeue every entry, so while it
is held a queued command cannot start running and an in-flight command
cannot complete and let the next command become pending. This keeps the
flag test and hci_cmd_sync_cancel() atomic with respect to the worker,
so a queued command is simply dequeued, and an in-flight command owned
by this connection is cancelled without the risk of cancelling an
unrelated command that became pending in the meantime. CIS uses the same
path via the existing HCI_CONN_CREATE_CIS flag.

hci_acl_create_conn_sync() and hci_le_create_conn_sync() clear
HCI_CONN_CREATE after the create command completes, but the command
status handler can free conn via hci_conn_del() (for example when the
controller rejects the connection) while the worker is still blocked on
the connection complete event. Hold a reference on conn across the
create command so the flag can be cleared without a use-after-free.

Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
Cc: stable@vger.kernel.org
Suggested-by: XIAO WU <xiaowu.417@qq.com>
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         | 96 ++++++++++++++++++++++++++++----
 3 files changed, 88 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..06b87e9cd999 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -6611,6 +6611,11 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
 
 	bt_dev_dbg(hdev, "conn %p", conn);
 
+	/* Hold a reference so conn stays valid for the HCI_CONN_CREATE
+	 * clear_bit() at done.
+	 */
+	hci_conn_get(conn);
+
 	clear_bit(HCI_CONN_SCANNING, &conn->flags);
 	conn->state = BT_CONNECT;
 
@@ -6623,6 +6628,7 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
 		    hdev->le_scan_type == LE_SCAN_ACTIVE &&
 		    !hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
 			hci_conn_del(conn);
+			hci_conn_put(conn);
 			return -EBUSY;
 		}
 
@@ -6668,6 +6674,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,11 +6715,14 @@ 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);
 
 	/* Re-enable advertising after the connection attempt is finished. */
 	hci_resume_advertising_sync(hdev);
+	hci_conn_put(conn);
 	return err;
 }
 
@@ -6982,10 +6997,25 @@ 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);
+	/* Hold a reference so conn stays valid for the HCI_CONN_CREATE
+	 * clear_bit() below.
+	 */
+	hci_conn_get(conn);
+
+	/* 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);
+	hci_conn_put(conn);
+
+	return err;
 }
 
 int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
@@ -7039,20 +7069,62 @@ 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;
+	struct hci_cmd_sync_work_entry *entry;
+	hci_cmd_sync_work_func_t func = NULL;
+	hci_cmd_sync_work_destroy_t destroy = NULL;
+	int create_flag = -1;
+	int err = -EBUSY;
 
 	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 either still queued or in flight. Hold
+	 * cmd_sync_work_lock across the test and the cancel: the worker takes
+	 * this lock to dequeue every entry, so while it is held no other command
+	 * can become pending, which keeps hci_cmd_sync_cancel() from racing with
+	 * completion and cancelling an unrelated command.
+	 */
+	mutex_lock(&hdev->cmd_sync_work_lock);
+
+	/* The flag is set while the worker blocks on the connection complete
+	 * event, so if it is set this connection owns the pending request.
+	 */
+	if (create_flag >= 0 && test_bit(create_flag, &conn->flags)) {
+		hci_cmd_sync_cancel(hdev, ECANCELED);
+		goto unlock;
+	}
+
+	/* Otherwise it may still be queued; dequeue it. A successful dequeue
+	 * means it never started, so there is nothing to disconnect.
+	 */
+	if (func) {
+		entry = _hci_cmd_sync_lookup_entry(hdev, func, conn, destroy);
+		if (entry) {
+			_hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
+			err = 0;
+		}
+	}
+
+unlock:
+	mutex_unlock(&hdev->cmd_sync_work_lock);
+	return err;
 }
 
 int hci_le_conn_update_sync(struct hci_dev *hdev, struct hci_conn *conn,
-- 
2.54.0

[PATCH v5 2/2] Bluetooth: hci_sync: Remove unused hci_cmd_sync_dequeue_once()

[Siwei Zhang]

hci_cmd_sync_dequeue_once() had a single in-tree caller,
hci_cancel_connect_sync(), which now holds cmd_sync_work_lock across the
in-flight create flag test and the dequeue and so open-codes the lookup
and cancel under that lock. That leaves the exported
hci_cmd_sync_dequeue_once() with no in-tree user, so remove it along with
its declaration.

Signed-off-by: Siwei Zhang <oss@fourdim.xyz>
---
 include/net/bluetooth/hci_sync.h |  3 ---
 net/bluetooth/hci_sync.c         | 26 --------------------------
 2 files changed, 29 deletions(-)

diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index 73e494b2591d..818e62d9fe9e 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -84,9 +84,6 @@ void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
 			       struct hci_cmd_sync_work_entry *entry);
 bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 			  void *data, hci_cmd_sync_work_destroy_t destroy);
-bool hci_cmd_sync_dequeue_once(struct hci_dev *hdev,
-			      hci_cmd_sync_work_func_t func, void *data,
-			      hci_cmd_sync_work_destroy_t destroy);
 
 int hci_update_eir_sync(struct hci_dev *hdev);
 int hci_update_class_sync(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 06b87e9cd999..7410c640d4ff 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -860,32 +860,6 @@ void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
 }
 EXPORT_SYMBOL(hci_cmd_sync_cancel_entry);
 
-/* Dequeue one HCI command entry:
- *
- * - Lookup and cancel first entry that matches.
- */
-bool hci_cmd_sync_dequeue_once(struct hci_dev *hdev,
-			       hci_cmd_sync_work_func_t func,
-			       void *data, hci_cmd_sync_work_destroy_t destroy)
-{
-	struct hci_cmd_sync_work_entry *entry;
-
-	mutex_lock(&hdev->cmd_sync_work_lock);
-
-	entry = _hci_cmd_sync_lookup_entry(hdev, func, data, destroy);
-	if (!entry) {
-		mutex_unlock(&hdev->cmd_sync_work_lock);
-		return false;
-	}
-
-	_hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
-
-	mutex_unlock(&hdev->cmd_sync_work_lock);
-
-	return true;
-}
-EXPORT_SYMBOL(hci_cmd_sync_dequeue_once);
-
 /* Dequeue HCI command entry:
  *
  * - Lookup and cancel any entry that matches by function callback or data or
-- 
2.54.0

Re: [PATCH v5 1/2] Bluetooth: hci_conn: Fix null ptr deref in hci_abort_conn()

[Luiz Augusto von Dentz]

Hi Siwei,

On Mon, Jun 15, 2026 at 9:12 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(). The create
> command is in exactly one of two states: still queued, or in flight.
> hci_cancel_connect_sync() holds cmd_sync_work_lock across the whole
> decision: the worker takes this lock to dequeue every entry, so while it
> is held a queued command cannot start running and an in-flight command
> cannot complete and let the next command become pending. This keeps the
> flag test and hci_cmd_sync_cancel() atomic with respect to the worker,
> so a queued command is simply dequeued, and an in-flight command owned
> by this connection is cancelled without the risk of cancelling an
> unrelated command that became pending in the meantime. CIS uses the same
> path via the existing HCI_CONN_CREATE_CIS flag.
>
> hci_acl_create_conn_sync() and hci_le_create_conn_sync() clear
> HCI_CONN_CREATE after the create command completes, but the command
> status handler can free conn via hci_conn_del() (for example when the
> controller rejects the connection) while the worker is still blocked on
> the connection complete event. Hold a reference on conn across the
> create command so the flag can be cleared without a use-after-free.
>
> Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
> Cc: stable@vger.kernel.org
> Suggested-by: XIAO WU <xiaowu.417@qq.com>
> 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         | 96 ++++++++++++++++++++++++++++----
>  3 files changed, 88 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..06b87e9cd999 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -6611,6 +6611,11 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>
>         bt_dev_dbg(hdev, "conn %p", conn);
>
> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
> +        * clear_bit() at done.
> +        */
> +       hci_conn_get(conn);
> +
>         clear_bit(HCI_CONN_SCANNING, &conn->flags);
>         conn->state = BT_CONNECT;
>
> @@ -6623,6 +6628,7 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>                     hdev->le_scan_type == LE_SCAN_ACTIVE &&
>                     !hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
>                         hci_conn_del(conn);
> +                       hci_conn_put(conn);
>                         return -EBUSY;
>                 }
>
> @@ -6668,6 +6674,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,11 +6715,14 @@ 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);
>
>         /* Re-enable advertising after the connection attempt is finished. */
>         hci_resume_advertising_sync(hdev);
> +       hci_conn_put(conn);
>         return err;
>  }
>
> @@ -6982,10 +6997,25 @@ 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);
> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
> +        * clear_bit() below.
> +        */
> +       hci_conn_get(conn);
> +
> +       /* 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);
> +       hci_conn_put(conn);
> +
> +       return err;
>  }
>
>  int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
> @@ -7039,20 +7069,62 @@ 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;
> +       struct hci_cmd_sync_work_entry *entry;
> +       hci_cmd_sync_work_func_t func = NULL;
> +       hci_cmd_sync_work_destroy_t destroy = NULL;
> +       int create_flag = -1;
> +       int err = -EBUSY;
>
>         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;

Instead of doing everything in the same function, it probably makes
more sense to add dedicated functions for each type, for example,
`hci_acl_cancel_create_conn_sync`, etc.

> +               break;
> +       default:
> +               return -ENOENT;
>         }
>
> -       return -ENOENT;
> +       /* The create command is either still queued or in flight. Hold
> +        * cmd_sync_work_lock across the test and the cancel: the worker takes
> +        * this lock to dequeue every entry, so while it is held no other command
> +        * can become pending, which keeps hci_cmd_sync_cancel() from racing with
> +        * completion and cancelling an unrelated command.
> +        */
> +       mutex_lock(&hdev->cmd_sync_work_lock);
> +
> +       /* The flag is set while the worker blocks on the connection complete
> +        * event, so if it is set this connection owns the pending request.
> +        */
> +       if (create_flag >= 0 && test_bit(create_flag, &conn->flags)) {
> +               hci_cmd_sync_cancel(hdev, ECANCELED);
> +               goto unlock;
> +       }
> +
> +       /* Otherwise it may still be queued; dequeue it. A successful dequeue
> +        * means it never started, so there is nothing to disconnect.
> +        */
> +       if (func) {
> +               entry = _hci_cmd_sync_lookup_entry(hdev, func, conn, destroy);
> +               if (entry) {
> +                       _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
> +                       err = 0;
> +               }
> +       }
> +
> +unlock:
> +       mutex_unlock(&hdev->cmd_sync_work_lock);
> +       return err;
>  }
>
>  int hci_le_conn_update_sync(struct hci_dev *hdev, struct hci_conn *conn,
> --
> 2.54.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v5 1/2] Bluetooth: hci_conn: Fix null ptr deref in hci_abort_conn()

[Siwei Zhang]

Hi Luiz,

On Mon, Jun 15, 2026, at 10:09 AM, Luiz Augusto von Dentz wrote:
> Hi Siwei,
>
> On Mon, Jun 15, 2026 at 9:12 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(). The create
>> command is in exactly one of two states: still queued, or in flight.
>> hci_cancel_connect_sync() holds cmd_sync_work_lock across the whole
>> decision: the worker takes this lock to dequeue every entry, so while it
>> is held a queued command cannot start running and an in-flight command
>> cannot complete and let the next command become pending. This keeps the
>> flag test and hci_cmd_sync_cancel() atomic with respect to the worker,
>> so a queued command is simply dequeued, and an in-flight command owned
>> by this connection is cancelled without the risk of cancelling an
>> unrelated command that became pending in the meantime. CIS uses the same
>> path via the existing HCI_CONN_CREATE_CIS flag.
>>
>> hci_acl_create_conn_sync() and hci_le_create_conn_sync() clear
>> HCI_CONN_CREATE after the create command completes, but the command
>> status handler can free conn via hci_conn_del() (for example when the
>> controller rejects the connection) while the worker is still blocked on
>> the connection complete event. Hold a reference on conn across the
>> create command so the flag can be cleared without a use-after-free.
>>
>> Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
>> Cc: stable@vger.kernel.org
>> Suggested-by: XIAO WU <xiaowu.417@qq.com>
>> 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         | 96 ++++++++++++++++++++++++++++----
>>  3 files changed, 88 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..06b87e9cd999 100644
>> --- a/net/bluetooth/hci_sync.c
>> +++ b/net/bluetooth/hci_sync.c
>> @@ -6611,6 +6611,11 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>>
>>         bt_dev_dbg(hdev, "conn %p", conn);
>>
>> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
>> +        * clear_bit() at done.
>> +        */
>> +       hci_conn_get(conn);
>> +
>>         clear_bit(HCI_CONN_SCANNING, &conn->flags);
>>         conn->state = BT_CONNECT;
>>
>> @@ -6623,6 +6628,7 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>>                     hdev->le_scan_type == LE_SCAN_ACTIVE &&
>>                     !hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
>>                         hci_conn_del(conn);
>> +                       hci_conn_put(conn);
>>                         return -EBUSY;
>>                 }
>>
>> @@ -6668,6 +6674,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,11 +6715,14 @@ 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);
>>
>>         /* Re-enable advertising after the connection attempt is finished. */
>>         hci_resume_advertising_sync(hdev);
>> +       hci_conn_put(conn);
>>         return err;
>>  }
>>
>> @@ -6982,10 +6997,25 @@ 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);
>> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
>> +        * clear_bit() below.
>> +        */
>> +       hci_conn_get(conn);
>> +
>> +       /* 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);
>> +       hci_conn_put(conn);
>> +
>> +       return err;
>>  }
>>
>>  int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
>> @@ -7039,20 +7069,62 @@ 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;
>> +       struct hci_cmd_sync_work_entry *entry;
>> +       hci_cmd_sync_work_func_t func = NULL;
>> +       hci_cmd_sync_work_destroy_t destroy = NULL;
>> +       int create_flag = -1;
>> +       int err = -EBUSY;
>>
>>         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;
>
> Instead of doing everything in the same function, it probably makes
> more sense to add dedicated functions for each type, for example,
> `hci_acl_cancel_create_conn_sync`, etc.
>

int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
{
	switch (conn->type) {
	case ACL_LINK:
		return hci_cancel_create_conn_sync(hdev, conn, HCI_CONN_CREATE,
						   hci_acl_create_conn_sync,
						   NULL);
	case LE_LINK:
		return hci_cancel_create_conn_sync(hdev, conn, HCI_CONN_CREATE,
						   hci_le_create_conn_sync,
						   create_le_conn_complete);
	case CIS_LINK:
		return hci_cancel_create_conn_sync(hdev, conn,
						   HCI_CONN_CREATE_CIS, NULL,
						   NULL);
	default:
		return -ENOENT;
	}
}

I would like to have something like this. 
hci_acl_cancel_create_conn_sync will have an unnecessary wrapper.
What do you think?

>> +               break;
>> +       default:
>> +               return -ENOENT;
>>         }
>>
>> -       return -ENOENT;
>> +       /* The create command is either still queued or in flight. Hold
>> +        * cmd_sync_work_lock across the test and the cancel: the worker takes
>> +        * this lock to dequeue every entry, so while it is held no other command
>> +        * can become pending, which keeps hci_cmd_sync_cancel() from racing with
>> +        * completion and cancelling an unrelated command.
>> +        */
>> +       mutex_lock(&hdev->cmd_sync_work_lock);
>> +
>> +       /* The flag is set while the worker blocks on the connection complete
>> +        * event, so if it is set this connection owns the pending request.
>> +        */
>> +       if (create_flag >= 0 && test_bit(create_flag, &conn->flags)) {
>> +               hci_cmd_sync_cancel(hdev, ECANCELED);
>> +               goto unlock;
>> +       }
>> +
>> +       /* Otherwise it may still be queued; dequeue it. A successful dequeue
>> +        * means it never started, so there is nothing to disconnect.
>> +        */
>> +       if (func) {
>> +               entry = _hci_cmd_sync_lookup_entry(hdev, func, conn, destroy);
>> +               if (entry) {
>> +                       _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
>> +                       err = 0;
>> +               }
>> +       }
>> +
>> +unlock:
>> +       mutex_unlock(&hdev->cmd_sync_work_lock);
>> +       return err;
>>  }
>>
>>  int hci_le_conn_update_sync(struct hci_dev *hdev, struct hci_conn *conn,
>> --
>> 2.54.0
>>
>
>
> -- 
> Luiz Augusto von Dentz

Best,
Siwei

Re: [PATCH v5 1/2] Bluetooth: hci_conn: Fix null ptr deref in hci_abort_conn()

[Luiz Augusto von Dentz]

Hi Siwei,

On Mon, Jun 15, 2026 at 11:06 AM Siwei Zhang <oss@fourdim.xyz> wrote:
>
> Hi Luiz,
>
> On Mon, Jun 15, 2026, at 10:09 AM, Luiz Augusto von Dentz wrote:
> > Hi Siwei,
> >
> > On Mon, Jun 15, 2026 at 9:12 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(). The create
> >> command is in exactly one of two states: still queued, or in flight.
> >> hci_cancel_connect_sync() holds cmd_sync_work_lock across the whole
> >> decision: the worker takes this lock to dequeue every entry, so while it
> >> is held a queued command cannot start running and an in-flight command
> >> cannot complete and let the next command become pending. This keeps the
> >> flag test and hci_cmd_sync_cancel() atomic with respect to the worker,
> >> so a queued command is simply dequeued, and an in-flight command owned
> >> by this connection is cancelled without the risk of cancelling an
> >> unrelated command that became pending in the meantime. CIS uses the same
> >> path via the existing HCI_CONN_CREATE_CIS flag.
> >>
> >> hci_acl_create_conn_sync() and hci_le_create_conn_sync() clear
> >> HCI_CONN_CREATE after the create command completes, but the command
> >> status handler can free conn via hci_conn_del() (for example when the
> >> controller rejects the connection) while the worker is still blocked on
> >> the connection complete event. Hold a reference on conn across the
> >> create command so the flag can be cleared without a use-after-free.
> >>
> >> Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
> >> Cc: stable@vger.kernel.org
> >> Suggested-by: XIAO WU <xiaowu.417@qq.com>
> >> 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         | 96 ++++++++++++++++++++++++++++----
> >>  3 files changed, 88 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..06b87e9cd999 100644
> >> --- a/net/bluetooth/hci_sync.c
> >> +++ b/net/bluetooth/hci_sync.c
> >> @@ -6611,6 +6611,11 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
> >>
> >>         bt_dev_dbg(hdev, "conn %p", conn);
> >>
> >> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
> >> +        * clear_bit() at done.
> >> +        */
> >> +       hci_conn_get(conn);
> >> +
> >>         clear_bit(HCI_CONN_SCANNING, &conn->flags);
> >>         conn->state = BT_CONNECT;
> >>
> >> @@ -6623,6 +6628,7 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
> >>                     hdev->le_scan_type == LE_SCAN_ACTIVE &&
> >>                     !hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
> >>                         hci_conn_del(conn);
> >> +                       hci_conn_put(conn);
> >>                         return -EBUSY;
> >>                 }
> >>
> >> @@ -6668,6 +6674,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,11 +6715,14 @@ 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);
> >>
> >>         /* Re-enable advertising after the connection attempt is finished. */
> >>         hci_resume_advertising_sync(hdev);
> >> +       hci_conn_put(conn);
> >>         return err;
> >>  }
> >>
> >> @@ -6982,10 +6997,25 @@ 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);
> >> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
> >> +        * clear_bit() below.
> >> +        */
> >> +       hci_conn_get(conn);
> >> +
> >> +       /* 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);
> >> +       hci_conn_put(conn);
> >> +
> >> +       return err;
> >>  }
> >>
> >>  int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
> >> @@ -7039,20 +7069,62 @@ 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;
> >> +       struct hci_cmd_sync_work_entry *entry;
> >> +       hci_cmd_sync_work_func_t func = NULL;
> >> +       hci_cmd_sync_work_destroy_t destroy = NULL;
> >> +       int create_flag = -1;
> >> +       int err = -EBUSY;
> >>
> >>         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;
> >
> > Instead of doing everything in the same function, it probably makes
> > more sense to add dedicated functions for each type, for example,
> > `hci_acl_cancel_create_conn_sync`, etc.
> >
>
> int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
> {
>         switch (conn->type) {
>         case ACL_LINK:
>                 return hci_cancel_create_conn_sync(hdev, conn, HCI_CONN_CREATE,
>                                                    hci_acl_create_conn_sync,
>                                                    NULL);
>         case LE_LINK:
>                 return hci_cancel_create_conn_sync(hdev, conn, HCI_CONN_CREATE,
>                                                    hci_le_create_conn_sync,
>                                                    create_le_conn_complete);
>         case CIS_LINK:
>                 return hci_cancel_create_conn_sync(hdev, conn,
>                                                    HCI_CONN_CREATE_CIS, NULL,
>                                                    NULL);
>         default:
>                 return -ENOENT;
>         }
> }
>
> I would like to have something like this.
> hci_acl_cancel_create_conn_sync will have an unnecessary wrapper.
> What do you think?

No, just do what I said. Things like hci_cis_cancel_create_conn_sync
are actually quite different on their own. In fact, if we handle more
variants of conn->type the differences will actually become more
pronounced so let's not pretend they are similar.

> >> +               break;
> >> +       default:
> >> +               return -ENOENT;
> >>         }
> >>
> >> -       return -ENOENT;
> >> +       /* The create command is either still queued or in flight. Hold
> >> +        * cmd_sync_work_lock across the test and the cancel: the worker takes
> >> +        * this lock to dequeue every entry, so while it is held no other command
> >> +        * can become pending, which keeps hci_cmd_sync_cancel() from racing with
> >> +        * completion and cancelling an unrelated command.
> >> +        */
> >> +       mutex_lock(&hdev->cmd_sync_work_lock);
> >> +
> >> +       /* The flag is set while the worker blocks on the connection complete
> >> +        * event, so if it is set this connection owns the pending request.
> >> +        */
> >> +       if (create_flag >= 0 && test_bit(create_flag, &conn->flags)) {
> >> +               hci_cmd_sync_cancel(hdev, ECANCELED);
> >> +               goto unlock;
> >> +       }
> >> +
> >> +       /* Otherwise it may still be queued; dequeue it. A successful dequeue
> >> +        * means it never started, so there is nothing to disconnect.
> >> +        */
> >> +       if (func) {
> >> +               entry = _hci_cmd_sync_lookup_entry(hdev, func, conn, destroy);
> >> +               if (entry) {
> >> +                       _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
> >> +                       err = 0;
> >> +               }
> >> +       }
> >> +
> >> +unlock:
> >> +       mutex_unlock(&hdev->cmd_sync_work_lock);
> >> +       return err;
> >>  }
> >>
> >>  int hci_le_conn_update_sync(struct hci_dev *hdev, struct hci_conn *conn,
> >> --
> >> 2.54.0
> >>
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Best,
> Siwei



-- 
Luiz Augusto von Dentz

Re: [PATCH v5 1/2] Bluetooth: hci_conn: Fix null ptr deref in hci_abort_conn()

[Siwei Zhang]

Hi Luiz,

On Mon, Jun 15, 2026, at 11:11 AM, Luiz Augusto von Dentz wrote:
> Hi Siwei,
>
> On Mon, Jun 15, 2026 at 11:06 AM Siwei Zhang <oss@fourdim.xyz> wrote:
>>
>> Hi Luiz,
>>
>> On Mon, Jun 15, 2026, at 10:09 AM, Luiz Augusto von Dentz wrote:
>> > Hi Siwei,
>> >
>> > On Mon, Jun 15, 2026 at 9:12 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(). The create
>> >> command is in exactly one of two states: still queued, or in flight.
>> >> hci_cancel_connect_sync() holds cmd_sync_work_lock across the whole
>> >> decision: the worker takes this lock to dequeue every entry, so while it
>> >> is held a queued command cannot start running and an in-flight command
>> >> cannot complete and let the next command become pending. This keeps the
>> >> flag test and hci_cmd_sync_cancel() atomic with respect to the worker,
>> >> so a queued command is simply dequeued, and an in-flight command owned
>> >> by this connection is cancelled without the risk of cancelling an
>> >> unrelated command that became pending in the meantime. CIS uses the same
>> >> path via the existing HCI_CONN_CREATE_CIS flag.
>> >>
>> >> hci_acl_create_conn_sync() and hci_le_create_conn_sync() clear
>> >> HCI_CONN_CREATE after the create command completes, but the command
>> >> status handler can free conn via hci_conn_del() (for example when the
>> >> controller rejects the connection) while the worker is still blocked on
>> >> the connection complete event. Hold a reference on conn across the
>> >> create command so the flag can be cleared without a use-after-free.
>> >>
>> >> Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
>> >> Cc: stable@vger.kernel.org
>> >> Suggested-by: XIAO WU <xiaowu.417@qq.com>
>> >> 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         | 96 ++++++++++++++++++++++++++++----
>> >>  3 files changed, 88 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..06b87e9cd999 100644
>> >> --- a/net/bluetooth/hci_sync.c
>> >> +++ b/net/bluetooth/hci_sync.c
>> >> @@ -6611,6 +6611,11 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>> >>
>> >>         bt_dev_dbg(hdev, "conn %p", conn);
>> >>
>> >> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
>> >> +        * clear_bit() at done.
>> >> +        */
>> >> +       hci_conn_get(conn);
>> >> +
>> >>         clear_bit(HCI_CONN_SCANNING, &conn->flags);
>> >>         conn->state = BT_CONNECT;
>> >>
>> >> @@ -6623,6 +6628,7 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>> >>                     hdev->le_scan_type == LE_SCAN_ACTIVE &&
>> >>                     !hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
>> >>                         hci_conn_del(conn);
>> >> +                       hci_conn_put(conn);
>> >>                         return -EBUSY;
>> >>                 }
>> >>
>> >> @@ -6668,6 +6674,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,11 +6715,14 @@ 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);
>> >>
>> >>         /* Re-enable advertising after the connection attempt is finished. */
>> >>         hci_resume_advertising_sync(hdev);
>> >> +       hci_conn_put(conn);
>> >>         return err;
>> >>  }
>> >>
>> >> @@ -6982,10 +6997,25 @@ 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);
>> >> +       /* Hold a reference so conn stays valid for the HCI_CONN_CREATE
>> >> +        * clear_bit() below.
>> >> +        */
>> >> +       hci_conn_get(conn);
>> >> +
>> >> +       /* 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);
>> >> +       hci_conn_put(conn);
>> >> +
>> >> +       return err;
>> >>  }
>> >>
>> >>  int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
>> >> @@ -7039,20 +7069,62 @@ 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;
>> >> +       struct hci_cmd_sync_work_entry *entry;
>> >> +       hci_cmd_sync_work_func_t func = NULL;
>> >> +       hci_cmd_sync_work_destroy_t destroy = NULL;
>> >> +       int create_flag = -1;
>> >> +       int err = -EBUSY;
>> >>
>> >>         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;
>> >
>> > Instead of doing everything in the same function, it probably makes
>> > more sense to add dedicated functions for each type, for example,
>> > `hci_acl_cancel_create_conn_sync`, etc.
>> >
>>
>> int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
>> {
>>         switch (conn->type) {
>>         case ACL_LINK:
>>                 return hci_cancel_create_conn_sync(hdev, conn, HCI_CONN_CREATE,
>>                                                    hci_acl_create_conn_sync,
>>                                                    NULL);
>>         case LE_LINK:
>>                 return hci_cancel_create_conn_sync(hdev, conn, HCI_CONN_CREATE,
>>                                                    hci_le_create_conn_sync,
>>                                                    create_le_conn_complete);
>>         case CIS_LINK:
>>                 return hci_cancel_create_conn_sync(hdev, conn,
>>                                                    HCI_CONN_CREATE_CIS, NULL,
>>                                                    NULL);
>>         default:
>>                 return -ENOENT;
>>         }
>> }
>>
>> I would like to have something like this.
>> hci_acl_cancel_create_conn_sync will have an unnecessary wrapper.
>> What do you think?
>
> No, just do what I said. Things like hci_cis_cancel_create_conn_sync
> are actually quite different on their own. In fact, if we handle more
> variants of conn->type the differences will actually become more
> pronounced so let's not pretend they are similar.
>

OK, will do.

>> >> +               break;
>> >> +       default:
>> >> +               return -ENOENT;
>> >>         }
>> >>
>> >> -       return -ENOENT;
>> >> +       /* The create command is either still queued or in flight. Hold
>> >> +        * cmd_sync_work_lock across the test and the cancel: the worker takes
>> >> +        * this lock to dequeue every entry, so while it is held no other command
>> >> +        * can become pending, which keeps hci_cmd_sync_cancel() from racing with
>> >> +        * completion and cancelling an unrelated command.
>> >> +        */
>> >> +       mutex_lock(&hdev->cmd_sync_work_lock);
>> >> +
>> >> +       /* The flag is set while the worker blocks on the connection complete
>> >> +        * event, so if it is set this connection owns the pending request.
>> >> +        */
>> >> +       if (create_flag >= 0 && test_bit(create_flag, &conn->flags)) {
>> >> +               hci_cmd_sync_cancel(hdev, ECANCELED);
>> >> +               goto unlock;
>> >> +       }
>> >> +
>> >> +       /* Otherwise it may still be queued; dequeue it. A successful dequeue
>> >> +        * means it never started, so there is nothing to disconnect.
>> >> +        */
>> >> +       if (func) {
>> >> +               entry = _hci_cmd_sync_lookup_entry(hdev, func, conn, destroy);
>> >> +               if (entry) {
>> >> +                       _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
>> >> +                       err = 0;
>> >> +               }
>> >> +       }
>> >> +
>> >> +unlock:
>> >> +       mutex_unlock(&hdev->cmd_sync_work_lock);
>> >> +       return err;
>> >>  }
>> >>
>> >>  int hci_le_conn_update_sync(struct hci_dev *hdev, struct hci_conn *conn,
>> >> --
>> >> 2.54.0
>> >>
>> >
>> >
>> > --
>> > Luiz Augusto von Dentz
>>
>> Best,
>> Siwei
>
>
>
> -- 
> Luiz Augusto von Dentz

Best,
Siwei