[PATCH v2] Bluetooth: MGMT: validate LOAD_CONN_PARAM entry before update

[PATCH v2] Bluetooth: MGMT: validate LOAD_CONN_PARAM entry before update

[Cen Zhang]

MGMT_OP_LOAD_CONN_PARAM queues conn_update_sync() when a single parameter
update changes an existing LE central connection. The queued work currently
stores the hci_conn_params object from hdev->le_conn_params. A later
LOAD_CONN_PARAM request can clear disabled parameters and free that object
before hci_cmd_sync_work() runs the queued callback.

Do not keep that borrowed pointer in the queued work. Queue only the
address key. conn_update_sync() can then look up the current params entry
while holding hdev->lock. If userspace removed that entry while the work
was pending, cancel the queued update.

If the entry is still present, copy the current params and connection
handle under the lock. Then issue LE Connection Update after dropping the
lock.

Validation reproduced this kernel report:
BUG: KASAN: slab-use-after-free in conn_update_sync+0x2a/0xf0 [bluetooth]
Read of size 1 at addr ffff88810c697126 by task kworker/u17:0/377
Workqueue: hci0 hci_cmd_sync_work [bluetooth]

Call Trace:
 <TASK>
 dump_stack_lvl+0x66/0xa0
 print_report+0xce/0x5f0
 ? conn_update_sync+0x2a/0xf0 [bluetooth]
 ? __virt_addr_valid+0x19f/0x330
 ? conn_update_sync+0x2a/0xf0 [bluetooth]
 kasan_report+0xe0/0x110
 ? conn_update_sync+0x2a/0xf0 [bluetooth]
 ? __pfx_conn_update_sync+0x10/0x10 [bluetooth]
 conn_update_sync+0x2a/0xf0 [bluetooth]
 hci_cmd_sync_work+0x187/0x210 [bluetooth]
 process_one_work+0x4fd/0xbc0
 worker_thread+0x2d8/0x570
 kthread+0x1ad/0x1f0
 ret_from_fork+0x3c9/0x540
 ret_from_fork_asm+0x1a/0x30

Allocated by task 466:
 kasan_save_stack+0x33/0x60
 kasan_save_track+0x17/0x60
 __kasan_kmalloc+0xaa/0xb0
 hci_conn_params_add+0xa6/0x240 [bluetooth]
 load_conn_param+0x4e1/0x850 [bluetooth]
 hci_sock_sendmsg+0x96b/0xf80 [bluetooth]
 sock_write_iter+0x28e/0x2a0
 vfs_write+0x6e4/0x810
 ksys_write+0x147/0x170
 do_syscall_64+0x115/0x6a0
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Freed by task 474:
 kasan_save_stack+0x33/0x60
 kasan_save_track+0x17/0x60
 kasan_save_free_info+0x3b/0x60
 __kasan_slab_free+0x5f/0x80
 kfree+0x313/0x590
 hci_conn_params_clear_disabled+0x9b/0xc0 [bluetooth]
 load_conn_param+0x4bf/0x850 [bluetooth]
 hci_sock_sendmsg+0x96b/0xf80 [bluetooth]
 sock_write_iter+0x28e/0x2a0
 vfs_write+0x6e4/0x810
 ksys_write+0x147/0x170
 do_syscall_64+0x115/0x6a0
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fixes: 0ece498c27d8c ("Bluetooth: MGMT: Make MGMT_OP_LOAD_CONN_PARAM update existing connection")
Suggested-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Assisted-by: Codex:gpt-5.5
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
v2:
- Re-check hci_conn_params in conn_update_sync() and skip the queued
  update if the entry was removed while the work was pending, as
  suggested by Luiz.
- Queue only the address key and copy the current params and connection
  handle under hdev->lock before issuing the HCI command.

 net/bluetooth/mgmt.c | 62 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 733a4b70e10c..6fbabf437786 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7935,16 +7935,50 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 	return err;
 }
 
+struct conn_update_sync_data {
+	bdaddr_t addr;
+	u8 addr_type;
+};
+
 static int conn_update_sync(struct hci_dev *hdev, void *data)
 {
-	struct hci_conn_params *params = data;
+	struct conn_update_sync_data *update = data;
+	struct hci_conn_params *params;
+	struct hci_cp_le_conn_update cp;
 	struct hci_conn *conn;
 
-	conn = hci_conn_hash_lookup_le(hdev, &params->addr, params->addr_type);
-	if (!conn)
-		return -ECANCELED;
+	hci_dev_lock(hdev);
+
+	params = hci_conn_params_lookup(hdev, &update->addr, update->addr_type);
+	if (!params)
+		goto cancel;
+
+	conn = hci_conn_hash_lookup_le(hdev, &update->addr, update->addr_type);
+	if (!conn || conn->role != HCI_ROLE_MASTER)
+		goto cancel;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.handle		= cpu_to_le16(conn->handle);
+	cp.conn_interval_min	= cpu_to_le16(params->conn_min_interval);
+	cp.conn_interval_max	= cpu_to_le16(params->conn_max_interval);
+	cp.conn_latency		= cpu_to_le16(params->conn_latency);
+	cp.supervision_timeout	= cpu_to_le16(params->supervision_timeout);
+	cp.min_ce_len		= cpu_to_le16(0x0000);
+	cp.max_ce_len		= cpu_to_le16(0x0000);
 
-	return hci_le_conn_update_sync(hdev, conn, params);
+	hci_dev_unlock(hdev);
+
+	return __hci_cmd_sync_status(hdev, HCI_OP_LE_CONN_UPDATE,
+				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
+
+cancel:
+	hci_dev_unlock(hdev);
+	return -ECANCELED;
+}
+
+static void conn_update_sync_destroy(struct hci_dev *hdev, void *data, int err)
+{
+	kfree(data);
 }
 
 static int load_conn_param(struct sock *sk, struct hci_dev *hdev, void *data,
@@ -8054,9 +8088,21 @@ static int load_conn_param(struct sock *sk, struct hci_dev *hdev, void *data,
 			    (conn->le_conn_min_interval != min ||
 			     conn->le_conn_max_interval != max ||
 			     conn->le_conn_latency != latency ||
-			     conn->le_supv_timeout != timeout))
-				hci_cmd_sync_queue(hdev, conn_update_sync,
-						   hci_param, NULL);
+			     conn->le_supv_timeout != timeout)) {
+				struct conn_update_sync_data *update;
+
+				update = kzalloc_obj(*update);
+				if (!update)
+					continue;
+
+				bacpy(&update->addr, &hci_param->addr);
+				update->addr_type = hci_param->addr_type;
+
+				if (hci_cmd_sync_queue(hdev, conn_update_sync,
+						       update,
+						       conn_update_sync_destroy) < 0)
+					kfree(update);
+			}
 		}
 	}
 
-- 
2.43.0

RE: [v2] Bluetooth: MGMT: validate LOAD_CONN_PARAM entry before update

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1903 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=1121293

---Test result---

Test Summary:
CheckPatch                    PASS      0.82 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.17 seconds
GitLint                       PASS      0.34 seconds
SubjectPrefix                 PASS      0.16 seconds
BuildKernel                   PASS      27.08 seconds
CheckAllWarning               PASS      29.49 seconds
CheckSparse                   PASS      28.61 seconds
BuildKernel32                 PASS      26.33 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      494.89 seconds
TestRunner_mgmt-tester        FAIL      208.00 seconds
TestRunner_mesh-tester        FAIL      27.05 seconds
IncrementalBuild              PASS      26.66 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.245 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.731 seconds
Mesh - Send cancel - 2                               Timed out    1.987 seconds


https://github.com/bluez/bluetooth-next/pull/394

---
Regards,
Linux Bluetooth