Re: [PATCH] usbip: fix use-after-free in event_handler()

[Shuah Khan]

On 8/3/26 09:05, syzbot wrote:
> From: Aleksandr Nogikh <nogikh@xxxxxxxxxx>
>
> The event_handler function in drivers/usb/usbip/usbip_event.c is a
> workqueue item responsible for processing events for a struct usbip_device.
> During device teardown, usbip_stop_eh() is called to wait for the event
> handler to finish processing. However, usbip_stop_eh() uses
> wait_event_interruptible() and ignores its return value. If the process
> unbinding the driver receives a signal, wait_event_interruptible() returns
> immediately, causing the teardown process to falsely assume the event
> handler has finished. 

Does this mean unbinding didn't happen?

The teardown process then proceeds to free the
> usbip_device memory. Meanwhile, the event_handler workqueue is still
> running and attempts to access the freed usbip_device, resulting in a KASAN
> slab-use-after-free crash.
>
> BUG: KASAN: slab-use-after-free in __mutex_lock_common
> kernel/locking/rtmutex_api.c:559 [inline]
> BUG: KASAN: slab-use-after-free in mutex_lock_nested+0x5a/0x1d0
> kernel/locking/rtmutex_api.c:578
> Read of size 1 at addr ffff8881145245b0 by task kworker/u8:5/6177
>
> Call Trace:
>   <TASK>
>   lock_acquire+0x84/0x350 kernel/locking/lockdep.c:5842
>   __mutex_lock_common kernel/locking/rtmutex_api.c:559 [inline]
>   mutex_lock_nested+0x5a/0x1d0 kernel/locking/rtmutex_api.c:578
>   event_handler+0x1e3/0x4a0 drivers/usb/usbip/usbip_event.c:73
>   process_one_work kernel/workqueue.c:3322 [inline]
>   process_scheduled_works+0xa8e/0x14e0 kernel/workqueue.c:3405
>   worker_thread+0xa47/0xfb0 kernel/workqueue.c:3486
>   kthread+0x388/0x470 kernel/kthread.c:436
>   ret_from_fork+0x514/0xb70 arch/x86/kernel/process.c:158
>   ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>   </TASK>
>
> To fix this, change wait_event_interruptible() to wait_event() in
> usbip_stop_eh(). This ensures that the teardown process strictly waits for
> the event handler to finish its execution and drop all references to the
> usbip_device before the memory is freed, preventing the use-after-free.
> This change is safe from deadlocks because usbip_stop_eh() is never called
> with locks held that the event_handler would need to acquire.

Correct - usbip_stop_eh() is called without lock hold after updating
the shutdown_busid status to true. However I am curious what happens
to the unbinding? Should usbip_stop_eh() check the return value of
wait_event_interruptible() and handle the error instead?

thanks,
-- Shuah