[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

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

[Luiz Augusto von Dentz]

Hi Cen,

On Fri, Jul 3, 2026 at 1:25 PM Cen Zhang <zzzccc427@gmail.com> wrote:
>
> 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);

Don't really like where this is going, we shouldn't need to open code
hci_le_conn_update_sync, perhaps we should use the likes RCU to
prevent removing concurrently otherwise this sounds more like a
workaround and there might be other user of hci_conn_params_lookup
that are not protected from concurrent deletion.

> -       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);

It would probably be a lot simpler to just pass the connection here
rather than allocating another object just to carry the address and
address type.

> +                       }
>                 }
>         }
>
> --
> 2.43.0



-- 
Luiz Augusto von Dentz

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

[Cen Zhang]

Hi Luiz,

Thanks for the review and suggestions.

> Don't really like where this is going, we shouldn't need to open code
> hci_le_conn_update_sync

Agreed. In v3 I kept using hci_le_conn_update_sync() instead of
open-coding the HCI command in mgmt.c.

> It would probably be a lot simpler to just pass the connection here
> rather than allocating another object just to carry the address and
> address type.

I reworked the queued data that way as well. The queue side now passes
the hci_conn directly and holds it with hci_conn_get(). The destroy
callback drops that reference with hci_conn_put().

conn_update_sync() now revalidates the connection with hci_conn_valid(),
looks up the current hci_conn_params under hdev->lock, copies the four
interval fields to a stack-local params object, drops the lock, and then
calls hci_le_conn_update_sync().

Regarding RCU: I agree there is a wider question around protecting
hci_conn_params_lookup() users. I left that out of v3 to keep this fix
focused, since this queued-work bug cannot be fixed by holding an RCU
read-side section across the work lifetime. I can look into that as a
separate follow-up if useful.

Best regards
Cen Zhang