diff mbox series

[v2] Bluetooth: MGMT: Fix possible crash on mgmt_index_removed

Message ID 20240913165010.3240215-1-luiz.dentz@gmail.com (mailing list archive)
State Accepted
Commit eb3ad76a07b6cdaaa156766da5fe6c384a12930b
Headers show
Series [v2] Bluetooth: MGMT: Fix possible crash on mgmt_index_removed | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #103: Reported-by: jiaymao <quic_jiaymao@quicinc.com> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> total: 0 errors, 1 warnings, 0 checks, 54 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13803844.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS

Commit Message

Luiz Augusto von Dentz Sept. 13, 2024, 4:50 p.m. UTC
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If mgmt_index_removed is called while there are commands queued on
cmd_sync it could lead to crashes like the bellow trace:

0x0000053D: __list_del_entry_valid_or_report+0x98/0xdc
0x0000053D: mgmt_pending_remove+0x18/0x58 [bluetooth]
0x0000053E: mgmt_remove_adv_monitor_complete+0x80/0x108 [bluetooth]
0x0000053E: hci_cmd_sync_work+0xbc/0x164 [bluetooth]

So while handling mgmt_index_removed this attempts to dequeue
commands passed as user_data to cmd_sync.

Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor")
Reported-by: jiaymao <quic_jiaymao@quicinc.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/mgmt.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

patchwork-bot+bluetooth@kernel.org Sept. 16, 2024, 4 p.m. UTC | #1
Hello:

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

On Fri, 13 Sep 2024 12:50:10 -0400 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> If mgmt_index_removed is called while there are commands queued on
> cmd_sync it could lead to crashes like the bellow trace:
> 
> 0x0000053D: __list_del_entry_valid_or_report+0x98/0xdc
> 0x0000053D: mgmt_pending_remove+0x18/0x58 [bluetooth]
> 0x0000053E: mgmt_remove_adv_monitor_complete+0x80/0x108 [bluetooth]
> 0x0000053E: hci_cmd_sync_work+0xbc/0x164 [bluetooth]
> 
> [...]

Here is the summary with links:
  - [v2] Bluetooth: MGMT: Fix possible crash on mgmt_index_removed
    https://git.kernel.org/bluetooth/bluetooth-next/c/eb3ad76a07b6

You are awesome, thank you!
Sungwoo Kim Sept. 30, 2024, 11:58 p.m. UTC | #2
Is this bug related to the recent patch?
It happend bt-next 667e8026261de5d230908, cloned at Sep 23.

==================================================================
BUG: KASAN: slab-use-after-free in cmd_complete_rsp+0x1b3/0x1e0 net/bluetooth/mgmt.c:1463
Read of size 8 at addr ffff888112f0b740 by task kworker/u9:0/246

CPU: 0 UID: 0 PID: 246 Comm: kworker/u9:0 Not tainted 6.11.0-rc6-g667e8026261d-dirty #10
Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Workqueue: hci0 hci_error_reset
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:93 [inline]
 dump_stack_lvl+0x7b/0xa0 lib/dump_stack.c:119
 print_address_description mm/kasan/report.c:377 [inline]
 print_report+0xce/0x670 mm/kasan/report.c:488
 kasan_report+0xc6/0x100 mm/kasan/report.c:601
 cmd_complete_rsp+0x1b3/0x1e0 net/bluetooth/mgmt.c:1463
 mgmt_pending_foreach+0x98/0x160 net/bluetooth/mgmt_util.c:259
 __mgmt_power_off+0x122/0x290 net/bluetooth/mgmt.c:9474
 hci_dev_close_sync+0xbd5/0x1110 net/bluetooth/hci_sync.c:5191
 hci_dev_do_close net/bluetooth/hci_core.c:483 [inline]
 hci_error_reset+0x1cb/0x420 net/bluetooth/hci_core.c:1016
 process_one_work+0x61a/0x1050 kernel/workqueue.c:3231
 process_scheduled_works kernel/workqueue.c:3312 [inline]
 worker_thread+0x8d9/0x1120 kernel/workqueue.c:3389
 kthread+0x25a/0x330 kernel/kthread.c:389
 ret_from_fork+0x48/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
 </TASK>

Allocated by task 250:
 kasan_save_stack+0x33/0x60 mm/kasan/common.c:47
 kasan_save_track+0x14/0x30 mm/kasan/common.c:68
 poison_kmalloc_redzone mm/kasan/common.c:370 [inline]
 __kasan_kmalloc+0x8f/0xa0 mm/kasan/common.c:387
 kmalloc_noprof include/linux/slab.h:681 [inline]
 kzalloc_noprof include/linux/slab.h:807 [inline]
 mgmt_pending_new+0x5b/0x270 net/bluetooth/mgmt_util.c:269
 mgmt_pending_add+0x34/0x110 net/bluetooth/mgmt_util.c:296
 set_local_name+0x18e/0x3e0 net/bluetooth/mgmt.c:3857
 hci_mgmt_cmd net/bluetooth/hci_sock.c:1726 [inline]
 hci_sock_sendmsg+0x122f/0x2200 net/bluetooth/hci_sock.c:1846
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg net/socket.c:745 [inline]
 sock_write_iter+0x4a9/0x570 net/socket.c:1160
 do_iter_readv_writev+0x4ee/0x680 fs/read_write.c:741
 vfs_writev+0x328/0xae0 fs/read_write.c:971
 do_writev+0x236/0x2f0 fs/read_write.c:1018
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xa6/0x1a0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Freed by task 246:
 kasan_save_stack+0x33/0x60 mm/kasan/common.c:47
 kasan_save_track+0x14/0x30 mm/kasan/common.c:68
 kasan_save_free_info+0x3b/0x60 mm/kasan/generic.c:579
 poison_slab_object+0x109/0x170 mm/kasan/common.c:240
 __kasan_slab_free+0x14/0x30 mm/kasan/common.c:256
 kasan_slab_free include/linux/kasan.h:184 [inline]
 slab_free_hook mm/slub.c:2256 [inline]
 slab_free mm/slub.c:4477 [inline]
 kfree+0x9c/0x220 mm/slub.c:4598
 set_name_complete+0x14b/0x200 net/bluetooth/mgmt.c:3799
 _hci_cmd_sync_cancel_entry net/bluetooth/hci_sync.c:641 [inline]
 hci_cmd_sync_dequeue+0x202/0x370 net/bluetooth/hci_sync.c:886
 cmd_complete_rsp+0x46/0x1e0 net/bluetooth/mgmt.c:1461
 mgmt_pending_foreach+0x98/0x160 net/bluetooth/mgmt_util.c:259
 __mgmt_power_off+0x122/0x290 net/bluetooth/mgmt.c:9474
 hci_dev_close_sync+0xbd5/0x1110 net/bluetooth/hci_sync.c:5191
 hci_dev_do_close net/bluetooth/hci_core.c:483 [inline]
 hci_error_reset+0x1cb/0x420 net/bluetooth/hci_core.c:1016
 process_one_work+0x61a/0x1050 kernel/workqueue.c:3231
 process_scheduled_works kernel/workqueue.c:3312 [inline]
 worker_thread+0x8d9/0x1120 kernel/workqueue.c:3389
 kthread+0x25a/0x330 kernel/kthread.c:389
 ret_from_fork+0x48/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
diff mbox series

Patch

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e4f564d6f6fb..4157d9f23f46 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1453,10 +1453,15 @@  static void cmd_status_rsp(struct mgmt_pending_cmd *cmd, void *data)
 
 static void cmd_complete_rsp(struct mgmt_pending_cmd *cmd, void *data)
 {
-	if (cmd->cmd_complete) {
-		u8 *status = data;
+	struct cmd_lookup *match = data;
 
-		cmd->cmd_complete(cmd, *status);
+	/* dequeue cmd_sync entries using cmd as data as that is about to be
+	 * removed/freed.
+	 */
+	hci_cmd_sync_dequeue(match->hdev, NULL, cmd, NULL);
+
+	if (cmd->cmd_complete) {
+		cmd->cmd_complete(cmd, match->mgmt_status);
 		mgmt_pending_remove(cmd);
 
 		return;
@@ -9394,12 +9399,12 @@  void mgmt_index_added(struct hci_dev *hdev)
 void mgmt_index_removed(struct hci_dev *hdev)
 {
 	struct mgmt_ev_ext_index ev;
-	u8 status = MGMT_STATUS_INVALID_INDEX;
+	struct cmd_lookup match = { NULL, hdev, MGMT_STATUS_INVALID_INDEX };
 
 	if (test_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks))
 		return;
 
-	mgmt_pending_foreach(0, hdev, cmd_complete_rsp, &status);
+	mgmt_pending_foreach(0, hdev, cmd_complete_rsp, &match);
 
 	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
 		mgmt_index_event(MGMT_EV_UNCONF_INDEX_REMOVED, hdev, NULL, 0,
@@ -9450,7 +9455,7 @@  void mgmt_power_on(struct hci_dev *hdev, int err)
 void __mgmt_power_off(struct hci_dev *hdev)
 {
 	struct cmd_lookup match = { NULL, hdev };
-	u8 status, zero_cod[] = { 0, 0, 0 };
+	u8 zero_cod[] = { 0, 0, 0 };
 
 	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
 
@@ -9462,11 +9467,11 @@  void __mgmt_power_off(struct hci_dev *hdev)
 	 * status responses.
 	 */
 	if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
-		status = MGMT_STATUS_INVALID_INDEX;
+		match.mgmt_status = MGMT_STATUS_INVALID_INDEX;
 	else
-		status = MGMT_STATUS_NOT_POWERED;
+		match.mgmt_status = MGMT_STATUS_NOT_POWERED;
 
-	mgmt_pending_foreach(0, hdev, cmd_complete_rsp, &status);
+	mgmt_pending_foreach(0, hdev, cmd_complete_rsp, &match);
 
 	if (memcmp(hdev->dev_class, zero_cod, sizeof(zero_cod)) != 0) {
 		mgmt_limited_event(MGMT_EV_CLASS_OF_DEV_CHANGED, hdev,