MIME-Version: 1.0
References: <CAAgLYK7qZAotVT5mQ_FjaO+RvG_z8HGezKbjeRrd_03ekyrjFA@mail.gmail.com>
In-Reply-To: <CAAgLYK7qZAotVT5mQ_FjaO+RvG_z8HGezKbjeRrd_03ekyrjFA@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 3 Mar 2023 10:09:34 -0800
Message-ID: <CABBYNZKr=TdMkEyqvaJ2pJaFtS+4unwVY7QotiJdWtZ5-8Es5w@mail.gmail.com>
Subject: Re: [PATCH 1/1] Bluetooth: fix race condition in hci_cmd_sync_clear
To:     lm0963 <lm0963hack@gmail.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>, davem@davemloft.net,
        edumazet@google.com, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, jkosina@suse.cz,
        hdegoede@redhat.com, david.rheinsberg@gmail.com,
        wsa+renesas@sang-engineering.com, linux@weissschuh.net,
        linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Min,

On Wed, Mar 1, 2023 at 11:21=E2=80=AFPM lm0963 <lm0963hack@gmail.com> 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@gmail.com>
> ---
>  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


--=20
Luiz Augusto von Dentz