Re: [PATCH] media: usb: siano: initialize URB work once

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Unconditional cancel_work_sync() on uninitialized work items during device initialization error paths triggers a kernel warning (WARN_ON(!work->func)).

Pre-existing issues:
- [Critical] Slab corruption occurs during device teardown due to the `URB_FREE_BUFFER` flag being incorrectly set on an interior pointer.
- [Critical] A race condition between `usb_kill_urb()` and `cancel_work_sync()` allows an URB to be submitted after teardown has completed, leading to a Use-After-Free of the device.
--

commit e5db1a18a599b8f6972c2ba7a73f82dde8e97661
Author: Rohith Matam <rohithmatham@gmail.com>

media: usb: siano: initialize URB work once

This commit initializes each work item once when the URB is allocated,
and removes the conditional check before cancelling the work in
smsusb_stop_streaming().

> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index 0fdc2e0950b77..4e80ccc20e3eb 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c
[ ... ]
> @@ -179,8 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>  
>  	for (i = 0; i < MAX_URBS; i++) {
>  		usb_kill_urb(dev->surbs[i].urb);

[Severity: Critical]
This is a pre-existing issue, but does this sequence cause slab corruption
during device teardown?

In smsusb_submit_urb(), the URB's transfer_buffer is assigned to surb->cb->p,
and URB_FREE_BUFFER is added to transfer_flags. The pointer surb->cb->p
addresses an interior chunk inside a larger allocated block. During teardown,
smsusb_term_device() calls usb_free_urb(), which respects the URB_FREE_BUFFER
flag and executes kfree() on this interior pointer, causing slab corruption.

[Severity: Critical]
This is a pre-existing issue, but does this ordering allow an URB to be
submitted after teardown has completed?

If the do_submit_urb work is concurrently executing and blocked (for example
inside smscore_getbuffer()), usb_kill_urb() sees no active URB submission,
temporarily increments and then decrements urb->reject, and returns
immediately.

> -		if (dev->surbs[i].wq.func)
> -			cancel_work_sync(&dev->surbs[i].wq);
> +		cancel_work_sync(&dev->surbs[i].wq);

Next, cancel_work_sync() blocks waiting for the work to finish. The work
thread unblocks, successfully submits the URB via usb_submit_urb() (since
urb->reject is now 0), and exits. cancel_work_sync() then unblocks, teardown
completes, and the device structure is freed. However, the URB is actively
in-flight and will dereference the freed device structure in
smsusb_onresponse() upon completion.

[ ... ]
> @@ -471,6 +469,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>  		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
>  		if (!dev->surbs[i].urb)
>  			goto err_unregister_device;
> +		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
>  	}

[Severity: Medium]
Can this unconditional execution of cancel_work_sync() trigger a warning
splat in the error path?

By removing the if (dev->surbs[i].wq.func) check in smsusb_stop_streaming(),
uninitialized work items will now be passed to cancel_work_sync(). If
usb_alloc_urb() fails on an early loop iteration here, it jumps to teardown
before initializing the work structures for the remaining URBs.

When the teardown path calls smsusb_term_device() -> smsusb_stop_streaming(),
cancel_work_sync() is unconditionally called on these zero-filled structures,
which encounters a WARN_ON(!work->func) condition in __flush_work().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601061855.47423-1-rohithmatham@gmail.com?part=1