Re: [PATCH v4] media: siano: fix URB work teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Moving the buffer cleanup loop before cancel_work_sync() introduces a data race on surb->cb, causing Use-After-Free and NULL pointer dereference.
- [Critical] Data race on the usb_submit_urb() error path leads to a Double-Free list corruption.
--

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

media: siano: fix URB work teardown

This commit reorders the URB teardown sequence to kill URBs and release
buffers before cancelling the workqueue, aiming to prevent blocked workers.
It also adds an explicit error path to free buffers if urb submission fails.

Fixes: ebad8e731c1c ("media: usb: siano: Fix use after free bugs caused by do_submit_urb")
Reported-by: syzbot+0d6ef2b7ceb6014d756c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0d6ef2b7ceb6014d756c

> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index 0fdc2e0950b77..c941a05a41247 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c
[ ... ]
> @@ -168,31 +177,44 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
>  		smsusb_onresponse,
>  		surb
>  	);
> -	surb->urb->transfer_flags |= URB_FREE_BUFFER;
> +	rc = usb_submit_urb(surb->urb, GFP_ATOMIC);
> +	if (rc) {
> +		smscore_putbuffer(dev->coredev, surb->cb);
> +		surb->cb = NULL;
> +	}
>  
> -	return usb_submit_urb(surb->urb, GFP_ATOMIC);
> +	return rc;
>  }
>  
>  static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>  {
>  	int i;
>  
> -	for (i = 0; i < MAX_URBS; i++) {
> +	WRITE_ONCE(dev->streaming, false);
> +
> +	for (i = 0; i < MAX_URBS; i++)
>  		usb_kill_urb(dev->surbs[i].urb);
> -		if (dev->surbs[i].wq.func)
> -			cancel_work_sync(&dev->surbs[i].wq);
>  
> +	for (i = 0; i < MAX_URBS; i++) {
>  		if (dev->surbs[i].cb) {
>  			smscore_putbuffer(dev->coredev, dev->surbs[i].cb);
>  			dev->surbs[i].cb = NULL;
>  		}
>  	}
[Severity: Critical]
Does moving this buffer cleanup loop before cancel_work_sync() introduce a
data race with the worker thread?

If a worker thread is concurrently running smsusb_submit_urb(), there is no
lock protecting surb->cb. If the worker evaluates the following and is then
preempted:

smsusb_submit_urb() {
        if (!surb->cb) {
                ...
        }

The teardown thread can then concurrently reclaim the buffer and set
surb->cb = NULL. When the worker resumes, it accesses surb->cb->p in
usb_fill_bulk_urb(), which might cause a NULL pointer dereference.

If the pointer was cached in a register instead, could the worker submit an
URB using a buffer that was just returned to the free pool, resulting in a
use-after-free?

[Severity: Critical]
Could this reordering also cause a double-free of the buffer?

Because the worker thread now explicitly returns the buffer on submission
failure:

smsusb_submit_urb() {
        rc = usb_submit_urb(surb->urb, GFP_ATOMIC);
        if (rc) {
                smscore_putbuffer(dev->coredev, surb->cb);
                surb->cb = NULL;
        }
}

Both the worker thread and the teardown thread in smsusb_stop_streaming()
might concurrently observe dev->surbs[i].cb != NULL. They could then both
invoke smscore_putbuffer() on the exact same buffer.

Since smscore_putbuffer() blindly adds the buffer to the free list, might
this corrupt the list structure and cause a panic on the next allocation?

> +
> +	for (i = 0; i < MAX_URBS; i++)
> +		cancel_work_sync(&dev->surbs[i].wq);
> +
> +	for (i = 0; i < MAX_URBS; i++)
> +		usb_kill_urb(dev->surbs[i].urb);
>  }

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