[PATCH] Bluetooth: MGMT: fix mesh_tx leak on hci_cmd_sync_queue() failure

[PATCH] Bluetooth: MGMT: fix mesh_tx leak on hci_cmd_sync_queue() failure

[Hui Peng]

mesh_send() only queues mesh_send_sync() when no transmission is already
in progress:

	sending = hci_dev_test_flag(hdev, HCI_MESH_SENDING);
	mesh_tx = mgmt_mesh_add(sk, hdev, send, len);

	if (!mesh_tx)
		err = -ENOMEM;
	else if (!sending)
		err = hci_cmd_sync_queue(hdev, mesh_send_sync, mesh_tx,
					 mesh_send_start_complete);

so the only way to reach the error path with a live mesh_tx is for that
hci_cmd_sync_queue() call to fail, which can only happen when sending is
false. The cleanup, however, is guarded the other way round:

	if (mesh_tx) {
		if (sending)
			mgmt_mesh_remove(mesh_tx);
	}

With mesh_tx non-NULL the inner condition is therefore always false, and
mgmt_mesh_remove() is never reached: the entry stays on
hdev->mesh_pending even though the command has been failed back to
userspace with MGMT_STATUS_FAILED. It is only released when the socket
is closed or the controller goes away, and until then it counts against
the MESH_HANDLES_MAX budget enforced by send_count(), so repeated
failures eventually make MGMT_OP_MESH_SEND return MGMT_STATUS_BUSY.

Conversely, when sending is true err is left at 0 and the success branch
runs, so the guard never protects anything either.

Drop the inner condition and remove the entry whenever one was added.

Fixes: b338d91703fa ("Bluetooth: Implement support for Mesh")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
This is the remaining half of
https://lore.kernel.org/all/20260919112517.3871992-1-benquike@gmail.com/
("[PATCH v3] Bluetooth: MGMT: Fix mesh_tx Use-After-Free and leak in
mesh_send()"), which I am withdrawing.

That patch bundled this leak together with a fix for the mesh_tx
use-after-free, and was written against mainline, so it neither applied
to bluetooth-next nor was still needed: 71af682ba469 ("Bluetooth: mgmt:
Dequeue pending mesh_send_sync entries on cancel") already fixed the
use-after-free, and did it better than I had - dequeuing the pending
mesh_send_sync entry at the point of the free rather than locking
around the dereference. Only the leak was left, so this is just that,
rebased onto bluetooth-next.

Found by code inspection while investigating the use-after-free. I have
not reproduced the leak on its own; hci_cmd_sync_queue() only fails on
-ENOMEM or when the controller is going down, which I did not manage to
induce reliably. Compile tested only.

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 9d3de5a..8991aa8 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2543,10 +2543,8 @@ static int mesh_send(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 		err = mgmt_cmd_status(sk, hdev->id, MGMT_OP_MESH_SEND,
 				      MGMT_STATUS_FAILED);
 
-		if (mesh_tx) {
-			if (sending)
-				mgmt_mesh_remove(mesh_tx);
-		}
+		if (mesh_tx)
+			mgmt_mesh_remove(mesh_tx);
 	} else {
 		hci_dev_set_flag(hdev, HCI_MESH_SENDING);
 

RE: Bluetooth: MGMT: fix mesh_tx leak on hci_cmd_sync_queue() failure

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2736 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/series/1169247/

---Test result---

Test Summary:
CheckPatch                    PASS      0.70 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.11 seconds
GitLint                       FAIL      0.29 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      28.95 seconds
CheckAllWarning               PASS      32.94 seconds
CheckSparse                   PASS      35.66 seconds
BuildKernel32                 PASS      29.20 seconds
CheckKernelLLVM               PASS      33.21 seconds
TestRunnerSetup               PASS      782.58 seconds
TestRunner_mgmt-tester        FAIL      61.32 seconds
TestRunner_mesh-tester        FAIL      7.82 seconds
IncrementalBuild              PASS      29.25 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: MGMT: fix mesh_tx leak on hci_cmd_sync_queue() failure

6: B3 Line contains hard tab characters (\t): "	sending = hci_dev_test_flag(hdev, HCI_MESH_SENDING);"
7: B3 Line contains hard tab characters (\t): "	mesh_tx = mgmt_mesh_add(sk, hdev, send, len);"
9: B3 Line contains hard tab characters (\t): "	if (!mesh_tx)"
10: B3 Line contains hard tab characters (\t): "		err = -ENOMEM;"
11: B3 Line contains hard tab characters (\t): "	else if (!sending)"
12: B3 Line contains hard tab characters (\t): "		err = hci_cmd_sync_queue(hdev, mesh_send_sync, mesh_tx,"
13: B3 Line contains hard tab characters (\t): "					 mesh_send_start_complete);"
19: B3 Line contains hard tab characters (\t): "	if (mesh_tx) {"
20: B3 Line contains hard tab characters (\t): "		if (sending)"
21: B3 Line contains hard tab characters (\t): "			mgmt_mesh_remove(mesh_tx);"
22: B3 Line contains hard tab characters (\t): "	}"
##############################
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.022 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.403 seconds
Mesh - Send cancel - 2                               Timed out    1.999 seconds


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

---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: MGMT: fix mesh_tx leak on hci_cmd_sync_queue() failure

[Iaroslav Voitovych]

Hi Hui,

I reproduced the leak and confirmed that your patch fixes it in the
tested cases. I applied the patch as posted on top of bluetooth-next
036d4119079a and compared it against a control kernel built from the
same revision without the patch, in QEMU (BlueZ test-runner, emulated
controller, KASAN and lockdep), with legacy and extended advertising.

The leak is reachable without fault injection: with LE and the mesh
experimental feature enabled, a Mesh Send while the adapter is powered
off reaches hci_cmd_sync_queue(), which returns -ENETDOWN after the
kernel has assigned the request a handle. I also made that call
return -ENOMEM using failslab.

In both cases, without the patch the failed handle stays listed in
Read Mesh Features. When the next transmission ends (in the
powered-off case, after powering the adapter back on and sending
again), a Mesh Packet Complete is reported for the failed handle, and
the next request's packet is started on the controller twice.

The leaked entries also count against MESH_HANDLES_MAX. After three
consecutive failures, three handles stay outstanding and subsequent
Mesh Sends from that socket are answered Busy. In the powered-off
case, once the adapter is powered on, a send from another socket
causes the first leaked handle to be completed without ever being
started, and the other two failed requests to be started on the
controller, although their Mesh Sends had been answered Failed.

With the patch, in all tested scenarios no failed handle is listed,
completed or started, each accepted send is started on the controller
and completed once, and repeated failures leave no outstanding
handles. The powered-off reproducer gives the same control-vs-patched
result on a 4-CPU guest.

Separately, with both kernels rebuilt with CONFIG_DEBUG_KMEMLEAK,
kmemleak reports leaked allocations from mgmt_mesh_add() on the
unpatched build only, for -ENETDOWN, -ENOMEM and -ENODEV, when scanned
after the test program had closed its socket, removed the controller
and exited. The leaked entry keeps the MGMT socket referenced, so
closing the socket does not release it. It also stays leaked after
the controller is removed, although the commit message says either
should release it. The -ENODEV case needed a
test-only delay in mesh_send(), added to diagnostic builds of both
kernels, so that the controller could be removed while the Mesh Send
was in progress.

No KASAN, lockdep or WARNING reports. BlueZ's unmodified mgmt-tester
passes 503/503 on both kernels; mesh-tester gives 8/10 on both, with
the same two "Mesh - Send cancel" cases timing out.

Reproducer, kernel configs and logs:
https://github.com/ivoitovych/qca9377-bt-hang/tree/ccfc358e2cd866948aea3ec81bfabd5298e30618/retest/mesh-tx-leak

Tested-by: Iaroslav Voitovych <yaroslav.voytovych@gmail.com>