[PATCH v2 0/8] Bluetooth: avoid concurrent deletion of hci_conn

[PATCH v2 0/8] Bluetooth: avoid concurrent deletion of hci_conn

[Pauli Virtanen]

This contains the simpler fixes from
https://lore.kernel.org/linux-bluetooth/cover.1758481869.git.pav@iki.fi/

Changes:
  v2:
  - Fix also hci_past_sync()
  - Drop "Bluetooth: L2CAP: fix hci_conn_valid() usage".
    This does not fix the race completely, as conn->hchan also
    can race, so leave for later.

hdev has two workqueues that run concurrently, and both may delete
hci_conn. hci_conn* pointers then require either (i) hdev/rcu lock
covering lookup and usage, (ii) hci_conn_get reference held, or (iii)
other more complex invariant.

If none applies, it's likely there are corner cases that hit UAF,
especially if controller misbehaves.

Trying to protect access with "other invariants" is quite error prone,
and I don't think there are such in most of this series.

Correct code in several places to follow the patterns (1)

    take lock
    conn = hci_conn_hash_lookup(...)
    if (conn)
	do_something(conn)
    release lock

or (2)

    take lock
    conn = hci_conn_hash_lookup(...)
    if (conn)
	conn = hci_conn_get(conn)
    release lock
    do_something_carefully(conn)
    hci_conn_put(conn)

Generally do_something_carefully should do (3)

    take lock
    if (hci_conn_valid(hdev, conn))
	do_something(conn)
    release lock

hci_conn_valid() shouldn't be called unless refcount on conn is known to
be held, as the pointer may otherwise already be freed, and even though
hci_conn_valid() doesn't dereference the comparison of freed pointer it
does is strictly speaking undefined behavior (kmalloc is not guaranteed
to not reuse addresses).

Some of the existing code is missing locks for (3), those need to be
addressed in separate series.

Pauli Virtanen (8):
  Bluetooth: hci_event: extend hdev lock in
    hci_le_remote_conn_param_req_evt
  Bluetooth: hci_conn: take hdev lock in set_cig_params_sync
  Bluetooth: mgmt: take lock and hold reference when handling hci_conn
  Bluetooth: hci_sync: extend conn_hash lookup RCU critical sections
  Bluetooth: hci_sync: make hci_cmd_sync_run* return -EEXIST if not run
  Bluetooth: hci_sync: hci_cmd_sync_queue_once() return -EEXIST if
    exists
  Bluetooth: hci_conn: hold reference in abort_conn_sync
  Bluetooth: hci_sync: hold references in hci_sync callbacks

 net/bluetooth/hci_conn.c  |  22 +++++-
 net/bluetooth/hci_event.c |  33 ++++----
 net/bluetooth/hci_sync.c  | 157 ++++++++++++++++++++++++++++++--------
 net/bluetooth/mgmt.c      |  42 +++++++++-
 4 files changed, 204 insertions(+), 50 deletions(-)

-- 
2.51.1

[PATCH v2 1/8] Bluetooth: hci_event: extend hdev lock in hci_le_remote_conn_param_req_evt

[Pauli Virtanen]

Cover conn lookup and field access with hdev lock in
hci_le_remote_conn_param_req_evt.

This avoids any concurrent deletion of the conn before we are done
dereferencing it.

Fixes: 95118dd4edfec ("Bluetooth: hci_event: Use of a function table to handle LE subevents")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - no change

 net/bluetooth/hci_event.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ba0a7b41611f..54f757017f3f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6675,25 +6675,31 @@ static void hci_le_remote_conn_param_req_evt(struct hci_dev *hdev, void *data,
 	latency = le16_to_cpu(ev->latency);
 	timeout = le16_to_cpu(ev->timeout);
 
+	hci_dev_lock(hdev);
+
 	hcon = hci_conn_hash_lookup_handle(hdev, handle);
-	if (!hcon || hcon->state != BT_CONNECTED)
-		return send_conn_param_neg_reply(hdev, handle,
-						 HCI_ERROR_UNKNOWN_CONN_ID);
+	if (!hcon || hcon->state != BT_CONNECTED) {
+		send_conn_param_neg_reply(hdev, handle,
+					  HCI_ERROR_UNKNOWN_CONN_ID);
+		goto unlock;
+	}
 
-	if (max > hcon->le_conn_max_interval)
-		return send_conn_param_neg_reply(hdev, handle,
-						 HCI_ERROR_INVALID_LL_PARAMS);
+	if (max > hcon->le_conn_max_interval) {
+		send_conn_param_neg_reply(hdev, handle,
+					  HCI_ERROR_INVALID_LL_PARAMS);
+		goto unlock;
+	}
 
-	if (hci_check_conn_params(min, max, latency, timeout))
-		return send_conn_param_neg_reply(hdev, handle,
-						 HCI_ERROR_INVALID_LL_PARAMS);
+	if (hci_check_conn_params(min, max, latency, timeout)) {
+		send_conn_param_neg_reply(hdev, handle,
+					  HCI_ERROR_INVALID_LL_PARAMS);
+		goto unlock;
+	}
 
 	if (hcon->role == HCI_ROLE_MASTER) {
 		struct hci_conn_params *params;
 		u8 store_hint;
 
-		hci_dev_lock(hdev);
-
 		params = hci_conn_params_lookup(hdev, &hcon->dst,
 						hcon->dst_type);
 		if (params) {
@@ -6706,8 +6712,6 @@ static void hci_le_remote_conn_param_req_evt(struct hci_dev *hdev, void *data,
 			store_hint = 0x00;
 		}
 
-		hci_dev_unlock(hdev);
-
 		mgmt_new_conn_param(hdev, &hcon->dst, hcon->dst_type,
 				    store_hint, min, max, latency, timeout);
 	}
@@ -6721,6 +6725,9 @@ static void hci_le_remote_conn_param_req_evt(struct hci_dev *hdev, void *data,
 	cp.max_ce_len = 0;
 
 	hci_send_cmd(hdev, HCI_OP_LE_CONN_PARAM_REQ_REPLY, sizeof(cp), &cp);
+
+unlock:
+	hci_dev_unlock(hdev);
 }
 
 static void hci_le_direct_adv_report_evt(struct hci_dev *hdev, void *data,
-- 
2.51.1

[PATCH v2 2/8] Bluetooth: hci_conn: take hdev lock in set_cig_params_sync

[Pauli Virtanen]

Take hdev lock to prevent hci_conn from being deleted or modified
concurrently.

Fixes: a091289218202 ("Bluetooth: hci_conn: Fix hci_le_set_cig_params")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - no change

 net/bluetooth/hci_conn.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d6162a95048e..d140e5740f92 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1825,9 +1825,13 @@ static int set_cig_params_sync(struct hci_dev *hdev, void *data)
 	u8 aux_num_cis = 0;
 	u8 cis_id;
 
+	hci_dev_lock(hdev);
+
 	conn = hci_conn_hash_lookup_cig(hdev, cig_id);
-	if (!conn)
+	if (!conn) {
+		hci_dev_unlock(hdev);
 		return 0;
+	}
 
 	qos = &conn->iso_qos;
 	pdu->cig_id = cig_id;
@@ -1866,6 +1870,8 @@ static int set_cig_params_sync(struct hci_dev *hdev, void *data)
 	}
 	pdu->num_cis = aux_num_cis;
 
+	hci_dev_unlock(hdev);
+
 	if (!pdu->num_cis)
 		return 0;
 
-- 
2.51.1

[PATCH v2 3/8] Bluetooth: mgmt: take lock and hold reference when handling hci_conn

[Pauli Virtanen]

Take hdev/rcu lock to prevent concurrent deletion of the hci_conn we are
handling.

When using hci_conn* pointers across critical sections, always take
refcount to keep the pointer valid.

For hci_abort_conn() only hold refcount, as the function takes
hdev->lock itself.

Fixes: 227a0cdf4a028 ("Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - no change

 net/bluetooth/mgmt.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 78b7af8bf45f..535c475c2d25 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3081,6 +3081,8 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
 	struct mgmt_cp_unpair_device *cp = cmd->param;
 	struct hci_conn *conn;
 
+	rcu_read_lock();
+
 	if (cp->addr.type == BDADDR_BREDR)
 		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
 					       &cp->addr.bdaddr);
@@ -3088,6 +3090,11 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
 		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
 					       le_addr_type(cp->addr.type));
 
+	if (conn)
+		hci_conn_get(conn);
+
+	rcu_read_unlock();
+
 	if (!conn)
 		return 0;
 
@@ -3095,6 +3102,7 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
 	 * will clean up the connection no matter the error.
 	 */
 	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
+	hci_conn_put(conn);
 
 	return 0;
 }
@@ -3242,6 +3250,8 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
 	struct mgmt_cp_disconnect *cp = cmd->param;
 	struct hci_conn *conn;
 
+	rcu_read_lock();
+
 	if (cp->addr.type == BDADDR_BREDR)
 		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
 					       &cp->addr.bdaddr);
@@ -3249,6 +3259,11 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
 		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
 					       le_addr_type(cp->addr.type));
 
+	if (conn)
+		hci_conn_get(conn);
+
+	rcu_read_unlock();
+
 	if (!conn)
 		return -ENOTCONN;
 
@@ -3256,6 +3271,7 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
 	 * will clean up the connection no matter the error.
 	 */
 	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
+	hci_conn_put(conn);
 
 	return 0;
 }
@@ -7372,6 +7388,9 @@ static void get_conn_info_complete(struct hci_dev *hdev, void *data, int err)
 		rp.max_tx_power = HCI_TX_POWER_INVALID;
 	}
 
+	if (conn)
+		hci_conn_put(conn);
+
 	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, MGMT_OP_GET_CONN_INFO, status,
 			  &rp, sizeof(rp));
 
@@ -7386,6 +7405,8 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
 	int err;
 	__le16   handle;
 
+	hci_dev_lock(hdev);
+
 	/* Make sure we are still connected */
 	if (cp->addr.type == BDADDR_BREDR)
 		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
@@ -7393,12 +7414,16 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
 	else
 		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
 
-	if (!conn || conn->state != BT_CONNECTED)
+	if (!conn || conn->state != BT_CONNECTED) {
+		hci_dev_unlock(hdev);
 		return MGMT_STATUS_NOT_CONNECTED;
+	}
 
-	cmd->user_data = conn;
+	cmd->user_data = hci_conn_get(conn);
 	handle = cpu_to_le16(conn->handle);
 
+	hci_dev_unlock(hdev);
+
 	/* Refresh RSSI each time */
 	err = hci_read_rssi_sync(hdev, handle);
 
@@ -7532,6 +7557,9 @@ static void get_clock_info_complete(struct hci_dev *hdev, void *data, int err)
 	}
 
 complete:
+	if (conn)
+		hci_conn_put(conn);
+
 	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, status, &rp,
 			  sizeof(rp));
 
@@ -7548,15 +7576,21 @@ static int get_clock_info_sync(struct hci_dev *hdev, void *data)
 	memset(&hci_cp, 0, sizeof(hci_cp));
 	hci_read_clock_sync(hdev, &hci_cp);
 
+	hci_dev_lock(hdev);
+
 	/* Make sure connection still exists */
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->addr.bdaddr);
-	if (!conn || conn->state != BT_CONNECTED)
+	if (!conn || conn->state != BT_CONNECTED) {
+		hci_dev_unlock(hdev);
 		return MGMT_STATUS_NOT_CONNECTED;
+	}
 
-	cmd->user_data = conn;
+	cmd->user_data = hci_conn_get(conn);
 	hci_cp.handle = cpu_to_le16(conn->handle);
 	hci_cp.which = 0x01; /* Piconet clock */
 
+	hci_dev_unlock(hdev);
+
 	return hci_read_clock_sync(hdev, &hci_cp);
 }
 
-- 
2.51.1

[PATCH v2 4/8] Bluetooth: hci_sync: extend conn_hash lookup RCU critical sections

[Pauli Virtanen]

Extend critical section to cover both hci_conn_hash lookup and use of
the returned conn.  Add separate function for when we are just checking
if a conn exists.

This avoids concurrent deletion of the conn before we are done
dereferencing it.

Fixes: 58ddd115fe063 ("Bluetooth: hci_conn: Fix not setting conn_timeout for Broadcast Receiver")
Fixes: cf75ad8b41d2a ("Bluetooth: hci_sync: Convert MGMT_SET_POWERED")
Fixes: c2994b008492d ("Bluetooth: hci_sync: Fix not setting Random Address when required")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - no change

 net/bluetooth/hci_sync.c | 49 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index a87ae23f7bbc..a71a1b7b2541 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1035,6 +1035,21 @@ static bool adv_use_rpa(struct hci_dev *hdev, uint32_t flags)
 	return true;
 }
 
+static bool hci_check_le_connect(struct hci_dev *hdev)
+{
+	bool found;
+
+	rcu_read_lock();
+	found = hci_lookup_le_connect(hdev);
+	rcu_read_unlock();
+
+	/* The return value may be wrong if the conn is modified concurrently,
+	 * e.g. by HCI event. This function should be used only when this is OK
+	 * (time of check doesn't matter or operation will be tried again).
+	 */
+	return found;
+}
+
 static int hci_set_random_addr_sync(struct hci_dev *hdev, bdaddr_t *rpa)
 {
 	/* If a random_addr has been set we're advertising or initiating an LE
@@ -1049,7 +1064,7 @@ static int hci_set_random_addr_sync(struct hci_dev *hdev, bdaddr_t *rpa)
 	 */
 	if (bacmp(&hdev->random_addr, BDADDR_ANY) &&
 	    (hci_dev_test_flag(hdev, HCI_LE_ADV) ||
-	    hci_lookup_le_connect(hdev))) {
+	    hci_check_le_connect(hdev))) {
 		bt_dev_dbg(hdev, "Deferring random address update");
 		hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
 		return 0;
@@ -2636,7 +2651,7 @@ static int hci_pause_addr_resolution(struct hci_dev *hdev)
 	 * when initiating an LE connection.
 	 */
 	if (hci_dev_test_flag(hdev, HCI_LE_SCAN) ||
-	    hci_lookup_le_connect(hdev)) {
+	    hci_check_le_connect(hdev)) {
 		bt_dev_err(hdev, "Command not allowed when scan/LE connect");
 		return -EPERM;
 	}
@@ -2778,6 +2793,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 	if (hci_dev_test_flag(hdev, HCI_PA_SYNC)) {
 		struct hci_conn *conn;
 
+		rcu_read_lock();
+
 		conn = hci_conn_hash_lookup_create_pa_sync(hdev);
 		if (conn) {
 			struct conn_params pa;
@@ -2787,6 +2804,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 			bacpy(&pa.addr, &conn->dst);
 			pa.addr_type = conn->dst_type;
 
+			rcu_read_unlock();
+
 			/* Clear first since there could be addresses left
 			 * behind.
 			 */
@@ -2796,6 +2815,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 			err = hci_le_add_accept_list_sync(hdev, &pa,
 							  &num_entries);
 			goto done;
+		} else {
+			rcu_read_unlock();
 		}
 	}
 
@@ -2806,10 +2827,13 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 	 * the controller.
 	 */
 	list_for_each_entry_safe(b, t, &hdev->le_accept_list, list) {
-		if (hci_conn_hash_lookup_le(hdev, &b->bdaddr, b->bdaddr_type))
-			continue;
+		rcu_read_lock();
+
+		if (hci_conn_hash_lookup_le(hdev, &b->bdaddr, b->bdaddr_type)) {
+			rcu_read_unlock();
+			continue;
+		}
 
-		/* Pointers not dereferenced, no locks needed */
 		pend_conn = hci_pend_le_action_lookup(&hdev->pend_le_conns,
 						      &b->bdaddr,
 						      b->bdaddr_type);
@@ -2817,6 +2841,8 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
 							&b->bdaddr,
 							b->bdaddr_type);
 
+		rcu_read_unlock();
+
 		/* If the device is not likely to connect or report,
 		 * remove it from the acceptlist.
 		 */
@@ -2943,6 +2969,8 @@ static int hci_le_set_ext_scan_param_sync(struct hci_dev *hdev, u8 type,
 		if (sent) {
 			struct hci_conn *conn;
 
+			rcu_read_lock();
+
 			conn = hci_conn_hash_lookup_ba(hdev, PA_LINK,
 						       &sent->bdaddr);
 			if (conn) {
@@ -2967,8 +2995,12 @@ static int hci_le_set_ext_scan_param_sync(struct hci_dev *hdev, u8 type,
 					phy++;
 				}
 
+				rcu_read_unlock();
+
 				if (num_phy)
 					goto done;
+			} else {
+				rcu_read_unlock();
 			}
 		}
 	}
@@ -3224,7 +3256,7 @@ int hci_update_passive_scan_sync(struct hci_dev *hdev)
 		 * since some controllers are not able to scan and connect at
 		 * the same time.
 		 */
-		if (hci_lookup_le_connect(hdev))
+		if (hci_check_le_connect(hdev))
 			return 0;
 
 		bt_dev_dbg(hdev, "start background scanning");
@@ -3479,12 +3511,17 @@ int hci_update_scan_sync(struct hci_dev *hdev)
 	if (hdev->scanning_paused)
 		return 0;
 
+	/* If connection states change we update again, so lockless is OK */
+	rcu_read_lock();
+
 	if (hci_dev_test_flag(hdev, HCI_CONNECTABLE) ||
 	    disconnected_accept_list_entries(hdev))
 		scan = SCAN_PAGE;
 	else
 		scan = SCAN_DISABLED;
 
+	rcu_read_unlock();
+
 	if (hci_dev_test_flag(hdev, HCI_DISCOVERABLE))
 		scan |= SCAN_INQUIRY;
 
-- 
2.51.1

[PATCH v2 5/8] Bluetooth: hci_sync: make hci_cmd_sync_run* return -EEXIST if not run

[Pauli Virtanen]

hci_cmd_sync_run_once() needs to indicate whether a queue item was
added, so caller can know if callbacks are called, so it can avoid
leaking resources.

Change the function to return -EEXIST if queue item already exists.

hci_cmd_sync_run() may run the work immediately. If so, make it behave
as if item was queued successfully. Change it to call also destroy() and
return 0.

Modify all callsites vs. the changes.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - no change

 net/bluetooth/hci_conn.c |  4 +++-
 net/bluetooth/hci_sync.c | 13 ++++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d140e5740f92..214fa6ec832b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2959,6 +2959,7 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data)
 int hci_abort_conn(struct hci_conn *conn, u8 reason)
 {
 	struct hci_dev *hdev = conn->hdev;
+	int err;
 
 	/* If abort_reason has already been set it means the connection is
 	 * already being aborted so don't attempt to overwrite it.
@@ -2995,7 +2996,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
 	 * as a result to MGMT_OP_DISCONNECT/MGMT_OP_UNPAIR which does
 	 * already queue its callback on cmd_sync_work.
 	 */
-	return hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
+	err = hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
+	return (err == -EEXIST) ? 0 : err;
 }
 
 void hci_setup_tx_timestamp(struct sk_buff *skb, size_t key_offset,
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index a71a1b7b2541..6c4c736cf93a 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -801,8 +801,15 @@ int hci_cmd_sync_run(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 		return -ENETDOWN;
 
 	/* If on cmd_sync_work then run immediately otherwise queue */
-	if (current_work() == &hdev->cmd_sync_work)
-		return func(hdev, data);
+	if (current_work() == &hdev->cmd_sync_work) {
+		int err;
+
+		err = func(hdev, data);
+		if (destroy)
+			destroy(hdev, data, err);
+
+		return 0;
+	}
 
 	return hci_cmd_sync_submit(hdev, func, data, destroy);
 }
@@ -818,7 +825,7 @@ int hci_cmd_sync_run_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 			  void *data, hci_cmd_sync_work_destroy_t destroy)
 {
 	if (hci_cmd_sync_lookup_entry(hdev, func, data, destroy))
-		return 0;
+		return -EEXIST;
 
 	return hci_cmd_sync_run(hdev, func, data, destroy);
 }
-- 
2.51.1

[PATCH v2 6/8] Bluetooth: hci_sync: hci_cmd_sync_queue_once() return -EEXIST if exists

[Pauli Virtanen]

hci_cmd_sync_queue_once() needs to indicate whether a queue item was
added, so caller can know if callbacks are called, so it can avoid
leaking resources.

Change the function to return -EEXIST if queue item already exists.

Modify all callsites to handle that.

Fixes leak in hci_past_sync() if command already queued.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - fix also hci_past_sync

 net/bluetooth/hci_sync.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 6c4c736cf93a..dc95a1ebe65e 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -780,7 +780,7 @@ int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 			    void *data, hci_cmd_sync_work_destroy_t destroy)
 {
 	if (hci_cmd_sync_lookup_entry(hdev, func, data, destroy))
-		return 0;
+		return -EEXIST;
 
 	return hci_cmd_sync_queue(hdev, func, data, destroy);
 }
@@ -3294,6 +3294,8 @@ static int update_passive_scan_sync(struct hci_dev *hdev, void *data)
 
 int hci_update_passive_scan(struct hci_dev *hdev)
 {
+	int err;
+
 	/* Only queue if it would have any effect */
 	if (!test_bit(HCI_UP, &hdev->flags) ||
 	    test_bit(HCI_INIT, &hdev->flags) ||
@@ -3303,8 +3305,9 @@ int hci_update_passive_scan(struct hci_dev *hdev)
 	    hci_dev_test_flag(hdev, HCI_UNREGISTER))
 		return 0;
 
-	return hci_cmd_sync_queue_once(hdev, update_passive_scan_sync, NULL,
-				       NULL);
+	err = hci_cmd_sync_queue_once(hdev, update_passive_scan_sync, NULL,
+				      NULL);
+	return (err == -EEXIST) ? 0 : err;
 }
 
 int hci_write_sc_support_sync(struct hci_dev *hdev, u8 val)
@@ -6961,8 +6964,11 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
 
 int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
-	return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
-				       NULL);
+	int err;
+
+	err = hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
+				      NULL);
+	return (err == -EEXIST) ? 0 : err;
 }
 
 static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
@@ -6998,8 +7004,11 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
 
 int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
-	return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
-				       create_le_conn_complete);
+	int err;
+
+	err = hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
+				      create_le_conn_complete);
+	return (err == -EEXIST) ? 0 : err;
 }
 
 int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
@@ -7206,8 +7215,11 @@ static int hci_le_pa_create_sync(struct hci_dev *hdev, void *data)
 
 int hci_connect_pa_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
-	return hci_cmd_sync_queue_once(hdev, hci_le_pa_create_sync, conn,
-				       create_pa_complete);
+	int err;
+
+	err = hci_cmd_sync_queue_once(hdev, hci_le_pa_create_sync, conn,
+				      create_pa_complete);
+	return (err == -EEXIST) ? 0 : err;
 }
 
 static void create_big_complete(struct hci_dev *hdev, void *data, int err)
@@ -7269,8 +7281,11 @@ static int hci_le_big_create_sync(struct hci_dev *hdev, void *data)
 
 int hci_connect_big_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
-	return hci_cmd_sync_queue_once(hdev, hci_le_big_create_sync, conn,
-				       create_big_complete);
+	int err;
+
+	err = hci_cmd_sync_queue_once(hdev, hci_le_big_create_sync, conn,
+				      create_big_complete);
+	return (err == -EEXIST) ? 0 : err;
 }
 
 struct past_data {
@@ -7362,5 +7377,5 @@ int hci_past_sync(struct hci_conn *conn, struct hci_conn *le)
 	if (err)
 		kfree(data);
 
-	return err;
+	return (err == -EEXIST) ? 0 : err;
 }
-- 
2.51.1

[PATCH v2 7/8] Bluetooth: hci_conn: hold reference in abort_conn_sync

[Pauli Virtanen]

hci_conn_valid() should not be used on potentially freed hci_conn
pointers, as relying on kmalloc not reusing addresses is bad practice.

Hold a hci_conn reference for the queue job so the pointer is not freed
too early.

This also avoids potential UAF during abort_conn_sync().

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - no change

 net/bluetooth/hci_conn.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 214fa6ec832b..64066f6a0af8 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2956,6 +2956,13 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data)
 	return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
 }
 
+static void abort_conn_destroy(struct hci_dev *hdev, void *data, int err)
+{
+	struct hci_conn *conn = data;
+
+	hci_conn_put(conn);
+}
+
 int hci_abort_conn(struct hci_conn *conn, u8 reason)
 {
 	struct hci_dev *hdev = conn->hdev;
@@ -2996,7 +3003,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
 	 * as a result to MGMT_OP_DISCONNECT/MGMT_OP_UNPAIR which does
 	 * already queue its callback on cmd_sync_work.
 	 */
-	err = hci_cmd_sync_run_once(hdev, abort_conn_sync, conn, NULL);
+	err = hci_cmd_sync_run_once(hdev, abort_conn_sync, hci_conn_get(conn),
+				    abort_conn_destroy);
+	if (err)
+		hci_conn_put(conn);
 	return (err == -EEXIST) ? 0 : err;
 }
 
-- 
2.51.1

[PATCH v2 8/8] Bluetooth: hci_sync: hold references in hci_sync callbacks

[Pauli Virtanen]

hci_conn_valid() should not be used on potentially freed hci_conn
pointers, as relying on kmalloc not reusing addresses is bad practice.

Hold a hci_conn reference for queued jobs so the pointers are not freed
too early.

This also avoids potential UAF if the conn would be freed while the jobs
are running.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2:
    - fix also hci_past_sync()

 net/bluetooth/hci_sync.c | 66 +++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 15 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index dc95a1ebe65e..d4420d5882d6 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -6962,12 +6962,23 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
 					conn->conn_timeout, NULL);
 }
 
+static void hci_acl_create_conn_sync_complete(struct hci_dev *hdev, void *data,
+					      int err)
+{
+	struct hci_conn *conn = data;
+
+	hci_conn_put(conn);
+}
+
 int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
 	int err;
 
-	err = hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
-				      NULL);
+	err = hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync,
+				      hci_conn_get(conn),
+				      hci_acl_create_conn_sync_complete);
+	if (err)
+		hci_conn_put(conn);
 	return (err == -EEXIST) ? 0 : err;
 }
 
@@ -6978,36 +6989,41 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
 	bt_dev_dbg(hdev, "err %d", err);
 
 	if (err == -ECANCELED)
-		return;
+		goto done;
 
 	hci_dev_lock(hdev);
 
 	if (!hci_conn_valid(hdev, conn))
-		goto done;
+		goto unlock;
 
 	if (!err) {
 		hci_connect_le_scan_cleanup(conn, 0x00);
-		goto done;
+		goto unlock;
 	}
 
 	/* Check if connection is still pending */
 	if (conn != hci_lookup_le_connect(hdev))
-		goto done;
+		goto unlock;
 
 	/* Flush to make sure we send create conn cancel command if needed */
 	flush_delayed_work(&conn->le_conn_timeout);
 	hci_conn_failed(conn, bt_status(err));
 
-done:
+unlock:
 	hci_dev_unlock(hdev);
+done:
+	hci_conn_put(conn);
 }
 
 int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
 	int err;
 
-	err = hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
+	err = hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync,
+				      hci_conn_get(conn),
 				      create_le_conn_complete);
+	if (err)
+		hci_conn_put(conn);
 	return (err == -EEXIST) ? 0 : err;
 }
 
@@ -7055,7 +7071,7 @@ static void create_pa_complete(struct hci_dev *hdev, void *data, int err)
 	bt_dev_dbg(hdev, "err %d", err);
 
 	if (err == -ECANCELED)
-		return;
+		goto done;
 
 	hci_dev_lock(hdev);
 
@@ -7079,6 +7095,8 @@ static void create_pa_complete(struct hci_dev *hdev, void *data, int err)
 
 unlock:
 	hci_dev_unlock(hdev);
+done:
+	hci_conn_put(conn);
 }
 
 static int hci_le_past_params_sync(struct hci_dev *hdev, struct hci_conn *conn,
@@ -7217,8 +7235,11 @@ int hci_connect_pa_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
 	int err;
 
-	err = hci_cmd_sync_queue_once(hdev, hci_le_pa_create_sync, conn,
+	err = hci_cmd_sync_queue_once(hdev, hci_le_pa_create_sync,
+				      hci_conn_get(conn),
 				      create_pa_complete);
+	if (err)
+		hci_conn_put(conn);
 	return (err == -EEXIST) ? 0 : err;
 }
 
@@ -7229,10 +7250,17 @@ static void create_big_complete(struct hci_dev *hdev, void *data, int err)
 	bt_dev_dbg(hdev, "err %d", err);
 
 	if (err == -ECANCELED)
-		return;
+		goto done;
+
+	hci_dev_lock(hdev);
 
 	if (hci_conn_valid(hdev, conn))
 		clear_bit(HCI_CONN_CREATE_BIG_SYNC, &conn->flags);
+
+	hci_dev_unlock(hdev);
+
+done:
+	hci_conn_put(conn);
 }
 
 static int hci_le_big_create_sync(struct hci_dev *hdev, void *data)
@@ -7283,8 +7311,11 @@ int hci_connect_big_sync(struct hci_dev *hdev, struct hci_conn *conn)
 {
 	int err;
 
-	err = hci_cmd_sync_queue_once(hdev, hci_le_big_create_sync, conn,
+	err = hci_cmd_sync_queue_once(hdev, hci_le_big_create_sync,
+				      hci_conn_get(conn),
 				      create_big_complete);
+	if (err)
+		hci_conn_put(conn);
 	return (err == -EEXIST) ? 0 : err;
 }
 
@@ -7299,6 +7330,8 @@ static void past_complete(struct hci_dev *hdev, void *data, int err)
 
 	bt_dev_dbg(hdev, "err %d", err);
 
+	hci_conn_put(past->conn);
+	hci_conn_put(past->le);
 	kfree(past);
 }
 
@@ -7363,8 +7396,8 @@ int hci_past_sync(struct hci_conn *conn, struct hci_conn *le)
 	if (!data)
 		return -ENOMEM;
 
-	data->conn = conn;
-	data->le = le;
+	data->conn = hci_conn_get(conn);
+	data->le = hci_conn_get(le);
 
 	if (conn->role == HCI_ROLE_MASTER)
 		err = hci_cmd_sync_queue_once(conn->hdev,
@@ -7374,8 +7407,11 @@ int hci_past_sync(struct hci_conn *conn, struct hci_conn *le)
 		err = hci_cmd_sync_queue_once(conn->hdev, hci_le_past_sync,
 					      data, past_complete);
 
-	if (err)
+	if (err) {
+		hci_conn_put(data->conn);
+		hci_conn_put(data->le);
 		kfree(data);
+	}
 
 	return (err == -EEXIST) ? 0 : err;
 }
-- 
2.51.1

RE: Bluetooth: avoid concurrent deletion of hci_conn

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2639 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1018632

---Test result---

Test Summary:
CheckPatch                    PENDING   0.45 seconds
GitLint                       PENDING   0.30 seconds
SubjectPrefix                 PASS      0.44 seconds
BuildKernel                   PASS      26.15 seconds
CheckAllWarning               PASS      28.32 seconds
CheckSparse                   WARNING   32.21 seconds
BuildKernel32                 PASS      25.26 seconds
TestRunnerSetup               PASS      511.05 seconds
TestRunner_l2cap-tester       PASS      24.32 seconds
TestRunner_iso-tester         PASS      95.20 seconds
TestRunner_bnep-tester        PASS      6.46 seconds
TestRunner_mgmt-tester        FAIL      124.39 seconds
TestRunner_rfcomm-tester      PASS      9.34 seconds
TestRunner_sco-tester         PASS      14.57 seconds
TestRunner_ioctl-tester       PASS      10.13 seconds
TestRunner_mesh-tester        FAIL      10.41 seconds
TestRunner_smp-tester         PASS      8.54 seconds
TestRunner_userchan-tester    PASS      6.66 seconds
IncrementalBuild              PENDING   0.71 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.105 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
BUG: KASAN: slab-use-after-free in run_timer_softirq+0x76b/0x7d0
WARNING: CPU: 0 PID: 68 at kernel/workqueue.c:2257 __queue_work+0x97d/0xbe0
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Failed       0.130 seconds
Mesh - Send cancel - 2                               Timed out    2.672 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: Bluetooth: avoid concurrent deletion of hci_conn

[Pauli Virtanen]

su, 2025-11-02 kello 09:04 -0800, bluez.test.bot@gmail.com kirjoitti:

[clip]
> ##############################
> Test: TestRunner_mesh-tester - FAIL
> Desc: Run mesh-tester with test-runner
> Output:
> BUG: KASAN: slab-use-after-free in run_timer_softirq+0x76b/0x7d0
> WARNING: CPU: 0 PID: 68 at kernel/workqueue.c:2257 __queue_work+0x97d/0xbe0
> Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0
> 
> Failed Test Cases
> Mesh - Send cancel - 1                               Failed       0.130 seconds
> Mesh - Send cancel - 2                               Timed out    2.672 seconds

The BUG: KASAN: is a bit worrying, but seems like a pre-existing issue.

Earliest is from Nov 2024 when we added reporting BUG/WARNING in
testbot, so probably the issue is yet older:

https://lore.kernel.org/linux-bluetooth/67364c09.0c0a0220.113cba.39ff@mx.google.com/

https://lore.kernel.org/linux-bluetooth/68624da5.050a0220.250d4b.db51@mx.google.com/

https://lore.kernel.org/linux-bluetooth/6901d2e3.170a0220.3c1513.43df@mx.google.com/

https://lore.kernel.org/linux-bluetooth/6905e85a.020a0220.2018ac.29be@mx.google.com/

Can't reproduce it locally, so probably somehow timing sensitive.

-- 
Pauli Virtanen

Re: [PATCH v2 3/8] Bluetooth: mgmt: take lock and hold reference when handling hci_conn

[Hillf Danton]

On Sun,  2 Nov 2025 18:19:35 +0200 Pauli Virtanen wrote:
> Take hdev/rcu lock to prevent concurrent deletion of the hci_conn we are
> handling.
> 
> When using hci_conn* pointers across critical sections, always take
> refcount to keep the pointer valid.
> 
> For hci_abort_conn() only hold refcount, as the function takes
> hdev->lock itself.
> 
> Fixes: 227a0cdf4a028 ("Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT")
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
> 
> Notes:
>     v2:
>     - no change
> 
>  net/bluetooth/mgmt.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 78b7af8bf45f..535c475c2d25 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3081,6 +3081,8 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
>  	struct mgmt_cp_unpair_device *cp = cmd->param;
>  	struct hci_conn *conn;
>  
> +	rcu_read_lock();
> +
>  	if (cp->addr.type == BDADDR_BREDR)
>  		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>  					       &cp->addr.bdaddr);
> @@ -3088,6 +3090,11 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
>  		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
>  					       le_addr_type(cp->addr.type));
>  
> +	if (conn)
> +		hci_conn_get(conn);
> +
> +	rcu_read_unlock();
> +
Given RCU in the lookup helpers, nested RCU makes no sense.
Feel free to add the lookup_and_get helper instead to bump the refcnt up
for the conn found.

Another simpler option is -- add conn to hash with refcnt increased and
correspondingly put conn after deleting it from hash.

>  	if (!conn)
>  		return 0;
>  
> @@ -3095,6 +3102,7 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
>  	 * will clean up the connection no matter the error.
>  	 */
>  	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> +	hci_conn_put(conn);
>  
>  	return 0;
>  }
> @@ -3242,6 +3250,8 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
>  	struct mgmt_cp_disconnect *cp = cmd->param;
>  	struct hci_conn *conn;
>  
> +	rcu_read_lock();
> +
>  	if (cp->addr.type == BDADDR_BREDR)
>  		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>  					       &cp->addr.bdaddr);
> @@ -3249,6 +3259,11 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
>  		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
>  					       le_addr_type(cp->addr.type));
>  
> +	if (conn)
> +		hci_conn_get(conn);
> +
> +	rcu_read_unlock();
> +
>  	if (!conn)
>  		return -ENOTCONN;
>  
> @@ -3256,6 +3271,7 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
>  	 * will clean up the connection no matter the error.
>  	 */
>  	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> +	hci_conn_put(conn);
>  
>  	return 0;
>  }
> @@ -7372,6 +7388,9 @@ static void get_conn_info_complete(struct hci_dev *hdev, void *data, int err)
>  		rp.max_tx_power = HCI_TX_POWER_INVALID;
>  	}
>  
> +	if (conn)
> +		hci_conn_put(conn);
> +
>  	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, MGMT_OP_GET_CONN_INFO, status,
>  			  &rp, sizeof(rp));
>  
> @@ -7386,6 +7405,8 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
>  	int err;
>  	__le16   handle;
>  
> +	hci_dev_lock(hdev);
> +
>  	/* Make sure we are still connected */
>  	if (cp->addr.type == BDADDR_BREDR)
>  		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> @@ -7393,12 +7414,16 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
>  	else
>  		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
>  
> -	if (!conn || conn->state != BT_CONNECTED)
> +	if (!conn || conn->state != BT_CONNECTED) {
> +		hci_dev_unlock(hdev);
>  		return MGMT_STATUS_NOT_CONNECTED;
> +	}
>  
> -	cmd->user_data = conn;
> +	cmd->user_data = hci_conn_get(conn);
>  	handle = cpu_to_le16(conn->handle);
>  
> +	hci_dev_unlock(hdev);
> +
>  	/* Refresh RSSI each time */
>  	err = hci_read_rssi_sync(hdev, handle);
>  
> @@ -7532,6 +7557,9 @@ static void get_clock_info_complete(struct hci_dev *hdev, void *data, int err)
>  	}
>  
>  complete:
> +	if (conn)
> +		hci_conn_put(conn);
> +
>  	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, status, &rp,
>  			  sizeof(rp));
>  
> @@ -7548,15 +7576,21 @@ static int get_clock_info_sync(struct hci_dev *hdev, void *data)
>  	memset(&hci_cp, 0, sizeof(hci_cp));
>  	hci_read_clock_sync(hdev, &hci_cp);
>  
> +	hci_dev_lock(hdev);
> +
>  	/* Make sure connection still exists */
>  	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->addr.bdaddr);
> -	if (!conn || conn->state != BT_CONNECTED)
> +	if (!conn || conn->state != BT_CONNECTED) {
> +		hci_dev_unlock(hdev);
>  		return MGMT_STATUS_NOT_CONNECTED;
> +	}
>  
> -	cmd->user_data = conn;
> +	cmd->user_data = hci_conn_get(conn);
>  	hci_cp.handle = cpu_to_le16(conn->handle);
>  	hci_cp.which = 0x01; /* Piconet clock */
>  
> +	hci_dev_unlock(hdev);
> +
>  	return hci_read_clock_sync(hdev, &hci_cp);
>  }
>  
> -- 
> 2.51.1

Re: [PATCH v2 3/8] Bluetooth: mgmt: take lock and hold reference when handling hci_conn

[Pauli Virtanen]

ma, 2025-11-03 kello 07:21 +0800, Hillf Danton kirjoitti:
> On Sun,  2 Nov 2025 18:19:35 +0200 Pauli Virtanen wrote:
> > Take hdev/rcu lock to prevent concurrent deletion of the hci_conn we are
> > handling.
> > 
> > When using hci_conn* pointers across critical sections, always take
> > refcount to keep the pointer valid.
> > 
> > For hci_abort_conn() only hold refcount, as the function takes
> > hdev->lock itself.
> > 
> > Fixes: 227a0cdf4a028 ("Bluetooth: MGMT: Fix not generating command complete for MGMT_OP_DISCONNECT")
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> > 
> > Notes:
> >     v2:
> >     - no change
> > 
> >  net/bluetooth/mgmt.c | 42 ++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 78b7af8bf45f..535c475c2d25 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -3081,6 +3081,8 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> >  	struct mgmt_cp_unpair_device *cp = cmd->param;
> >  	struct hci_conn *conn;
> >  
> > +	rcu_read_lock();
> > +
> >  	if (cp->addr.type == BDADDR_BREDR)
> >  		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> >  					       &cp->addr.bdaddr);
> > @@ -3088,6 +3090,11 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> >  		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> >  					       le_addr_type(cp->addr.type));
> >  
> > +	if (conn)
> > +		hci_conn_get(conn);
> > +
> > +	rcu_read_unlock();
> > +
> Given RCU in the lookup helpers, nested RCU makes no sense.
> Feel free to add the lookup_and_get helper instead to bump the refcnt up
> for the conn found.

RCU must be held until the refcount is increased. I don't think there
is difference in whether it's done by nesting or by duplicating all the
helpers without the rcu_read_lock().

The rcu_read_lock() in the helpers is probaly something that should be
done away with --- the caller needs to hold RCU or suitable other lock
if it wants to use the returned value safely.

> Another simpler option is -- add conn to hash with refcnt increased and
> correspondingly put conn after deleting it from hash.
> 
> >  	if (!conn)
> >  		return 0;
> >  
> > @@ -3095,6 +3102,7 @@ static int unpair_device_sync(struct hci_dev *hdev, void *data)
> >  	 * will clean up the connection no matter the error.
> >  	 */
> >  	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> > +	hci_conn_put(conn);
> >  
> >  	return 0;
> >  }
> > @@ -3242,6 +3250,8 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> >  	struct mgmt_cp_disconnect *cp = cmd->param;
> >  	struct hci_conn *conn;
> >  
> > +	rcu_read_lock();
> > +
> >  	if (cp->addr.type == BDADDR_BREDR)
> >  		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> >  					       &cp->addr.bdaddr);
> > @@ -3249,6 +3259,11 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> >  		conn = hci_conn_hash_lookup_le(hdev, &cp->addr.bdaddr,
> >  					       le_addr_type(cp->addr.type));
> >  
> > +	if (conn)
> > +		hci_conn_get(conn);
> > +
> > +	rcu_read_unlock();
> > +
> >  	if (!conn)
> >  		return -ENOTCONN;
> >  
> > @@ -3256,6 +3271,7 @@ static int disconnect_sync(struct hci_dev *hdev, void *data)
> >  	 * will clean up the connection no matter the error.
> >  	 */
> >  	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> > +	hci_conn_put(conn);
> >  
> >  	return 0;
> >  }
> > @@ -7372,6 +7388,9 @@ static void get_conn_info_complete(struct hci_dev *hdev, void *data, int err)
> >  		rp.max_tx_power = HCI_TX_POWER_INVALID;
> >  	}
> >  
> > +	if (conn)
> > +		hci_conn_put(conn);
> > +
> >  	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, MGMT_OP_GET_CONN_INFO, status,
> >  			  &rp, sizeof(rp));
> >  
> > @@ -7386,6 +7405,8 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
> >  	int err;
> >  	__le16   handle;
> >  
> > +	hci_dev_lock(hdev);
> > +
> >  	/* Make sure we are still connected */
> >  	if (cp->addr.type == BDADDR_BREDR)
> >  		conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> > @@ -7393,12 +7414,16 @@ static int get_conn_info_sync(struct hci_dev *hdev, void *data)
> >  	else
> >  		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
> >  
> > -	if (!conn || conn->state != BT_CONNECTED)
> > +	if (!conn || conn->state != BT_CONNECTED) {
> > +		hci_dev_unlock(hdev);
> >  		return MGMT_STATUS_NOT_CONNECTED;
> > +	}
> >  
> > -	cmd->user_data = conn;
> > +	cmd->user_data = hci_conn_get(conn);
> >  	handle = cpu_to_le16(conn->handle);
> >  
> > +	hci_dev_unlock(hdev);
> > +
> >  	/* Refresh RSSI each time */
> >  	err = hci_read_rssi_sync(hdev, handle);
> >  
> > @@ -7532,6 +7557,9 @@ static void get_clock_info_complete(struct hci_dev *hdev, void *data, int err)
> >  	}
> >  
> >  complete:
> > +	if (conn)
> > +		hci_conn_put(conn);
> > +
> >  	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, status, &rp,
> >  			  sizeof(rp));
> >  
> > @@ -7548,15 +7576,21 @@ static int get_clock_info_sync(struct hci_dev *hdev, void *data)
> >  	memset(&hci_cp, 0, sizeof(hci_cp));
> >  	hci_read_clock_sync(hdev, &hci_cp);
> >  
> > +	hci_dev_lock(hdev);
> > +
> >  	/* Make sure connection still exists */
> >  	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &cp->addr.bdaddr);
> > -	if (!conn || conn->state != BT_CONNECTED)
> > +	if (!conn || conn->state != BT_CONNECTED) {
> > +		hci_dev_unlock(hdev);
> >  		return MGMT_STATUS_NOT_CONNECTED;
> > +	}
> >  
> > -	cmd->user_data = conn;
> > +	cmd->user_data = hci_conn_get(conn);
> >  	hci_cp.handle = cpu_to_le16(conn->handle);
> >  	hci_cp.which = 0x01; /* Piconet clock */
> >  
> > +	hci_dev_unlock(hdev);
> > +
> >  	return hci_read_clock_sync(hdev, &hci_cp);
> >  }
> >  
> > -- 
> > 2.51.1

-- 
Pauli Virtanen