| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABBYNZJcpgsPFsRLfyZ7iBvQC8oXRGnqOAnGY0uqMEsPD45DqQ@mail.gmail.com>
Date: Mon, 6 Jan 2025 16:27:37 -0500
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Mazin Al Haddad <mazin@...state.dev>
Cc: marcel@...tmann.org, johan.hedberg@...il.com,
linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
syzbot+479aff51bb361ef5aa18@...kaller.appspotmail.com
Subject: Re: [PATCH v2] bluetooth: hci: Fix UAF from MGMT_OP_REMOVE_ADV_MONITOR
during closure
Hi Mazin,
On Sat, Dec 21, 2024 at 12:06 PM Mazin Al Haddad <mazin@...state.dev> wrote:
>
> When hci_dev is closing down, mgmt_power_off will free parts of the device.
> The freed memory can then be accessed when processing pending
> MGMT_OP_REMOVE_ADV_MONITOR cmds. Since submitting the command is allowed
> when it is powered off (as in previous discussions linked below), fix
> this by returning MGMT_STATUS_BUSY to pending MGMT_OP_REMOVE_ADV_MONITOR
> operations submitted as hci_dev_close_sync is running. Avoid processing
> pending cmds since doing so will lead to reacquiring the same lock. Add
> a sanity check within mgmt_remove_adv_monitor to ensure the cmd is still
> valid and exit early if not.
>
> BUG: KASAN: slab-use-after-free in mgmt_remove_adv_monitor_sync+0x3a/0xd0
> net/bluetooth/mgmt.c:5543
> Read of size 8 at addr ffff88814128f898 by task kworker/u9:4/5961
>
> <TASK>
> __dump_stack lib/dump_stack.c:94 [inline]
> dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
> print_address_description mm/kasan/report.c:378 [inline]
> print_report+0x169/0x550 mm/kasan/report.c:489
> kasan_report+0x143/0x180 mm/kasan/report.c:602
> mgmt_remove_adv_monitor_sync+0x3a/0xd0 net/bluetooth/mgmt.c:5543
> hci_cmd_sync_work+0x22b/0x400 net/bluetooth/hci_sync.c:332
> process_one_work kernel/workqueue.c:3229 [inline]
> process_scheduled_works+0xa63/0x1850 kernel/workqueue.c:3310
> worker_thread+0x870/0xd30 kernel/workqueue.c:3391
> kthread+0x2f0/0x390 kernel/kthread.c:389
> ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> </TASK>
>
> Freed by task 16022:
> kasan_save_stack mm/kasan/common.c:47 [inline]
> kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
> kasan_save_free_info+0x40/0x50 mm/kasan/generic.c:582
> poison_slab_object mm/kasan/common.c:247 [inline]
> __kasan_slab_free+0x59/0x70 mm/kasan/common.c:264
> kasan_slab_free include/linux/kasan.h:233 [inline]
> slab_free_hook mm/slub.c:2338 [inline]
> slab_free mm/slub.c:4598 [inline]
> kfree+0x196/0x420 mm/slub.c:4746
> mgmt_pending_foreach+0xd1/0x130 net/bluetooth/mgmt_util.c:259
> __mgmt_power_off+0x183/0x430 net/bluetooth/mgmt.c:9550
> hci_dev_close_sync+0x6c4/0x11c0 net/bluetooth/hci_sync.c:5208
> hci_dev_do_close net/bluetooth/hci_core.c:483 [inline]
> hci_dev_close+0x112/0x210 net/bluetooth/hci_core.c:508
> sock_do_ioctl+0x158/0x460 net/socket.c:1209
> sock_ioctl+0x626/0x8e0 net/socket.c:1328
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:906 [inline]
> __se_sys_ioctl+0xf5/0x170 fs/ioctl.c:892
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Link: https://lore.kernel.org/lkml/20240424135903.24169-1-jlee@suse.com/
> Reported-by: syzbot+479aff51bb361ef5aa18@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=479aff51bb361ef5aa18
> Signed-off-by: Mazin Al Haddad <mazin@...state.dev>
Missing Fixes tag, Ive been adding it myself for some time now but you
better create the practice to include it so I don't have to go find
what commit introduced the problem to then do git log -1
--pretty=fixes and git commit --amend.
> ---
> Changes since v1:
> * Change return code to ECANCELED
> * Send out MGMT_STATUS_CANCELLED instead of MGMT_STATUS_BUSY
> * Style fixes
>
> net/bluetooth/hci_sync.c | 5 +++--
> net/bluetooth/mgmt.c | 20 ++++++++++++++++++--
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index c86f4e42e69c..aa5aa3fed32d 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5197,6 +5197,9 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> */
> drain_workqueue(hdev->workqueue);
>
> + /* flush cmd work */
> + flush_work(&hdev->cmd_work);
Might be a good idea to add a comment why it should be fine to flush
the command queue here, well in theory we could do immediately after
shutdown the driver since it shall no longer be processing commands,
in the other hand hdev is still considered running but perhaps that is
necessary in order to run things like hci_reset_sync but it doesn't
seem to be the case since that doesn't queue a command and instead it
send the command directly.
> hci_dev_lock(hdev);
>
> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> @@ -5234,8 +5237,6 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> clear_bit(HCI_INIT, &hdev->flags);
> }
>
> - /* flush cmd work */
> - flush_work(&hdev->cmd_work);
>
> /* Drop queues */
> skb_queue_purge(&hdev->rx_q);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index b31192d473d0..ec86ae851e56 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -5519,9 +5519,17 @@ static void mgmt_remove_adv_monitor_complete(struct hci_dev *hdev,
> {
> struct mgmt_rp_remove_adv_monitor rp;
> struct mgmt_pending_cmd *cmd = data;
> - struct mgmt_cp_remove_adv_monitor *cp = cmd->param;
> + struct mgmt_cp_remove_adv_monitor *cp;
> +
> + // if executing while device is closing down, status could
> + // be invalid as pending cmd could be removed by __mgmt_power_off
> + // so exit early if the device was busy.
We don't use C++ style comments in bluetooth.
> + if (status == -ECANCELED ||
> + cmd != pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev))
> + return;
>
> hci_dev_lock(hdev);
> + cp = cmd->param;
>
> rp.monitor_handle = cp->monitor_handle;
>
> @@ -5540,6 +5548,10 @@ static void mgmt_remove_adv_monitor_complete(struct hci_dev *hdev,
> static int mgmt_remove_adv_monitor_sync(struct hci_dev *hdev, void *data)
> {
> struct mgmt_pending_cmd *cmd = data;
> +
> + if (cmd != pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev))
> + return -ECANCELED;
> +
> struct mgmt_cp_remove_adv_monitor *cp = cmd->param;
> u16 handle = __le16_to_cpu(cp->monitor_handle);
>
> @@ -9544,8 +9556,12 @@ void __mgmt_power_off(struct hci_dev *hdev)
> */
> if (hci_dev_test_flag(hdev, HCI_UNREGISTER))
> match.mgmt_status = MGMT_STATUS_INVALID_INDEX;
> - else
CHECK: Unbalanced braces around else statement
#63: FILE: net/bluetooth/mgmt.c:9462:
+ else {
> + else {
> + match.mgmt_status = MGMT_STATUS_CANCELLED;
> + mgmt_pending_foreach(MGMT_OP_REMOVE_ADV_MONITOR, hdev,
> + cmd_status_rsp, &match);
> match.mgmt_status = MGMT_STATUS_NOT_POWERED;
> + }
>
> mgmt_pending_foreach(0, hdev, cmd_complete_rsp, &match);
>
>
> base-commit: 499551201b5f4fd3c0618a3e95e3d0d15ea18f31
> --
> 2.46.0
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists