* [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list
@ 2025-05-28 21:07 Luiz Augusto von Dentz
2025-05-28 21:34 ` [v2] " bluez.test.bot
2025-05-29 14:21 ` [PATCH v2] " Pauli Virtanen
0 siblings, 2 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2025-05-28 21:07 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This uses RCU procedures to protect from concurrent access of
mgmt_pending list which can cause crashes like:
==================================================================
BUG: KASAN: slab-use-after-free in mgmt_remove_adv_monitor_complete+0xe5/0x540 net/bluetooth/mgmt.c:5405
Read of size 8 at addr ffff888048891a18 by task kworker/u5:8/5333
CPU: 0 UID: 0 PID: 5333 Comm: kworker/u5:8 Not tainted 6.15.0-rc5-syzkaller-00197-gea34704d6ad7 #0 PREEMPT(full)
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
Workqueue: hci0 hci_cmd_sync_work
Call Trace:
<TASK>
dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
print_address_description mm/kasan/report.c:408 [inline]
print_report+0xb4/0x290 mm/kasan/report.c:521
kasan_report+0x118/0x150 mm/kasan/report.c:634
mgmt_remove_adv_monitor_complete+0xe5/0x540 net/bluetooth/mgmt.c:5405
hci_cmd_sync_work+0x25e/0x3a0 net/bluetooth/hci_sync.c:334
process_one_work kernel/workqueue.c:3238 [inline]
process_scheduled_works+0xadb/0x17a0 kernel/workqueue.c:3319
worker_thread+0x8a0/0xda0 kernel/workqueue.c:3400
kthread+0x70e/0x8a0 kernel/kthread.c:464
ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:153
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
</TASK>
Allocated by task 5702:
kasan_save_stack mm/kasan/common.c:47 [inline]
kasan_save_track+0x3e/0x80 mm/kasan/common.c:68
poison_kmalloc_redzone mm/kasan/common.c:377 [inline]
__kasan_kmalloc+0x93/0xb0 mm/kasan/common.c:394
kasan_kmalloc include/linux/kasan.h:260 [inline]
__kmalloc_cache_noprof+0x230/0x3d0 mm/slub.c:4358
kmalloc_noprof include/linux/slab.h:905 [inline]
kzalloc_noprof include/linux/slab.h:1039 [inline]
mgmt_pending_new+0x65/0x240 net/bluetooth/mgmt_util.c:252
mgmt_pending_add+0x34/0x120 net/bluetooth/mgmt_util.c:279
remove_adv_monitor+0x103/0x1b0 net/bluetooth/mgmt.c:5453
hci_mgmt_cmd+0x9c6/0xef0 net/bluetooth/hci_sock.c:1712
hci_sock_sendmsg+0x6ca/0xee0 net/bluetooth/hci_sock.c:1832
sock_sendmsg_nosec net/socket.c:712 [inline]
__sock_sendmsg+0x219/0x270 net/socket.c:727
sock_write_iter+0x258/0x330 net/socket.c:1131
new_sync_write fs/read_write.c:591 [inline]
vfs_write+0x548/0xa90 fs/read_write.c:684
ksys_write+0x145/0x250 fs/read_write.c:736
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xf6/0x210 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Freed by task 5700:
kasan_save_stack mm/kasan/common.c:47 [inline]
kasan_save_track+0x3e/0x80 mm/kasan/common.c:68
kasan_save_free_info+0x46/0x50 mm/kasan/generic.c:576
poison_slab_object mm/kasan/common.c:247 [inline]
__kasan_slab_free+0x62/0x70 mm/kasan/common.c:264
kasan_slab_free include/linux/kasan.h:233 [inline]
slab_free_hook mm/slub.c:2380 [inline]
slab_free mm/slub.c:4642 [inline]
kfree+0x193/0x440 mm/slub.c:4841
mgmt_pending_foreach+0xc9/0x120 net/bluetooth/mgmt_util.c:242
mgmt_index_removed+0x10d/0x2f0 net/bluetooth/mgmt.c:9362
hci_sock_bind+0xbe9/0x1000 net/bluetooth/hci_sock.c:1307
__sys_bind_socket net/socket.c:1810 [inline]
__sys_bind+0x2c3/0x3e0 net/socket.c:1841
__do_sys_bind net/socket.c:1846 [inline]
__se_sys_bind net/socket.c:1844 [inline]
__x64_sys_bind+0x7a/0x90 net/socket.c:1844
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0xf6/0x210 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Fixes: a380b6cff1a2 ("Bluetooth: Add generic mgmt helper API")
Closes: https://syzkaller.appspot.com/bug?extid=feb0dc579bbe30a13190
Closes: https://syzkaller.appspot.com/bug?extid=0a7039d5d9986ff4ececi
Closes: https://syzkaller.appspot.com/bug?extid=cc0cc52e7f43dc9e6df1
Reported-by: syzbot+feb0dc579bbe30a13190@syzkaller.appspotmail.com
Tested-by: syzbot+feb0dc579bbe30a13190@syzkaller.appspotmail.com
Tested-by: syzbot+0a7039d5d9986ff4ecec@syzkaller.appspotmail.com
Tested-by: syzbot+cc0cc52e7f43dc9e6df1@syzkaller.appspotmail.com
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/mgmt_util.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c
index 3713ff490c65..c2dc8ddf5f78 100644
--- a/net/bluetooth/mgmt_util.c
+++ b/net/bluetooth/mgmt_util.c
@@ -219,13 +219,20 @@ struct mgmt_pending_cmd *mgmt_pending_find(unsigned short channel, u16 opcode,
{
struct mgmt_pending_cmd *cmd;
- list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
+ rcu_read_lock();
+
+ list_for_each_entry_rcu(cmd, &hdev->mgmt_pending, list) {
if (hci_sock_get_channel(cmd->sk) != channel)
continue;
- if (cmd->opcode == opcode)
+
+ if (cmd->opcode == opcode) {
+ rcu_read_unlock();
return cmd;
+ }
}
+ rcu_read_unlock();
+
return NULL;
}
@@ -233,14 +240,11 @@ void mgmt_pending_foreach(u16 opcode, struct hci_dev *hdev,
void (*cb)(struct mgmt_pending_cmd *cmd, void *data),
void *data)
{
- struct mgmt_pending_cmd *cmd, *tmp;
-
- list_for_each_entry_safe(cmd, tmp, &hdev->mgmt_pending, list) {
- if (opcode > 0 && cmd->opcode != opcode)
- continue;
+ struct mgmt_pending_cmd *cmd;
+ cmd = mgmt_pending_find(HCI_CHANNEL_CONTROL, opcode, hdev);
+ if (cmd)
cb(cmd, data);
- }
}
struct mgmt_pending_cmd *mgmt_pending_new(struct sock *sk, u16 opcode,
@@ -280,7 +284,7 @@ struct mgmt_pending_cmd *mgmt_pending_add(struct sock *sk, u16 opcode,
if (!cmd)
return NULL;
- list_add_tail(&cmd->list, &hdev->mgmt_pending);
+ list_add_tail_rcu(&cmd->list, &hdev->mgmt_pending);
return cmd;
}
@@ -294,7 +298,8 @@ void mgmt_pending_free(struct mgmt_pending_cmd *cmd)
void mgmt_pending_remove(struct mgmt_pending_cmd *cmd)
{
- list_del(&cmd->list);
+ list_del_rcu(&cmd->list);
+ synchronize_rcu();
mgmt_pending_free(cmd);
}
--
2.49.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* RE: [v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list 2025-05-28 21:07 [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list Luiz Augusto von Dentz @ 2025-05-28 21:34 ` bluez.test.bot 2025-05-29 14:21 ` [PATCH v2] " Pauli Virtanen 1 sibling, 0 replies; 4+ messages in thread From: bluez.test.bot @ 2025-05-28 21:34 UTC (permalink / raw) To: linux-bluetooth, luiz.dentz [-- Attachment #1: Type: text/plain, Size: 3726 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=967137 ---Test result--- Test Summary: CheckPatch PENDING 0.32 seconds GitLint PENDING 0.39 seconds SubjectPrefix PASS 0.06 seconds BuildKernel PASS 24.71 seconds CheckAllWarning PASS 26.96 seconds CheckSparse PASS 30.34 seconds BuildKernel32 PASS 24.25 seconds TestRunnerSetup PASS 458.62 seconds TestRunner_l2cap-tester PASS 25.28 seconds TestRunner_iso-tester PASS 51.89 seconds TestRunner_bnep-tester PASS 5.80 seconds TestRunner_mgmt-tester FAIL 156.30 seconds TestRunner_rfcomm-tester PASS 11.38 seconds TestRunner_sco-tester PASS 55.78 seconds TestRunner_ioctl-tester PASS 10.02 seconds TestRunner_mesh-tester FAIL 7.52 seconds TestRunner_smp-tester PASS 8.46 seconds TestRunner_userchan-tester PASS 6.15 seconds IncrementalBuild PENDING 0.50 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 490, Passed: 470 (95.9%), Failed: 17, Not Run: 3 Failed Test Cases Stop Discovery - Success 1 Failed 0.162 seconds Pairing Acceptor - SMP over BR/EDR 2 Timed out 2.227 seconds Add Ext Advertising - Success 1 (Powered, Add Adv Inst) Failed 0.150 seconds Add Ext Advertising - Success 6 (Scan Rsp Dta, Adv ok) Failed 0.125 seconds Add Ext Advertising - Success 9 (General Discov Flag) Failed 2.157 seconds Add Ext Advertising - Success 10 (Limited Discov Flag) Failed 2.167 seconds Stop Discovery - (Ext Scan Disable) Failed 0.165 seconds LL Privacy - Add Device 2 (2 Devices to AL) Failed 0.192 seconds LL Privacy - Add Device 3 (AL is full) Failed 0.236 seconds LL Privacy - Set Flags 1 (Add to RL) Failed 0.180 seconds LL Privacy - Set Flags 2 (Enable RL) Failed 0.189 seconds LL Privacy - Set Flags 3 (2 Devices to RL) Failed 0.208 seconds LL Privacy - Set Flags 4 (RL is full) Failed 0.271 seconds LL Privacy - Remove Device 1 (Remove from AL) Timed out 1.971 seconds LL Privacy - Remove Device 2 (Remove from RL) Timed out 1.997 seconds LL Privacy - Remove Device 4 (Disable Adv) Timed out 2.762 seconds LL Privacy - Set Device Flag 1 (Device Privacy) Failed 0.184 seconds ############################## Test: TestRunner_mesh-tester - FAIL Desc: Run mesh-tester with test-runner Output: BUG: KASAN: slab-use-after-free in run_timer_softirq+0x76b/0x7d0 WARNING: CPU: 0 PID: 67 at kernel/workqueue.c:2257 __queue_work+0x93e/0xba0 Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0 Failed Test Cases Mesh - Send cancel - 1 Failed 0.140 seconds Mesh - Send cancel - 2 Failed 0.156 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list 2025-05-28 21:07 [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list Luiz Augusto von Dentz 2025-05-28 21:34 ` [v2] " bluez.test.bot @ 2025-05-29 14:21 ` Pauli Virtanen 2025-05-29 16:09 ` Luiz Augusto von Dentz 1 sibling, 1 reply; 4+ messages in thread From: Pauli Virtanen @ 2025-05-29 14:21 UTC (permalink / raw) To: Luiz Augusto von Dentz, linux-bluetooth; +Cc: dmantipov Hi, ke, 2025-05-28 kello 17:07 -0400, Luiz Augusto von Dentz kirjoitti: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This uses RCU procedures to protect from concurrent access of > mgmt_pending list which can cause crashes like: > > ================================================================== > BUG: KASAN: slab-use-after-free in mgmt_remove_adv_monitor_complete+0xe5/0x540 net/bluetooth/mgmt.c:5405 > Read of size 8 at addr ffff888048891a18 by task kworker/u5:8/5333 > > CPU: 0 UID: 0 PID: 5333 Comm: kworker/u5:8 Not tainted 6.15.0-rc5-syzkaller-00197-gea34704d6ad7 #0 PREEMPT(full) > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014 > Workqueue: hci0 hci_cmd_sync_work > Call Trace: > <TASK> > dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120 > print_address_description mm/kasan/report.c:408 [inline] > print_report+0xb4/0x290 mm/kasan/report.c:521 > kasan_report+0x118/0x150 mm/kasan/report.c:634 > mgmt_remove_adv_monitor_complete+0xe5/0x540 net/bluetooth/mgmt.c:5405 > hci_cmd_sync_work+0x25e/0x3a0 net/bluetooth/hci_sync.c:334 > process_one_work kernel/workqueue.c:3238 [inline] > process_scheduled_works+0xadb/0x17a0 kernel/workqueue.c:3319 > worker_thread+0x8a0/0xda0 kernel/workqueue.c:3400 > kthread+0x70e/0x8a0 kernel/kthread.c:464 > ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:153 > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245 > </TASK> > > Allocated by task 5702: > kasan_save_stack mm/kasan/common.c:47 [inline] > kasan_save_track+0x3e/0x80 mm/kasan/common.c:68 > poison_kmalloc_redzone mm/kasan/common.c:377 [inline] > __kasan_kmalloc+0x93/0xb0 mm/kasan/common.c:394 > kasan_kmalloc include/linux/kasan.h:260 [inline] > __kmalloc_cache_noprof+0x230/0x3d0 mm/slub.c:4358 > kmalloc_noprof include/linux/slab.h:905 [inline] > kzalloc_noprof include/linux/slab.h:1039 [inline] > mgmt_pending_new+0x65/0x240 net/bluetooth/mgmt_util.c:252 > mgmt_pending_add+0x34/0x120 net/bluetooth/mgmt_util.c:279 > remove_adv_monitor+0x103/0x1b0 net/bluetooth/mgmt.c:5453 > hci_mgmt_cmd+0x9c6/0xef0 net/bluetooth/hci_sock.c:1712 > hci_sock_sendmsg+0x6ca/0xee0 net/bluetooth/hci_sock.c:1832 > sock_sendmsg_nosec net/socket.c:712 [inline] > __sock_sendmsg+0x219/0x270 net/socket.c:727 > sock_write_iter+0x258/0x330 net/socket.c:1131 > new_sync_write fs/read_write.c:591 [inline] > vfs_write+0x548/0xa90 fs/read_write.c:684 > ksys_write+0x145/0x250 fs/read_write.c:736 > do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] > do_syscall_64+0xf6/0x210 arch/x86/entry/syscall_64.c:94 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > Freed by task 5700: > kasan_save_stack mm/kasan/common.c:47 [inline] > kasan_save_track+0x3e/0x80 mm/kasan/common.c:68 > kasan_save_free_info+0x46/0x50 mm/kasan/generic.c:576 > poison_slab_object mm/kasan/common.c:247 [inline] > __kasan_slab_free+0x62/0x70 mm/kasan/common.c:264 > kasan_slab_free include/linux/kasan.h:233 [inline] > slab_free_hook mm/slub.c:2380 [inline] > slab_free mm/slub.c:4642 [inline] > kfree+0x193/0x440 mm/slub.c:4841 > mgmt_pending_foreach+0xc9/0x120 net/bluetooth/mgmt_util.c:242 > mgmt_index_removed+0x10d/0x2f0 net/bluetooth/mgmt.c:9362 > hci_sock_bind+0xbe9/0x1000 net/bluetooth/hci_sock.c:1307 > __sys_bind_socket net/socket.c:1810 [inline] > __sys_bind+0x2c3/0x3e0 net/socket.c:1841 > __do_sys_bind net/socket.c:1846 [inline] > __se_sys_bind net/socket.c:1844 [inline] > __x64_sys_bind+0x7a/0x90 net/socket.c:1844 > do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] > do_syscall_64+0xf6/0x210 arch/x86/entry/syscall_64.c:94 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > Fixes: a380b6cff1a2 ("Bluetooth: Add generic mgmt helper API") > Closes: https://syzkaller.appspot.com/bug?extid=feb0dc579bbe30a13190 > Closes: https://syzkaller.appspot.com/bug?extid=0a7039d5d9986ff4ececi > Closes: https://syzkaller.appspot.com/bug?extid=cc0cc52e7f43dc9e6df1 > Reported-by: syzbot+feb0dc579bbe30a13190@syzkaller.appspotmail.com > Tested-by: syzbot+feb0dc579bbe30a13190@syzkaller.appspotmail.com > Tested-by: syzbot+0a7039d5d9986ff4ecec@syzkaller.appspotmail.com > Tested-by: syzbot+cc0cc52e7f43dc9e6df1@syzkaller.appspotmail.com > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > net/bluetooth/mgmt_util.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c > index 3713ff490c65..c2dc8ddf5f78 100644 > --- a/net/bluetooth/mgmt_util.c > +++ b/net/bluetooth/mgmt_util.c > @@ -219,13 +219,20 @@ struct mgmt_pending_cmd *mgmt_pending_find(unsigned short channel, u16 opcode, > { > struct mgmt_pending_cmd *cmd; > > - list_for_each_entry(cmd, &hdev->mgmt_pending, list) { > + rcu_read_lock(); > + > + list_for_each_entry_rcu(cmd, &hdev->mgmt_pending, list) { > if (hci_sock_get_channel(cmd->sk) != channel) > continue; > - if (cmd->opcode == opcode) > + > + if (cmd->opcode == opcode) { > + rcu_read_unlock(); > return cmd; RCU does not guarantee the returned pointer is not already freed when this returns. AFAIK this is exactly the "BUG!!!" mentioned in https://docs.kernel.org/RCU/whatisRCU.html#rcu-dereference Instead of calling rcu_read_lock/unlock here, maybe instead list_for_each_entry_rcu(cmd, &hdev->mgmt_pending, list, lockdep_is_held(&hdev->lock)) to force caller to either hold rcu_read_lock() or hdev->lock to protect the return value for the time it needs it. > + } > } > > + rcu_read_unlock(); > + > return NULL; > } > > @@ -233,14 +240,11 @@ void mgmt_pending_foreach(u16 opcode, struct hci_dev *hdev, > void (*cb)(struct mgmt_pending_cmd *cmd, void *data), > void *data) > { > - struct mgmt_pending_cmd *cmd, *tmp; > - > - list_for_each_entry_safe(cmd, tmp, &hdev->mgmt_pending, list) { > - if (opcode > 0 && cmd->opcode != opcode) > - continue; > + struct mgmt_pending_cmd *cmd; > > + cmd = mgmt_pending_find(HCI_CHANNEL_CONTROL, opcode, hdev); > + if (cmd) > cb(cmd, data); Hence, this is potential UAF, so caller probably shall hold locks as above. With the change in list_for_each_entry_rcu(), you'd then get lockdep splats if caller doesn't hold right locks. E.g. the UAF in struct mgmt_pending_cmd *cmd = data; if (cmd != pending_find(MGMT_OP_SET_POWERED, hdev)) return -ECANCELED; hci_dev_lock(hdev); /* operate on cmd */ hci_dev_unlock(hdev); should be found directly by the assert. Note that such pattern of checking if a pointer in the "data" of a delayed callback corresponds to an "alive" object in principle also has ABA problem (mgmt_pending_free(cmd) + mgmt_pending_add() allocating at same address -> operates on wrong item). Also hci_conn_valid() has this issue. > - } > } > > struct mgmt_pending_cmd *mgmt_pending_new(struct sock *sk, u16 opcode, > @@ -280,7 +284,7 @@ struct mgmt_pending_cmd *mgmt_pending_add(struct sock *sk, u16 opcode, > if (!cmd) > return NULL; > > - list_add_tail(&cmd->list, &hdev->mgmt_pending); > + list_add_tail_rcu(&cmd->list, &hdev->mgmt_pending); > > return cmd; > } > @@ -294,7 +298,8 @@ void mgmt_pending_free(struct mgmt_pending_cmd *cmd) > > void mgmt_pending_remove(struct mgmt_pending_cmd *cmd) > { > - list_del(&cmd->list); > + list_del_rcu(&cmd->list); > + synchronize_rcu(); Maybe it would be useful to add lockdep_assert_held(&hdev->lock) here and in mgmt_pending_add() to make sure callers hold right lock? These compile to nothing with !CONFIG_LOCKDEP so IIUC could be used more. > mgmt_pending_free(cmd); > } > -- Pauli Virtanen ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list 2025-05-29 14:21 ` [PATCH v2] " Pauli Virtanen @ 2025-05-29 16:09 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 4+ messages in thread From: Luiz Augusto von Dentz @ 2025-05-29 16:09 UTC (permalink / raw) To: Pauli Virtanen; +Cc: linux-bluetooth, dmantipov Hi Pauli, On Thu, May 29, 2025 at 10:21 AM Pauli Virtanen <pav@iki.fi> wrote: > > Hi, > > ke, 2025-05-28 kello 17:07 -0400, Luiz Augusto von Dentz kirjoitti: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > This uses RCU procedures to protect from concurrent access of > > mgmt_pending list which can cause crashes like: > > > > ================================================================== > > BUG: KASAN: slab-use-after-free in mgmt_remove_adv_monitor_complete+0xe5/0x540 net/bluetooth/mgmt.c:5405 > > Read of size 8 at addr ffff888048891a18 by task kworker/u5:8/5333 > > > > CPU: 0 UID: 0 PID: 5333 Comm: kworker/u5:8 Not tainted 6.15.0-rc5-syzkaller-00197-gea34704d6ad7 #0 PREEMPT(full) > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014 > > Workqueue: hci0 hci_cmd_sync_work > > Call Trace: > > <TASK> > > dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120 > > print_address_description mm/kasan/report.c:408 [inline] > > print_report+0xb4/0x290 mm/kasan/report.c:521 > > kasan_report+0x118/0x150 mm/kasan/report.c:634 > > mgmt_remove_adv_monitor_complete+0xe5/0x540 net/bluetooth/mgmt.c:5405 > > hci_cmd_sync_work+0x25e/0x3a0 net/bluetooth/hci_sync.c:334 > > process_one_work kernel/workqueue.c:3238 [inline] > > process_scheduled_works+0xadb/0x17a0 kernel/workqueue.c:3319 > > worker_thread+0x8a0/0xda0 kernel/workqueue.c:3400 > > kthread+0x70e/0x8a0 kernel/kthread.c:464 > > ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:153 > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245 > > </TASK> > > > > Allocated by task 5702: > > kasan_save_stack mm/kasan/common.c:47 [inline] > > kasan_save_track+0x3e/0x80 mm/kasan/common.c:68 > > poison_kmalloc_redzone mm/kasan/common.c:377 [inline] > > __kasan_kmalloc+0x93/0xb0 mm/kasan/common.c:394 > > kasan_kmalloc include/linux/kasan.h:260 [inline] > > __kmalloc_cache_noprof+0x230/0x3d0 mm/slub.c:4358 > > kmalloc_noprof include/linux/slab.h:905 [inline] > > kzalloc_noprof include/linux/slab.h:1039 [inline] > > mgmt_pending_new+0x65/0x240 net/bluetooth/mgmt_util.c:252 > > mgmt_pending_add+0x34/0x120 net/bluetooth/mgmt_util.c:279 > > remove_adv_monitor+0x103/0x1b0 net/bluetooth/mgmt.c:5453 > > hci_mgmt_cmd+0x9c6/0xef0 net/bluetooth/hci_sock.c:1712 > > hci_sock_sendmsg+0x6ca/0xee0 net/bluetooth/hci_sock.c:1832 > > sock_sendmsg_nosec net/socket.c:712 [inline] > > __sock_sendmsg+0x219/0x270 net/socket.c:727 > > sock_write_iter+0x258/0x330 net/socket.c:1131 > > new_sync_write fs/read_write.c:591 [inline] > > vfs_write+0x548/0xa90 fs/read_write.c:684 > > ksys_write+0x145/0x250 fs/read_write.c:736 > > do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] > > do_syscall_64+0xf6/0x210 arch/x86/entry/syscall_64.c:94 > > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > > > Freed by task 5700: > > kasan_save_stack mm/kasan/common.c:47 [inline] > > kasan_save_track+0x3e/0x80 mm/kasan/common.c:68 > > kasan_save_free_info+0x46/0x50 mm/kasan/generic.c:576 > > poison_slab_object mm/kasan/common.c:247 [inline] > > __kasan_slab_free+0x62/0x70 mm/kasan/common.c:264 > > kasan_slab_free include/linux/kasan.h:233 [inline] > > slab_free_hook mm/slub.c:2380 [inline] > > slab_free mm/slub.c:4642 [inline] > > kfree+0x193/0x440 mm/slub.c:4841 > > mgmt_pending_foreach+0xc9/0x120 net/bluetooth/mgmt_util.c:242 > > mgmt_index_removed+0x10d/0x2f0 net/bluetooth/mgmt.c:9362 > > hci_sock_bind+0xbe9/0x1000 net/bluetooth/hci_sock.c:1307 > > __sys_bind_socket net/socket.c:1810 [inline] > > __sys_bind+0x2c3/0x3e0 net/socket.c:1841 > > __do_sys_bind net/socket.c:1846 [inline] > > __se_sys_bind net/socket.c:1844 [inline] > > __x64_sys_bind+0x7a/0x90 net/socket.c:1844 > > do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] > > do_syscall_64+0xf6/0x210 arch/x86/entry/syscall_64.c:94 > > entry_SYSCALL_64_after_hwframe+0x77/0x7f > > > > Fixes: a380b6cff1a2 ("Bluetooth: Add generic mgmt helper API") > > Closes: https://syzkaller.appspot.com/bug?extid=feb0dc579bbe30a13190 > > Closes: https://syzkaller.appspot.com/bug?extid=0a7039d5d9986ff4ececi > > Closes: https://syzkaller.appspot.com/bug?extid=cc0cc52e7f43dc9e6df1 > > Reported-by: syzbot+feb0dc579bbe30a13190@syzkaller.appspotmail.com > > Tested-by: syzbot+feb0dc579bbe30a13190@syzkaller.appspotmail.com > > Tested-by: syzbot+0a7039d5d9986ff4ecec@syzkaller.appspotmail.com > > Tested-by: syzbot+cc0cc52e7f43dc9e6df1@syzkaller.appspotmail.com > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > net/bluetooth/mgmt_util.c | 25 +++++++++++++++---------- > > 1 file changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/net/bluetooth/mgmt_util.c b/net/bluetooth/mgmt_util.c > > index 3713ff490c65..c2dc8ddf5f78 100644 > > --- a/net/bluetooth/mgmt_util.c > > +++ b/net/bluetooth/mgmt_util.c > > @@ -219,13 +219,20 @@ struct mgmt_pending_cmd *mgmt_pending_find(unsigned short channel, u16 opcode, > > { > > struct mgmt_pending_cmd *cmd; > > > > - list_for_each_entry(cmd, &hdev->mgmt_pending, list) { > > + rcu_read_lock(); > > + > > + list_for_each_entry_rcu(cmd, &hdev->mgmt_pending, list) { > > if (hci_sock_get_channel(cmd->sk) != channel) > > continue; > > - if (cmd->opcode == opcode) > > + > > + if (cmd->opcode == opcode) { > > + rcu_read_unlock(); > > return cmd; > > RCU does not guarantee the returned pointer is not already freed when > this returns. AFAIK this is exactly the "BUG!!!" mentioned in > https://docs.kernel.org/RCU/whatisRCU.html#rcu-dereference > > Instead of calling rcu_read_lock/unlock here, maybe instead > > list_for_each_entry_rcu(cmd, &hdev->mgmt_pending, list, > lockdep_is_held(&hdev->lock)) > > to force caller to either hold rcu_read_lock() or hdev->lock to protect > the return value for the time it needs it. This is not so different then the current design though, perhaps we should refcount the entries so the likes of mgmt_pending_find returns a new reference using a kref. > > + } > > } > > > > + rcu_read_unlock(); > > + > > return NULL; > > } > > > > @@ -233,14 +240,11 @@ void mgmt_pending_foreach(u16 opcode, struct hci_dev *hdev, > > void (*cb)(struct mgmt_pending_cmd *cmd, void *data), > > void *data) > > { > > - struct mgmt_pending_cmd *cmd, *tmp; > > - > > - list_for_each_entry_safe(cmd, tmp, &hdev->mgmt_pending, list) { > > - if (opcode > 0 && cmd->opcode != opcode) > > - continue; > > + struct mgmt_pending_cmd *cmd; > > > > + cmd = mgmt_pending_find(HCI_CHANNEL_CONTROL, opcode, hdev); > > + if (cmd) > > cb(cmd, data); > > Hence, this is potential UAF, so caller probably shall hold locks as > above. > > With the change in list_for_each_entry_rcu(), you'd then get lockdep > splats if caller doesn't hold right locks. > > E.g. the UAF in > > struct mgmt_pending_cmd *cmd = data; > if (cmd != pending_find(MGMT_OP_SET_POWERED, hdev)) > return -ECANCELED; > > hci_dev_lock(hdev); > /* operate on cmd */ > hci_dev_unlock(hdev); > > should be found directly by the assert. > > Note that such pattern of checking if a pointer in the "data" of a > delayed callback corresponds to an "alive" object in principle also has > ABA problem (mgmt_pending_free(cmd) + mgmt_pending_add() allocating at > same address -> operates on wrong item). Also hci_conn_valid() has this > issue. These are normally run at the callback of cmd_sync and triggered with a MGMT command, so I very much doubt you can create a scenario where the same MGMT command, yes it needs to be the same opcode at least so pending_find will need to return a pointer, can be scheduled, cancelled and then rescheduled with the very same pointer, anyway even if that in theory is possible them we quite possible have a problem with cmd_sync work carrying a pointer that has been freed. > > - } > > } > > > > struct mgmt_pending_cmd *mgmt_pending_new(struct sock *sk, u16 opcode, > > @@ -280,7 +284,7 @@ struct mgmt_pending_cmd *mgmt_pending_add(struct sock *sk, u16 opcode, > > if (!cmd) > > return NULL; > > > > - list_add_tail(&cmd->list, &hdev->mgmt_pending); > > + list_add_tail_rcu(&cmd->list, &hdev->mgmt_pending); > > > > return cmd; > > } > > @@ -294,7 +298,8 @@ void mgmt_pending_free(struct mgmt_pending_cmd *cmd) > > > > void mgmt_pending_remove(struct mgmt_pending_cmd *cmd) > > { > > - list_del(&cmd->list); > > + list_del_rcu(&cmd->list); > > + synchronize_rcu(); > > Maybe it would be useful to add lockdep_assert_held(&hdev->lock) here > and in mgmt_pending_add() to make sure callers hold right lock? > > These compile to nothing with !CONFIG_LOCKDEP so IIUC could be used > more. Or we just introduce a mgmt_lock, since it appears we will need a lock anyway. > > mgmt_pending_free(cmd); > > } > > > > -- > Pauli Virtanen -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-29 16:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-28 21:07 [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list Luiz Augusto von Dentz 2025-05-28 21:34 ` [v2] " bluez.test.bot 2025-05-29 14:21 ` [PATCH v2] " Pauli Virtanen 2025-05-29 16:09 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox