[PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list

[PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list

[Luiz Augusto von Dentz]

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

RE: [v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list

[bluez.test.bot]

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

Re: [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list

[Pauli Virtanen]

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

Re: [PATCH v2] Bluetooth: MGMT: Use RCU-protected in mgmt_pending list

[Luiz Augusto von Dentz]

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