[PATCH v2 0/3] Bluetooth: MGMT: fix use-after-free of struct mgmt_mesh_tx

[PATCH v2 0/3] Bluetooth: MGMT: fix use-after-free of struct mgmt_mesh_tx

[Baul Lee]

hdev->mesh_pending is extended and walked from syscall context under
hdev->lock, and unlinked and freed from the hci_cmd_sync worker under
hci_req_sync_lock, so the list has no protection; the objects on it are
also handed to hci_cmd_sync_queue() as raw pointers, and those work
entries outlive the list.  Three use-after-frees follow, all reproduced
under KASAN.

Patch 1 removes mgmt_cleanup(), the one walker of the list that cannot
take hdev->lock.  Patch 2 puts the list under hdev->lock on the worker
side and asserts it in the helpers.  Patch 3 gives the object a
reference count for the work entry.  The order matters: the assertions
in patch 2 have no violating caller left once patch 1 is in, and the
reference in patch 3 is only well defined once every unlink happens
under one lock.

Reaching any of this needs CAP_NET_ADMIN in the init user namespace.
hci_sock.c sets HCI_SOCK_TRUSTED at bind under capable() rather than
ns_capable() and gates every later mgmt command on it, and the mesh
commands additionally need HCI_MESH_EXPERIMENTAL.  Nothing in
hci_event.c touches hdev->mesh_pending.

Each reproducer was run at identical parameters on the unpatched and the
patched kernel, built with KASAN, PROVE_LOCKING, DEBUG_LIST and
DEBUG_ATOMIC_SLEEP: every splat the unpatched kernel produces is gone.

v2: rebased onto bluetooth-next, where send_cancel() no longer ends in
    mgmt_pending_free(), so v1 did not apply for the CI.  Context only,
    no other change; v1 applies as posted to the bluetooth tree, which
    still has that line.

Baul Lee (3):
  Bluetooth: MGMT: remove the mesh walk from the socket destructor
  Bluetooth: MGMT: protect hdev->mesh_pending with hdev->lock
  Bluetooth: MGMT: reference-count struct mgmt_mesh_tx

 include/net/bluetooth/bluetooth.h |  1 -
 net/bluetooth/hci_sock.c          |  1 -
 net/bluetooth/mgmt.c              | 72 ++++++++++++++++++++++++---------------
 net/bluetooth/mgmt_util.c         | 31 +++++++++++++++--
 net/bluetooth/mgmt_util.h         |  3 ++
 5 files changed, 76 insertions(+), 32 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH v2 1/3] Bluetooth: MGMT: remove the mesh walk from the socket destructor

[Baul Lee]

hci_sock_destruct() calls mgmt_cleanup(), which walks hdev->mesh_pending
on every registered controller looking for entries owned by the socket
being destroyed, and completes the ones it finds.

It can never find one.  mgmt_mesh_add() takes a reference on the owning
socket for every entry it links onto the list and mgmt_mesh_remove()
drops it again, so the socket's reference count cannot reach zero while
one of its entries is there.

The walk still races the list.  mgmt_mesh_next() loads mesh_tx->sk from
every node it passes, including nodes owned by other sockets, while the
cmd_sync worker unlinks and frees nodes of the same list under a
different lock.  mgmt_cleanup() cannot take hdev->lock: it holds
read_lock(&hci_dev_list_lock) across the walk, and hdev->lock sleeps.
It is the one user of hdev->mesh_pending that cannot be brought under
that lock.

Remove mgmt_cleanup() and its caller.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>

Fixes: b338d91703fa ("Bluetooth: Implement support for Mesh")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 include/net/bluetooth/bluetooth.h |  1 -
 net/bluetooth/hci_sock.c          |  1 -
 net/bluetooth/mgmt.c              | 19 -------------------
 3 files changed, 21 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index b624da5026f5..f49cec5f01e6 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -675,7 +675,6 @@ static inline bool iso_inited(void)
 
 int mgmt_init(void);
 void mgmt_exit(void);
-void mgmt_cleanup(struct sock *sk);
 
 void bt_sock_reclassify_lock(struct sock *sk, int proto);
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 070ca388f9ac..5073f4fc3289 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -2164,7 +2164,6 @@ static int hci_sock_getsockopt(struct socket *sock, int level, int optname,
 
 static void hci_sock_destruct(struct sock *sk)
 {
-	mgmt_cleanup(sk);
 	skb_queue_purge(&sk->sk_receive_queue);
 	skb_queue_purge(&sk->sk_write_queue);
 	skb_queue_purge(&sk->sk_error_queue);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 860c086011b7..97408904f74b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -10894,22 +10894,3 @@ void mgmt_exit(void)
 {
 	hci_mgmt_chan_unregister(&chan);
 }
-
-void mgmt_cleanup(struct sock *sk)
-{
-	struct mgmt_mesh_tx *mesh_tx;
-	struct hci_dev *hdev;
-
-	read_lock(&hci_dev_list_lock);
-
-	list_for_each_entry(hdev, &hci_dev_list, list) {
-		do {
-			mesh_tx = mgmt_mesh_next(hdev, sk);
-
-			if (mesh_tx)
-				mesh_send_complete(hdev, mesh_tx, true);
-		} while (mesh_tx);
-	}
-
-	read_unlock(&hci_dev_list_lock);
-}
-- 
2.50.1 (Apple Git-155)

RE: Bluetooth: MGMT: fix use-after-free of struct mgmt_mesh_tx

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      2.75 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.11 seconds
GitLint                       FAIL      0.79 seconds
SubjectPrefix                 PASS      0.29 seconds
BuildKernel                   PASS      24.94 seconds
CheckAllWarning               PASS      26.93 seconds
CheckSparse                   PASS      25.80 seconds
BuildKernel32                 PASS      23.55 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      448.36 seconds
TestRunner_l2cap-tester       PASS      62.39 seconds
TestRunner_iso-tester         PASS      79.18 seconds
TestRunner_bnep-tester        PASS      18.40 seconds
TestRunner_mgmt-tester        FAIL      220.92 seconds
TestRunner_rfcomm-tester      PASS      30.69 seconds
TestRunner_sco-tester         PASS      31.03 seconds
TestRunner_ioctl-tester       PASS      26.35 seconds
TestRunner_mesh-tester        FAIL      25.85 seconds
TestRunner_smp-tester         PASS      23.28 seconds
TestRunner_userchan-tester    PASS      19.48 seconds
TestRunner_6lowpan-tester     PASS      22.70 seconds
IncrementalBuild              PASS      30.01 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2,2/3] Bluetooth: MGMT: protect hdev->mesh_pending with hdev->lock

55: B1 Line exceeds max length (97>80): "[   78.211873] ffff00001b0340b8 (&hdev->lock){+.+.}-{4:4}, at: mesh_send_start_complete+0x88/0xf4"
57: B1 Line exceeds max length (105>80): "[   78.212596] ffff00001b0347c0 (&hdev->cmd_sync_work_lock){+.+.}-{4:4}, at: hci_cmd_sync_clear+0x60/0xd4"
##############################
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.253 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.644 seconds
Mesh - Send cancel - 2                               Timed out    1.984 seconds


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

---
Regards,
Linux Bluetooth

[PATCH v2 2/3] Bluetooth: MGMT: protect hdev->mesh_pending with hdev->lock

[Baul Lee]

hdev->mesh_pending carries the queued struct mgmt_mesh_tx of the mesh
mgmt interface.  It is extended and walked from syscall context under
hci_dev_lock(hdev), in mgmt_mesh_add() and mgmt_mesh_foreach(), and it
is unlinked and freed from the hci_cmd_sync worker under
hci_req_sync_lock(hdev), in send_cancel(), mesh_send_done_sync() and
mesh_next().  The two sides hold different mutexes, so the list has no
protection: MGMT_OP_MESH_READ_FEATURES on one socket can be mid-walk
while MGMT_OP_MESH_SEND_CANCEL on another frees the entry it is
standing on.

[   47.418949] BUG: KASAN: slab-use-after-free in send_count+0x4c/0x6c
[   47.419029] Read of size 1 at addr ffff000015943228 by task c2_race/191
[   47.419546]  send_count+0x4c/0x6c
[   47.419591]  mgmt_mesh_foreach+0x68/0x100
[   47.419637]  mesh_features+0x180/0x184
[   47.421345] Freed by task 59:
[   47.421555]  mgmt_mesh_remove+0x94/0x110
[   47.421589]  send_cancel+0xd8/0x1cc
[   47.421627]  hci_cmd_sync_work+0xac/0x128

Offset 40 of the freed 96-byte object is mesh_tx->handle, the byte
send_count() puts in the MESH_READ_FEATURES reply.  The walk also
follows the LIST_POISON1 left by the concurrent list_del(), which oopses
with hci_dev_lock held and never released, so every later mgmt command
on that controller blocks.

The same missing lock lets the worker free the object under mesh_send(),
which reads mesh_tx->handle into the command complete after queueing:

[   41.093914] BUG: KASAN: slab-use-after-free in mgmt_cmd_complete+0xd0/0x210
[   41.094353] Read of size 1 at addr ffff00000a0dea28 by task c1_poc/162
[   41.096944]  mgmt_mesh_remove+0x94/0x110
[   41.096975]  mesh_send_start_complete+0x128/0x160

That read already sits inside mesh_send()'s hci_dev_lock() section; it
is the worker side that is missing the lock.

Take hci_dev_lock(hdev) on the worker side, around the list work only
and not around hci_disable_advertising_sync(), and assert it in the
helpers that walk or extend the list.  hci_req_sync_lock -> hdev->lock
is the order this subsystem already uses: hci_cmd_sync_work() calls
entry->func under req_lock, and callbacks such as hci_update_eir_sync()
take hci_dev_lock() inside it.

mesh_next() and mesh_send_start_complete() are destroy callbacks, and
hci_cmd_sync_clear() runs a destroy callback under cmd_sync_work_lock,
which every mgmt caller of hci_cmd_sync_queue() takes with hdev->lock
already held.  Both therefore return before hci_dev_lock() when err is
-ECANCELED, as the other destroy callbacks in this file do.  Without
those returns lockdep reports a circular dependency, reached by closing
/dev/vhci while a mesh entry is queued behind a parked req_workqueue:

[   78.211873] ffff00001b0340b8 (&hdev->lock){+.+.}-{4:4}, at: mesh_send_start_complete+0x88/0xf4
               but task is already holding lock:
[   78.212596] ffff00001b0347c0 (&hdev->cmd_sync_work_lock){+.+.}-{4:4}, at: hci_cmd_sync_clear+0x60/0xd4
               -> #1 (&hdev->cmd_sync_work_lock){+.+.}-{4:4}:
[   78.214268]        hci_update_passive_scan+0x6c/0x84
[   78.214455]        mgmt_set_powered_complete+0x1b0/0x1f8

The other leg of the cycle, mgmt_set_powered_complete() calling
hci_update_passive_scan() with hdev->lock held, is an existing in-tree
path.  The entry such a return leaves on hdev->mesh_pending stays there,
as entries already do across an unregister.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>

Fixes: b338d91703fa ("Bluetooth: Implement support for Mesh")
Fixes: 3bb88524b7d0 ("Bluetooth: MGMT: iterate over mesh commands in mgmt_mesh_foreach()")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 net/bluetooth/mgmt.c      | 30 ++++++++++++++++++++++++++++--
 net/bluetooth/mgmt_util.c |  8 ++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 97408904f74b..853a80fd15af 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1087,6 +1087,8 @@ static void mesh_send_complete(struct hci_dev *hdev,
 {
 	u8 handle = mesh_tx->handle;
 
+	lockdep_assert_held(&hdev->lock);
+
 	if (!silent)
 		mgmt_event(MGMT_EV_MESH_PACKET_CMPLT, hdev, &handle,
 			   sizeof(handle), NULL);
@@ -1101,11 +1103,16 @@ static int mesh_send_done_sync(struct hci_dev *hdev, void *data)
 	hci_dev_clear_flag(hdev, HCI_MESH_SENDING);
 	if (list_empty(&hdev->adv_instances))
 		hci_disable_advertising_sync(hdev);
+
+	hci_dev_lock(hdev);
+
 	mesh_tx = mgmt_mesh_next(hdev, NULL);
 
 	if (mesh_tx)
 		mesh_send_complete(hdev, mesh_tx, false);
 
+	hci_dev_unlock(hdev);
+
 	return 0;
 }
 
@@ -1113,11 +1120,19 @@ static int mesh_send_sync(struct hci_dev *hdev, void *data);
 static void mesh_send_start_complete(struct hci_dev *hdev, void *data, int err);
 static void mesh_next(struct hci_dev *hdev, void *data, int err)
 {
-	struct mgmt_mesh_tx *mesh_tx = mgmt_mesh_next(hdev, NULL);
+	struct mgmt_mesh_tx *mesh_tx;
 
-	if (!mesh_tx)
+	if (err == -ECANCELED)
 		return;
 
+	hci_dev_lock(hdev);
+
+	mesh_tx = mgmt_mesh_next(hdev, NULL);
+	if (!mesh_tx) {
+		hci_dev_unlock(hdev);
+		return;
+	}
+
 	err = hci_cmd_sync_queue(hdev, mesh_send_sync, mesh_tx,
 				 mesh_send_start_complete);
 
@@ -1125,6 +1140,8 @@ static void mesh_next(struct hci_dev *hdev, void *data, int err)
 		mesh_send_complete(hdev, mesh_tx, false);
 	else
 		hci_dev_set_flag(hdev, HCI_MESH_SENDING);
+
+	hci_dev_unlock(hdev);
 }
 
 static void mesh_send_done(struct work_struct *work)
@@ -2310,12 +2327,17 @@ static void mesh_send_start_complete(struct hci_dev *hdev, void *data, int err)
 	unsigned long mesh_send_interval;
 	u8 mgmt_err = mgmt_status(err);
 
+	if (err == -ECANCELED)
+		return;
+
 	/* Report any errors here, but don't report completion */
 
 	if (mgmt_err) {
 		hci_dev_clear_flag(hdev, HCI_MESH_SENDING);
 		/* Send Complete Error Code for handle */
+		hci_dev_lock(hdev);
 		mesh_send_complete(hdev, mesh_tx, false);
+		hci_dev_unlock(hdev);
 		return;
 	}
 
@@ -2421,6 +2443,8 @@ static int send_cancel(struct hci_dev *hdev, void *data)
 	struct mgmt_cp_mesh_send_cancel *cancel = (void *)cmd->param;
 	struct mgmt_mesh_tx *mesh_tx;
 
+	hci_dev_lock(hdev);
+
 	if (!cancel->handle) {
 		do {
 			mesh_tx = mgmt_mesh_next(hdev, cmd->sk);
@@ -2435,6 +2459,8 @@ static int send_cancel(struct hci_dev *hdev, void *data)
 			mesh_send_complete(hdev, mesh_tx, false);
 	}
 
+	hci_dev_unlock(hdev);
+
 	mgmt_cmd_complete(cmd->sk, hdev->id, MGMT_OP_MESH_SEND_CANCEL,
 			  0, NULL, 0);
 
diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c
index 6ea107c0e054..a822091f2907 100644
--- a/net/bluetooth/mgmt_util.c
+++ b/net/bluetooth/mgmt_util.c
@@ -369,6 +369,8 @@ void mgmt_mesh_foreach(struct hci_dev *hdev,
 {
 	struct mgmt_mesh_tx *mesh_tx, *tmp;
 
+	lockdep_assert_held(&hdev->lock);
+
 	list_for_each_entry_safe(mesh_tx, tmp, &hdev->mesh_pending, list) {
 		if (!sk || mesh_tx->sk == sk)
 			cb(mesh_tx, data);
@@ -379,6 +381,8 @@ struct mgmt_mesh_tx *mgmt_mesh_next(struct hci_dev *hdev, struct sock *sk)
 {
 	struct mgmt_mesh_tx *mesh_tx;
 
+	lockdep_assert_held(&hdev->lock);
+
 	if (list_empty(&hdev->mesh_pending))
 		return NULL;
 
@@ -394,6 +398,8 @@ struct mgmt_mesh_tx *mgmt_mesh_find(struct hci_dev *hdev, u8 handle)
 {
 	struct mgmt_mesh_tx *mesh_tx;
 
+	lockdep_assert_held(&hdev->lock);
+
 	if (list_empty(&hdev->mesh_pending))
 		return NULL;
 
@@ -410,6 +416,8 @@ struct mgmt_mesh_tx *mgmt_mesh_add(struct sock *sk, struct hci_dev *hdev,
 {
 	struct mgmt_mesh_tx *mesh_tx;
 
+	lockdep_assert_held(&hdev->lock);
+
 	mesh_tx = kzalloc_obj(*mesh_tx);
 	if (!mesh_tx)
 		return NULL;
-- 
2.50.1 (Apple Git-155)

[PATCH v2 3/3] Bluetooth: MGMT: reference-count struct mgmt_mesh_tx

[Baul Lee]

hci_cmd_sync_submit() stores the caller's pointer in a work entry and
takes no reference to what it names.  hci_cmd_sync_work() drains that
list from the head on an ordered workqueue, so an entry queued earlier
runs to completion before a later one is looked at.

mesh_send() links a struct mgmt_mesh_tx onto hdev->mesh_pending and
queues mesh_send_sync() with the raw pointer.  A MESH_SEND_CANCEL issued
before that send leaves its send_cancel() entry ahead in the queue, so
send_cancel() runs first: it picks the object out of hdev->mesh_pending
with mgmt_mesh_next(), which returns it without unlinking it, and frees
it through mesh_send_complete().  Nothing invalidates the pointer the
later entry still holds, so mesh_send_sync() and its destroy callback
mesh_send_start_complete() run on freed memory:

[   43.618774] BUG: KASAN: slab-use-after-free in mesh_send_sync+0xec/0x1b4
[   43.618851] Write of size 1 at addr ffff000009f2a6a9 by task kworker/u5:2/148
[   43.619464]  mesh_send_sync+0xec/0x1b4
[   43.619531]  hci_cmd_sync_work+0xac/0x128
[   43.620882] Freed by task 148:
[   43.621171]  mgmt_mesh_remove+0x94/0x110
[   43.621218]  send_cancel+0xd8/0x1cc
[   43.621270]  hci_cmd_sync_work+0xac/0x128

Offset 41 is mesh_tx->instance, the only byte mesh_send_sync() stores
through mesh_tx; it reads further fields of the same freed object, and
hci_set_adv_instance_data() copies 31 of those bytes into a live
struct adv_info.  The free and the use are consecutive iterations of one
hci_cmd_sync_work() loop on one kworker, so queue order alone decides
it.

Give struct mgmt_mesh_tx a reference count.  hdev->mesh_pending holds
one, and every pointer handed to hci_cmd_sync_queue() takes a second one
that the destroy callback drops, so a cancel that unlinks the object
while a work entry is still queued no longer releases it.
mgmt_mesh_remove() becomes an unlink plus a put and returns early when
the object is already unlinked, because it can now outlive its removal
from the list.  The list reference is still dropped under hci_dev_lock()
only, so mesh_send()'s use of mesh_tx->handle after queueing stays
covered by the lock it holds.

A cancel that arrives before the queued mesh_send_sync() now lets that
send run rather than freeing the object under it; suppressing the
transmission as well is a separate change.

hci_cmd_sync_dequeue() is the other in-tree option, but
mgmt_mesh_remove() is not given the hci_dev it needs and cannot tell
whether the work entry has already been taken off cmd_sync_work_list.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>

Fixes: b338d91703fa ("Bluetooth: Implement support for Mesh")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 net/bluetooth/mgmt.c      | 25 +++++++++++++++++--------
 net/bluetooth/mgmt_util.c | 23 +++++++++++++++++++++--
 net/bluetooth/mgmt_util.h |  3 +++
 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 853a80fd15af..61d279ae2f71 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1133,13 +1133,15 @@ static void mesh_next(struct hci_dev *hdev, void *data, int err)
 		return;
 	}
 
-	err = hci_cmd_sync_queue(hdev, mesh_send_sync, mesh_tx,
+	err = hci_cmd_sync_queue(hdev, mesh_send_sync, mgmt_mesh_get(mesh_tx),
 				 mesh_send_start_complete);
 
-	if (err < 0)
+	if (err < 0) {
+		mgmt_mesh_put(mesh_tx);
 		mesh_send_complete(hdev, mesh_tx, false);
-	else
+	} else {
 		hci_dev_set_flag(hdev, HCI_MESH_SENDING);
+	}
 
 	hci_dev_unlock(hdev);
 }
@@ -2328,7 +2330,7 @@ static void mesh_send_start_complete(struct hci_dev *hdev, void *data, int err)
 	u8 mgmt_err = mgmt_status(err);
 
 	if (err == -ECANCELED)
-		return;
+		goto put;
 
 	/* Report any errors here, but don't report completion */
 
@@ -2338,12 +2340,15 @@ static void mesh_send_start_complete(struct hci_dev *hdev, void *data, int err)
 		hci_dev_lock(hdev);
 		mesh_send_complete(hdev, mesh_tx, false);
 		hci_dev_unlock(hdev);
-		return;
+		goto put;
 	}
 
 	mesh_send_interval = msecs_to_jiffies((send->cnt) * 25);
 	queue_delayed_work(hdev->req_workqueue, &hdev->mesh_send_done,
 			   mesh_send_interval);
+
+put:
+	mgmt_mesh_put(mesh_tx);
 }
 
 static int mesh_send_sync(struct hci_dev *hdev, void *data)
@@ -2549,11 +2554,15 @@ static int mesh_send(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
 	sending = hci_dev_test_flag(hdev, HCI_MESH_SENDING);
 	mesh_tx = mgmt_mesh_add(sk, hdev, send, len);
 
-	if (!mesh_tx)
+	if (!mesh_tx) {
 		err = -ENOMEM;
-	else if (!sending)
-		err = hci_cmd_sync_queue(hdev, mesh_send_sync, mesh_tx,
+	} else if (!sending) {
+		err = hci_cmd_sync_queue(hdev, mesh_send_sync,
+					 mgmt_mesh_get(mesh_tx),
 					 mesh_send_start_complete);
+		if (err < 0)
+			mgmt_mesh_put(mesh_tx);
+	}
 
 	if (err < 0) {
 		bt_dev_err(hdev, "Send Mesh Failed %d", err);
diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c
index a822091f2907..c7543964525a 100644
--- a/net/bluetooth/mgmt_util.c
+++ b/net/bluetooth/mgmt_util.c
@@ -422,6 +422,7 @@ struct mgmt_mesh_tx *mgmt_mesh_add(struct sock *sk, struct hci_dev *hdev,
 	if (!mesh_tx)
 		return NULL;
 
+	refcount_set(&mesh_tx->ref, 1);
 	hdev->mesh_send_ref++;
 	if (!hdev->mesh_send_ref)
 		hdev->mesh_send_ref++;
@@ -438,9 +439,27 @@ struct mgmt_mesh_tx *mgmt_mesh_add(struct sock *sk, struct hci_dev *hdev,
 	return mesh_tx;
 }
 
-void mgmt_mesh_remove(struct mgmt_mesh_tx *mesh_tx)
+struct mgmt_mesh_tx *mgmt_mesh_get(struct mgmt_mesh_tx *mesh_tx)
+{
+	refcount_inc(&mesh_tx->ref);
+
+	return mesh_tx;
+}
+
+void mgmt_mesh_put(struct mgmt_mesh_tx *mesh_tx)
 {
-	list_del(&mesh_tx->list);
+	if (!refcount_dec_and_test(&mesh_tx->ref))
+		return;
+
 	sock_put(mesh_tx->sk);
 	kfree(mesh_tx);
 }
+
+void mgmt_mesh_remove(struct mgmt_mesh_tx *mesh_tx)
+{
+	if (list_empty(&mesh_tx->list))
+		return;
+
+	list_del_init(&mesh_tx->list);
+	mgmt_mesh_put(mesh_tx);
+}
diff --git a/net/bluetooth/mgmt_util.h b/net/bluetooth/mgmt_util.h
index 20810cf06e81..b38970c1332c 100644
--- a/net/bluetooth/mgmt_util.h
+++ b/net/bluetooth/mgmt_util.h
@@ -19,6 +19,7 @@
 
 struct mgmt_mesh_tx {
 	struct list_head list;
+	refcount_t ref;
 	int index;
 	size_t param_len;
 	struct sock *sk;
@@ -72,4 +73,6 @@ struct mgmt_mesh_tx *mgmt_mesh_find(struct hci_dev *hdev, u8 handle);
 struct mgmt_mesh_tx *mgmt_mesh_next(struct hci_dev *hdev, struct sock *sk);
 struct mgmt_mesh_tx *mgmt_mesh_add(struct sock *sk, struct hci_dev *hdev,
 				   void *data, u16 len);
+struct mgmt_mesh_tx *mgmt_mesh_get(struct mgmt_mesh_tx *mesh_tx);
+void mgmt_mesh_put(struct mgmt_mesh_tx *mesh_tx);
 void mgmt_mesh_remove(struct mgmt_mesh_tx *mesh_tx);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 0/3] Bluetooth: MGMT: fix use-after-free of struct mgmt_mesh_tx

[Luiz Augusto von Dentz]

Hi Baul,

On Fri, Aug 7, 2026 at 6:15 AM Baul Lee <baul.lee@xbow.com> wrote:
>
> hdev->mesh_pending is extended and walked from syscall context under
> hdev->lock, and unlinked and freed from the hci_cmd_sync worker under
> hci_req_sync_lock, so the list has no protection; the objects on it are
> also handed to hci_cmd_sync_queue() as raw pointers, and those work
> entries outlive the list.  Three use-after-frees follow, all reproduced
> under KASAN.
>
> Patch 1 removes mgmt_cleanup(), the one walker of the list that cannot
> take hdev->lock.  Patch 2 puts the list under hdev->lock on the worker
> side and asserts it in the helpers.  Patch 3 gives the object a
> reference count for the work entry.  The order matters: the assertions
> in patch 2 have no violating caller left once patch 1 is in, and the
> reference in patch 3 is only well defined once every unlink happens
> under one lock.
>
> Reaching any of this needs CAP_NET_ADMIN in the init user namespace.
> hci_sock.c sets HCI_SOCK_TRUSTED at bind under capable() rather than
> ns_capable() and gates every later mgmt command on it, and the mesh
> commands additionally need HCI_MESH_EXPERIMENTAL.  Nothing in
> hci_event.c touches hdev->mesh_pending.
>
> Each reproducer was run at identical parameters on the unpatched and the
> patched kernel, built with KASAN, PROVE_LOCKING, DEBUG_LIST and
> DEBUG_ATOMIC_SLEEP: every splat the unpatched kernel produces is gone.
>
> v2: rebased onto bluetooth-next, where send_cancel() no longer ends in
>     mgmt_pending_free(), so v1 did not apply for the CI.  Context only,
>     no other change; v1 applies as posted to the bluetooth tree, which
>     still has that line.
>
> Baul Lee (3):
>   Bluetooth: MGMT: remove the mesh walk from the socket destructor
>   Bluetooth: MGMT: protect hdev->mesh_pending with hdev->lock
>   Bluetooth: MGMT: reference-count struct mgmt_mesh_tx
>
>  include/net/bluetooth/bluetooth.h |  1 -
>  net/bluetooth/hci_sock.c          |  1 -
>  net/bluetooth/mgmt.c              | 72 ++++++++++++++++++++++++---------------
>  net/bluetooth/mgmt_util.c         | 31 +++++++++++++++--
>  net/bluetooth/mgmt_util.h         |  3 ++
>  5 files changed, 76 insertions(+), 32 deletions(-)
>
> --
> 2.50.1 (Apple Git-155)

Sashiko found a couple of problems:

https://sashiko.dev/#/patchset/20260807101529.17348-1-baul.lee%40xbow.com

-- 
Luiz Augusto von Dentz

[PATCH 1/3] Bluetooth: MGMT: remove the mesh walk from the socket destructor

[Baul Lee]

hci_sock_destruct() calls mgmt_cleanup(), which walks hdev->mesh_pending
on every registered controller looking for entries owned by the socket
being destroyed, and completes the ones it finds.

It can never find one.  mgmt_mesh_add() takes a reference on the owning
socket for every entry it links onto the list and mgmt_mesh_remove()
drops it again, so the socket's reference count cannot reach zero while
one of its entries is there.

The walk still races the list.  mgmt_mesh_next() loads mesh_tx->sk from
every node it passes, including nodes owned by other sockets, while the
cmd_sync worker unlinks and frees nodes of the same list under a
different lock.  mgmt_cleanup() cannot take hdev->lock: it holds
read_lock(&hci_dev_list_lock) across the walk, and hdev->lock sleeps.
It is the one user of hdev->mesh_pending that cannot be brought under
that lock.

Remove mgmt_cleanup() and its caller.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>

Fixes: b338d91703fa ("Bluetooth: Implement support for Mesh")
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 include/net/bluetooth/bluetooth.h |  1 -
 net/bluetooth/hci_sock.c          |  1 -
 net/bluetooth/mgmt.c              | 19 -------------------
 3 files changed, 21 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index b624da5026f5..f49cec5f01e6 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -675,7 +675,6 @@ static inline bool iso_inited(void)
 
 int mgmt_init(void);
 void mgmt_exit(void);
-void mgmt_cleanup(struct sock *sk);
 
 void bt_sock_reclassify_lock(struct sock *sk, int proto);
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 070ca388f9ac..5073f4fc3289 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -2164,7 +2164,6 @@ static int hci_sock_getsockopt(struct socket *sock, int level, int optname,
 
 static void hci_sock_destruct(struct sock *sk)
 {
-	mgmt_cleanup(sk);
 	skb_queue_purge(&sk->sk_receive_queue);
 	skb_queue_purge(&sk->sk_write_queue);
 	skb_queue_purge(&sk->sk_error_queue);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 167d75e34526..f13e73e3814f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -10739,22 +10739,3 @@ void mgmt_exit(void)
 {
 	hci_mgmt_chan_unregister(&chan);
 }
-
-void mgmt_cleanup(struct sock *sk)
-{
-	struct mgmt_mesh_tx *mesh_tx;
-	struct hci_dev *hdev;
-
-	read_lock(&hci_dev_list_lock);
-
-	list_for_each_entry(hdev, &hci_dev_list, list) {
-		do {
-			mesh_tx = mgmt_mesh_next(hdev, sk);
-
-			if (mesh_tx)
-				mesh_send_complete(hdev, mesh_tx, true);
-		} while (mesh_tx);
-	}
-
-	read_unlock(&hci_dev_list_lock);
-}
-- 
2.50.1 (Apple Git-155)

RE: Bluetooth: MGMT: fix use-after-free of struct mgmt_mesh_tx

[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:2429
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