[PATCH bluetooth 0/4] Bluetooth: fix cmd_sync payload lifetimes on the cancel path

[PATCH bluetooth 0/4] Bluetooth: fix cmd_sync payload lifetimes on the cancel path

[Linmao Li]

The cmd_sync worker calls entry->destroy() after running a command, and
_hci_cmd_sync_cancel_entry() does the same when an entry is cancelled --
but only if a destroy callback was supplied. Without one it frees the
work entry and leaves entry->data unreleased.

These four call sites pass a heap payload with a NULL destroy callback
and free it inside the sync function instead, so each of them leaks when
the entry is cancelled rather than run. hci_cmd_sync_clear() cancels
every pending entry when the controller is unregistered.

Patch 1 additionally holds the connection, as the payload stores a bare
hci_conn pointer. Patch 2 additionally frees the payload when queueing
fails, which it currently does not check for. Patches 3 and 4 also leak
the socket reference taken by mgmt_pending_new().

Each patch moves the release into a destroy callback; apart from holding
the connection in patch 1, no behaviour changes.

Found by inspection while reading the recent cmd_sync lifetime fixes.
Not tested on hardware: these paths need an adapter removal racing a
queued command, or allocation failure, to reach.

Linmao Li (4):
  Bluetooth: hci_conn: fix the SCO setup context lifetime
  Bluetooth: hci_sync: free the advertising instance on the failure and
    cancel paths
  Bluetooth: MGMT: free the mesh send cancel command when it is
    cancelled
  Bluetooth: MGMT: free the HCI command when it is cancelled

 net/bluetooth/hci_conn.c | 20 +++++++++++++++-----
 net/bluetooth/hci_sync.c | 12 +++++++++---
 net/bluetooth/mgmt.c     | 22 +++++++++++++++-------
 3 files changed, 39 insertions(+), 15 deletions(-)


base-commit: abd93c85c8667add738ee82aeab95dd9fc8265a2
-- 
2.25.1

[PATCH bluetooth 1/4] Bluetooth: hci_conn: fix the SCO setup context lifetime

[Linmao Li]

hci_setup_sync() queues a conn_handle_t with a NULL destroy callback, so
the context is only freed if hci_enhanced_setup_sync() actually runs. An
entry that is cancelled instead is leaked, as
_hci_cmd_sync_cancel_entry() does not release entry->data when there is
no destroy callback, and hci_cmd_sync_clear() cancels every pending entry
when the controller is unregistered.

The context also stores a bare hci_conn pointer, so the connection can be
freed while the work is queued. The dequeue in hci_conn_del() does not
cover it either, as it matches on entry->data == conn and entry->data is
the wrapper here. Same problem as commit 2f5d635ad590 ("Bluetooth:
hci_sync: hold conn in hci_connect_acl/le_sync() callbacks").

Hold the connection and release both from a destroy callback. The
submission failure path drops both, since hci_cmd_sync_submit() does not
call the destroy callback when it fails to queue.

Fixes: e07a06b4eb41 ("Bluetooth: Convert SCO configure_datapath to hci_sync")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 net/bluetooth/hci_conn.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b1f911fd4ad6a..19b7629b1cc10 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -283,8 +283,6 @@ static int hci_enhanced_setup_sync(struct hci_dev *hdev, void *data)
 	struct hci_cp_enhanced_setup_sync_conn cp;
 	const struct sco_param *param;
 
-	kfree(conn_handle);
-
 	if (!hci_conn_valid(hdev, conn))
 		return -ECANCELED;
 
@@ -453,6 +451,15 @@ static bool hci_setup_sync_conn(struct hci_conn *conn, __u16 handle)
 	return true;
 }
 
+static void hci_enhanced_setup_sync_destroy(struct hci_dev *hdev, void *data,
+					    int err)
+{
+	struct conn_handle_t *conn_handle = data;
+
+	hci_conn_put(conn_handle->conn);
+	kfree(conn_handle);
+}
+
 bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 {
 	int result;
@@ -464,12 +471,15 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 		if (!conn_handle)
 			return false;
 
-		conn_handle->conn = conn;
+		conn_handle->conn = hci_conn_get(conn);
 		conn_handle->handle = handle;
 		result = hci_cmd_sync_queue(conn->hdev, hci_enhanced_setup_sync,
-					    conn_handle, NULL);
-		if (result < 0)
+					    conn_handle,
+					    hci_enhanced_setup_sync_destroy);
+		if (result < 0) {
+			hci_conn_put(conn);
 			kfree(conn_handle);
+		}
 
 		return result == 0;
 	}
-- 
2.25.1

RE: Bluetooth: fix cmd_sync payload lifetimes on the cancel path

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      2.13 seconds
VerifyFixes                   PASS      0.07 seconds
VerifySignedoff               PASS      0.07 seconds
GitLint                       FAIL      0.84 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      27.37 seconds
CheckAllWarning               PASS      30.02 seconds
CheckSparse                   PASS      28.60 seconds
BuildKernel32                 PASS      26.62 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      507.04 seconds
TestRunner_l2cap-tester       PASS      65.22 seconds
TestRunner_iso-tester         PASS      86.41 seconds
TestRunner_bnep-tester        PASS      19.23 seconds
TestRunner_mgmt-tester        FAIL      224.39 seconds
TestRunner_rfcomm-tester      PASS      25.86 seconds
TestRunner_sco-tester         PASS      32.39 seconds
TestRunner_ioctl-tester       PASS      27.30 seconds
TestRunner_mesh-tester        FAIL      26.87 seconds
TestRunner_smp-tester         PASS      23.99 seconds
TestRunner_userchan-tester    PASS      20.69 seconds
TestRunner_6lowpan-tester     PASS      23.36 seconds
IncrementalBuild              PASS      35.61 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[2/4] Bluetooth: hci_sync: free the advertising instance on the failure and cancel paths

1: T1 Title exceeds max length (88>80): "[2/4] Bluetooth: hci_sync: free the advertising instance on the failure and cancel paths"
##############################
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: 501, Passed: 496 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.257 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.387 seconds
Mesh - Send cancel - 2                               Timed out    1.985 seconds


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

---
Regards,
Linux Bluetooth

[PATCH bluetooth 2/4] Bluetooth: hci_sync: free the advertising instance on the failure and cancel paths

[Linmao Li]

adv_timeout_expire() hands a kmalloc()ed instance byte to
hci_cmd_sync_queue() with a NULL destroy callback, and only
adv_timeout_expire_sync() frees it. That leaks on two paths:

 - the return value is not checked, and hci_cmd_sync_queue() does not
   take ownership when it fails (-ENETDOWN, -ENODEV, -ENOMEM);

 - a cancelled entry is not released, as _hci_cmd_sync_cancel_entry()
   does not free entry->data when there is no destroy callback.
   hci_cmd_sync_clear() cancels every pending entry when the controller
   is unregistered.

Free the buffer from a destroy callback, and in the caller when the entry
could not be queued at all.

Fixes: c249ea9b4309 ("Bluetooth: Move Adv Instance timer to hci_sync")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 net/bluetooth/hci_sync.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index c8d14128c363d..d21b7c8877545 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -540,8 +540,6 @@ static int adv_timeout_expire_sync(struct hci_dev *hdev, void *data)
 {
 	u8 instance = *(u8 *)data;
 
-	kfree(data);
-
 	hci_clear_adv_instance_sync(hdev, NULL, instance, false);
 
 	if (list_empty(&hdev->adv_instances))
@@ -550,6 +548,12 @@ static int adv_timeout_expire_sync(struct hci_dev *hdev, void *data)
 	return 0;
 }
 
+static void adv_timeout_expire_destroy(struct hci_dev *hdev, void *data,
+				       int err)
+{
+	kfree(data);
+}
+
 static void adv_timeout_expire(struct work_struct *work)
 {
 	u8 *inst_ptr;
@@ -570,7 +574,9 @@ static void adv_timeout_expire(struct work_struct *work)
 		goto unlock;
 
 	*inst_ptr = hdev->cur_adv_instance;
-	hci_cmd_sync_queue(hdev, adv_timeout_expire_sync, inst_ptr, NULL);
+	if (hci_cmd_sync_queue(hdev, adv_timeout_expire_sync, inst_ptr,
+			       adv_timeout_expire_destroy) < 0)
+		kfree(inst_ptr);
 
 unlock:
 	hci_dev_unlock(hdev);
-- 
2.25.1

[PATCH bluetooth 3/4] Bluetooth: MGMT: free the mesh send cancel command when it is cancelled

[Linmao Li]

mesh_send_cancel() queues the pending command with a NULL destroy
callback, so it is only freed if send_cancel() runs. A cancelled entry is
leaked, as _hci_cmd_sync_cancel_entry() does not release entry->data when
there is no destroy callback, and hci_cmd_sync_clear() cancels every
pending entry when the controller is unregistered. Nothing else reclaims
it either: mgmt_pending_new() does not put the command on
hdev->mgmt_pending.

The leak also pins the socket reference taken by mgmt_pending_new(), so
the mgmt socket is never released.

Free the command from a destroy callback.

Fixes: b338d91703fa ("Bluetooth: Implement support for Mesh")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 net/bluetooth/mgmt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 167d75e345266..a80653b5b875d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2431,11 +2431,15 @@ static int send_cancel(struct hci_dev *hdev, void *data)
 
 	mgmt_cmd_complete(cmd->sk, hdev->id, MGMT_OP_MESH_SEND_CANCEL,
 			  0, NULL, 0);
-	mgmt_pending_free(cmd);
 
 	return 0;
 }
 
+static void send_cancel_destroy(struct hci_dev *hdev, void *data, int err)
+{
+	mgmt_pending_free(data);
+}
+
 static int mesh_send_cancel(struct sock *sk, struct hci_dev *hdev,
 			    void *data, u16 len)
 {
@@ -2456,7 +2460,8 @@ static int mesh_send_cancel(struct sock *sk, struct hci_dev *hdev,
 	if (!cmd)
 		err = -ENOMEM;
 	else
-		err = hci_cmd_sync_queue(hdev, send_cancel, cmd, NULL);
+		err = hci_cmd_sync_queue(hdev, send_cancel, cmd,
+					 send_cancel_destroy);
 
 	if (err < 0) {
 		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_MESH_SEND_CANCEL,
-- 
2.25.1

[PATCH bluetooth 4/4] Bluetooth: MGMT: free the HCI command when it is cancelled

[Linmao Li]

mgmt_hci_cmd_sync() queues the pending command with a NULL destroy
callback, so it is only freed if send_hci_cmd_sync() runs. A cancelled
entry is leaked, as _hci_cmd_sync_cancel_entry() does not release
entry->data when there is no destroy callback, and hci_cmd_sync_clear()
cancels every pending entry when the controller is unregistered. Nothing
else reclaims it either: mgmt_pending_new() does not put the command on
hdev->mgmt_pending.

The leak also pins the socket reference taken by mgmt_pending_new(), so
the mgmt socket is never released.

Free the command from a destroy callback. The now-empty done label is
replaced by a direct return.

Fixes: 827af4787e74 ("Bluetooth: MGMT: Add initial implementation of MGMT_OP_HCI_CMD_SYNC")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 net/bluetooth/mgmt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a80653b5b875d..7e9d27eefb504 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2647,7 +2647,7 @@ static int send_hci_cmd_sync(struct hci_dev *hdev, void *data)
 	if (IS_ERR(skb)) {
 		mgmt_cmd_status(cmd->sk, hdev->id, MGMT_OP_HCI_CMD_SYNC,
 				mgmt_status(PTR_ERR(skb)));
-		goto done;
+		return 0;
 	}
 
 	mgmt_cmd_complete(cmd->sk, hdev->id, MGMT_OP_HCI_CMD_SYNC, 0,
@@ -2655,12 +2655,14 @@ static int send_hci_cmd_sync(struct hci_dev *hdev, void *data)
 
 	kfree_skb(skb);
 
-done:
-	mgmt_pending_free(cmd);
-
 	return 0;
 }
 
+static void send_hci_cmd_sync_destroy(struct hci_dev *hdev, void *data, int err)
+{
+	mgmt_pending_free(data);
+}
+
 static int mgmt_hci_cmd_sync(struct sock *sk, struct hci_dev *hdev,
 			     void *data, u16 len)
 {
@@ -2678,7 +2680,8 @@ static int mgmt_hci_cmd_sync(struct sock *sk, struct hci_dev *hdev,
 	if (!cmd)
 		err = -ENOMEM;
 	else
-		err = hci_cmd_sync_queue(hdev, send_hci_cmd_sync, cmd, NULL);
+		err = hci_cmd_sync_queue(hdev, send_hci_cmd_sync, cmd,
+					 send_hci_cmd_sync_destroy);
 
 	if (err < 0) {
 		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_HCI_CMD_SYNC,
-- 
2.25.1

Re: [PATCH bluetooth 0/4] Bluetooth: fix cmd_sync payload lifetimes on the cancel path

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu,  6 Aug 2026 20:59:53 +0800 you wrote:
> The cmd_sync worker calls entry->destroy() after running a command, and
> _hci_cmd_sync_cancel_entry() does the same when an entry is cancelled --
> but only if a destroy callback was supplied. Without one it frees the
> work entry and leaves entry->data unreleased.
> 
> These four call sites pass a heap payload with a NULL destroy callback
> and free it inside the sync function instead, so each of them leaks when
> the entry is cancelled rather than run. hci_cmd_sync_clear() cancels
> every pending entry when the controller is unregistered.
> 
> [...]

Here is the summary with links:
  - [1/4] Bluetooth: hci_conn: fix the SCO setup context lifetime
    https://git.kernel.org/bluetooth/bluetooth-next/c/7645e748704a
  - [2/4] Bluetooth: hci_sync: free the advertising instance on the failure and cancel paths
    https://git.kernel.org/bluetooth/bluetooth-next/c/19aa86a8d171
  - [3/4] Bluetooth: MGMT: free the mesh send cancel command when it is cancelled
    https://git.kernel.org/bluetooth/bluetooth-next/c/ac0965fb4b86
  - [4/4] Bluetooth: MGMT: free the HCI command when it is cancelled
    https://git.kernel.org/bluetooth/bluetooth-next/c/f2afd8eadf5d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html