Re: [PATCH 1/1] Bluetooth: fix race condition in hci_cmd_sync_clear

[Luiz Augusto von Dentz]

Hi Min,

On Wed, Mar 1, 2023 at 11:21 PM lm0963 <lm0963hack@xxxxxxxxx> wrote:
>
> There is a potential race condition in hci_cmd_sync_work and
> hci_cmd_sync_clear, and could lead to use-after-free. For instance,
> hci_cmd_sync_work is added to the 'req_workqueue' after cancel_work_sync
> The entry of 'cmd_sync_work_list' may be freed in hci_cmd_sync_clear, and
> causing kernel panic when it is used in hci_cmd_sync_work.
>
> Here's the call trace:
>
> dump_stack_lvl+0x49/0x63
> print_report.cold+0x5e/0x5d3
> ? hci_cmd_sync_work+0x282/0x320
> kasan_report+0xaa/0x120
> ? hci_cmd_sync_work+0x282/0x320
> __asan_report_load8_noabort+0x14/0x20
> hci_cmd_sync_work+0x282/0x320
> process_one_work+0x77b/0x11c0
> ? _raw_spin_lock_irq+0x8e/0xf0
> worker_thread+0x544/0x1180
> ? poll_idle+0x1e0/0x1e0
> kthread+0x285/0x320
> ? process_one_work+0x11c0/0x11c0
> ? kthread_complete_and_exit+0x30/0x30
> ret_from_fork+0x22/0x30
> </TASK>
>
> Allocated by task 266:
> kasan_save_stack+0x26/0x50
> __kasan_kmalloc+0xae/0xe0
> kmem_cache_alloc_trace+0x191/0x350
> hci_cmd_sync_queue+0x97/0x2b0
> hci_update_passive_scan+0x176/0x1d0
> le_conn_complete_evt+0x1b5/0x1a00
> hci_le_conn_complete_evt+0x234/0x340
> hci_le_meta_evt+0x231/0x4e0
> hci_event_packet+0x4c5/0xf00
> hci_rx_work+0x37d/0x880
> process_one_work+0x77b/0x11c0
> worker_thread+0x544/0x1180
> kthread+0x285/0x320
> ret_from_fork+0x22/0x30
>
> Freed by task 269:
> kasan_save_stack+0x26/0x50
> kasan_set_track+0x25/0x40
> kasan_set_free_info+0x24/0x40
> ____kasan_slab_free+0x176/0x1c0
> __kasan_slab_free+0x12/0x20
> slab_free_freelist_hook+0x95/0x1a0
> kfree+0xba/0x2f0
> hci_cmd_sync_clear+0x14c/0x210
> hci_unregister_dev+0xff/0x440
> vhci_release+0x7b/0xf0
> __fput+0x1f3/0x970
> ____fput+0xe/0x20
> task_work_run+0xd4/0x160
> do_exit+0x8b0/0x22a0
> do_group_exit+0xba/0x2a0
> get_signal+0x1e4a/0x25b0
> arch_do_signal_or_restart+0x93/0x1f80
> exit_to_user_mode_prepare+0xf5/0x1a0
> syscall_exit_to_user_mode+0x26/0x50
> ret_from_fork+0x15/0x30
>
> Signed-off-by: Min Li <lm0963hack@xxxxxxxxx>
> ---
>  net/bluetooth/hci_sync.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 117eedb6f709..3103daf49d63 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -643,6 +643,7 @@ void hci_cmd_sync_clear(struct hci_dev *hdev)
>   cancel_work_sync(&hdev->cmd_sync_work);
>   cancel_work_sync(&hdev->reenable_adv_work);
>
> + mutex_lock(&hdev->cmd_sync_work_lock);
>   list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list) {
>   if (entry->destroy)
>   entry->destroy(hdev, entry->data, -ECANCELED);
> @@ -650,6 +651,7 @@ void hci_cmd_sync_clear(struct hci_dev *hdev)
>   list_del(&entry->list);
>   kfree(entry);
>   }
> + mutex_unlock(&hdev->cmd_sync_work_lock);
>  }

The code style of this one seems broken, did you generate it using git
format-patch?

>  void __hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
> --
> 2.25.1



-- 
Luiz Augusto von Dentz