[PATCH] Bluetooth: MGMT: claim pending pairing command before completion

[PATCH] Bluetooth: MGMT: claim pending pairing command before completion

[Chengfeng Ye]

find_pairing() walks the pending command list without mgmt_pending_lock and
returns an unreferenced command.  Its callers then complete and remove the
command after the lookup.  cancel_pair_device() similarly drops the pending
lock after pending_find() and continues to use the returned command.

The HCI device lock does not close this gap because the SMP receive path
does not hold it.  The observed interleaving is:

  CPU 0 (cancel)                 CPU 1 (SMP failure)
  pending_find(cmd)
                                  find_pairing(cmd)
                                  cmd_complete(cmd)
                                  mgmt_pending_remove(cmd)
                                    kfree(cmd)
  cmd_complete(cmd)
  mgmt_pending_remove(cmd)

Thus CPU 0 dereferences and removes a command already freed by CPU 1.  The
same ownership race exists among the other pairing completion callbacks.

Find and unlink the matching pairing command while holding
mgmt_pending_lock, then complete and free the claimed command.  Use
mgmt_pending_valid() when pair_device() already has the exact command.
Keep cancellation limited to the first pending pairing command, as
before, so normal lookup and error behavior remain unchanged.

KASAN reported:

  BUG: KASAN: slab-use-after-free in mgmt_pending_remove+0x292/0x2f0
  Read of size 8 at addr ffff888100340498 by task poc/92

  Call Trace:
   mgmt_pending_remove+0x292/0x2f0
   cancel_pair_device+0x1d1/0x310
   hci_sock_sendmsg+0x1033/0x1ea0

  Allocated by task 86:
   mgmt_pending_new+0xb4/0x260
   mgmt_pending_add+0x1b/0x100
   pair_device+0x51d/0xcc0
   hci_sock_sendmsg+0x1033/0x1ea0

  Freed by task 87:
   kfree+0x131/0x3c0
   mgmt_pending_remove+0x20c/0x2f0
   mgmt_auth_failed+0x31a/0x4d0
   smp_recv_cb+0x4d6/0x8110
   l2cap_recv_frame+0xf14/0x9190
   l2cap_recv_acldata+0xa64/0xd40
   hci_rx_work+0x4ca/0x730

Fixes: f4a407bef20c ("Bluetooth: Wait for SMP key distribution completion when pairing")
Fixes: e1e930f591bf ("Bluetooth: Fix mgmt pairing failure when authentication fails")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/bluetooth/mgmt.c | 53 ++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1db10e0f617f..09cac79ce51b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3514,21 +3514,34 @@ static int set_io_capability(struct sock *sk, struct hci_dev *hdev, void *data,
 				 NULL, 0);
 }
 
-static struct mgmt_pending_cmd *find_pairing(struct hci_conn *conn)
+static struct mgmt_pending_cmd *claim_pairing(struct hci_dev *hdev,
+					      struct hci_conn *conn,
+					      const bdaddr_t *bdaddr)
 {
-	struct hci_dev *hdev = conn->hdev;
 	struct mgmt_pending_cmd *cmd;
+	struct hci_conn *cmd_conn;
+
+	mutex_lock(&hdev->mgmt_pending_lock);
 
 	list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
 		if (cmd->opcode != MGMT_OP_PAIR_DEVICE)
 			continue;
 
-		if (cmd->user_data != conn)
+		cmd_conn = cmd->user_data;
+
+		if (conn && cmd_conn != conn)
 			continue;
 
+		if (bdaddr && (!cmd_conn || bacmp(bdaddr, &cmd_conn->dst)))
+			break;
+
+		list_del(&cmd->list);
+		mutex_unlock(&hdev->mgmt_pending_lock);
 		return cmd;
 	}
 
+	mutex_unlock(&hdev->mgmt_pending_lock);
+
 	return NULL;
 }
 
@@ -3566,10 +3579,10 @@ void mgmt_smp_complete(struct hci_conn *conn, bool complete)
 	u8 status = complete ? MGMT_STATUS_SUCCESS : MGMT_STATUS_FAILED;
 	struct mgmt_pending_cmd *cmd;
 
-	cmd = find_pairing(conn);
+	cmd = claim_pairing(conn->hdev, conn, NULL);
 	if (cmd) {
 		cmd->cmd_complete(cmd, status);
-		mgmt_pending_remove(cmd);
+		mgmt_pending_free(cmd);
 	}
 }
 
@@ -3579,14 +3592,14 @@ static void pairing_complete_cb(struct hci_conn *conn, u8 status)
 
 	BT_DBG("status %u", status);
 
-	cmd = find_pairing(conn);
+	cmd = claim_pairing(conn->hdev, conn, NULL);
 	if (!cmd) {
 		BT_DBG("Unable to find a pending command");
 		return;
 	}
 
 	cmd->cmd_complete(cmd, mgmt_status(status));
-	mgmt_pending_remove(cmd);
+	mgmt_pending_free(cmd);
 }
 
 static void le_pairing_complete_cb(struct hci_conn *conn, u8 status)
@@ -3598,14 +3611,14 @@ static void le_pairing_complete_cb(struct hci_conn *conn, u8 status)
 	if (!status)
 		return;
 
-	cmd = find_pairing(conn);
+	cmd = claim_pairing(conn->hdev, conn, NULL);
 	if (!cmd) {
 		BT_DBG("Unable to find a pending command");
 		return;
 	}
 
 	cmd->cmd_complete(cmd, mgmt_status(status));
-	mgmt_pending_remove(cmd);
+	mgmt_pending_free(cmd);
 }
 
 static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
@@ -3729,12 +3742,16 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 	conn->io_capability = cp->io_cap;
+
+	mutex_lock(&hdev->mgmt_pending_lock);
 	cmd->user_data = hci_conn_get(conn);
+	mutex_unlock(&hdev->mgmt_pending_lock);
 
 	if ((conn->state == BT_CONNECTED || conn->state == BT_CONFIG) &&
-	    hci_conn_security(conn, sec_level, auth_type, true)) {
+	    hci_conn_security(conn, sec_level, auth_type, true) &&
+	    mgmt_pending_valid(hdev, cmd)) {
 		cmd->cmd_complete(cmd, 0);
-		mgmt_pending_remove(cmd);
+		mgmt_pending_free(cmd);
 	}
 
 	err = 0;
@@ -3762,7 +3779,7 @@ static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
-	cmd = pending_find(MGMT_OP_PAIR_DEVICE, hdev);
+	cmd = claim_pairing(hdev, NULL, &addr->bdaddr);
 	if (!cmd) {
 		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_CANCEL_PAIR_DEVICE,
 				      MGMT_STATUS_INVALID_PARAMS);
@@ -3771,14 +3788,8 @@ static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	conn = cmd->user_data;
 
-	if (bacmp(&addr->bdaddr, &conn->dst) != 0) {
-		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_CANCEL_PAIR_DEVICE,
-				      MGMT_STATUS_INVALID_PARAMS);
-		goto unlock;
-	}
-
 	cmd->cmd_complete(cmd, MGMT_STATUS_CANCELLED);
-	mgmt_pending_remove(cmd);
+	mgmt_pending_free(cmd);
 
 	err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_CANCEL_PAIR_DEVICE, 0,
 				addr, sizeof(*addr));
@@ -10137,14 +10148,14 @@ void mgmt_auth_failed(struct hci_conn *conn, u8 hci_status)
 	ev.addr.type = link_to_bdaddr(conn->type, conn->dst_type);
 	ev.status = status;
 
-	cmd = find_pairing(conn);
+	cmd = claim_pairing(conn->hdev, conn, NULL);
 
 	mgmt_event(MGMT_EV_AUTH_FAILED, conn->hdev, &ev, sizeof(ev),
 		    cmd ? cmd->sk : NULL);
 
 	if (cmd) {
 		cmd->cmd_complete(cmd, status);
-		mgmt_pending_remove(cmd);
+		mgmt_pending_free(cmd);
 	}
 }
 
-- 
2.43.0

RE: Bluetooth: MGMT: claim pending pairing command before completion

[bluez.test.bot]

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

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: net/bluetooth/mgmt.c:3514
error: net/bluetooth/mgmt.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth