linux-kernel - Re: [PATCH v2 3/8] Bluetooth: mgmt: take lock and hold reference when handling hci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20251102232137.8830-1-hdanton@sina.com>
Date: Mon,  3 Nov 2025 07:21:35 +0800
From: Hillf Danton <hdanton@...a.com>
To: Pauli Virtanen <pav@....fi>
Cc: marcel@...tmann.org,
	johan.hedberg@...il.com,
	luiz.dentz@...il.com,
	linux-bluetooth@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/8] Bluetooth: mgmt: take lock and hold reference when handling hci_conn

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@....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