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

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

[Rohith Matam]

smsusb_onresponse() reinitializes the URB work item immediately before
scheduling it. If teardown races with a queued work item,
cancel_work_sync() can observe workqueue state with WORK_STRUCT_PWQ
still set and trip the workqueue warning reported by syzbot.

Initialize each work item once when the URB is allocated, then schedule
and cancel that initialized work item for the lifetime of the URB. With
the work item always initialized, smsusb_stop_streaming() can cancel it
unconditionally.

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
Signed-off-by: Rohith Matam <rohithmatham@gmail.com>
---
 drivers/media/usb/siano/smsusb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 0fdc2e095..4e80ccc20 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -143,7 +143,6 @@ static void smsusb_onresponse(struct urb *urb)
 
 
 exit_and_resubmit:
-	INIT_WORK(&surb->wq, do_submit_urb);
 	schedule_work(&surb->wq);
 }
 
@@ -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);
-		if (dev->surbs[i].wq.func)
-			cancel_work_sync(&dev->surbs[i].wq);
+		cancel_work_sync(&dev->surbs[i].wq);
 
 		if (dev->surbs[i].cb) {
 			smscore_putbuffer(dev->coredev, dev->surbs[i].cb);
@@ -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);
 	}
 
 	pr_debug("smsusb_start_streaming(...).\n");
-- 
2.47.0

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

[sashiko-bot]

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

[PATCH v2] media: usb: siano: fix URB work teardown

[Rohith Matam]

smsusb_onresponse() reinitializes the URB work item immediately before
scheduling it. If teardown races with a queued work item,
cancel_work_sync() can observe workqueue state with WORK_STRUCT_PWQ
still set and trip the workqueue warning reported by syzbot.

The teardown path also has two related lifetime bugs: URB_FREE_BUFFER
makes USB core free a smscore-owned buffer, and a work item can submit
an URB after usb_kill_urb() has already returned.

Initialize each work item once before URB allocation, remove
URB_FREE_BUFFER, stop resubmission before killing URBs, and kill URBs
again after canceling work so any URB submitted by an already-running
worker is completed before buffers and the device are freed.

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
Signed-off-by: Rohith Matam <rohithmatham@gmail.com>
---
Changes in v2:
- Initialize all work items before allocating URBs.
- Remove URB_FREE_BUFFER from smscore-owned buffers.
- Stop resubmission before teardown and kill URBs again after canceling work.

 drivers/media/usb/siano/smsusb.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 0fdc2e095..e3ca51072 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -58,6 +58,7 @@ struct smsusb_device_t {
 	unsigned char in_ep;
 	unsigned char out_ep;
 	enum smsusb_state state;
+	bool streaming;
 };
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
@@ -72,7 +73,8 @@ static void do_submit_urb(struct work_struct *work)
 	struct smsusb_urb_t *surb = container_of(work, struct smsusb_urb_t, wq);
 	struct smsusb_device_t *dev = surb->dev;
 
-	smsusb_submit_urb(dev, surb);
+	if (READ_ONCE(dev->streaming))
+		smsusb_submit_urb(dev, surb);
 }
 
 /*
@@ -143,8 +145,8 @@ static void smsusb_onresponse(struct urb *urb)
 
 
 exit_and_resubmit:
-	INIT_WORK(&surb->wq, do_submit_urb);
-	schedule_work(&surb->wq);
+	if (READ_ONCE(dev->streaming))
+		schedule_work(&surb->wq);
 }
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
@@ -168,8 +170,6 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
 		smsusb_onresponse,
 		surb
 	);
-	surb->urb->transfer_flags |= URB_FREE_BUFFER;
-
 	return usb_submit_urb(surb->urb, GFP_ATOMIC);
 }
 
@@ -177,10 +177,12 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
 {
 	int 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);
+		cancel_work_sync(&dev->surbs[i].wq);
+		usb_kill_urb(dev->surbs[i].urb);
 
 		if (dev->surbs[i].cb) {
 			smscore_putbuffer(dev->coredev, dev->surbs[i].cb);
@@ -193,6 +195,8 @@ static int smsusb_start_streaming(struct smsusb_device_t *dev)
 {
 	int i, rc;
 
+	WRITE_ONCE(dev->streaming, true);
+
 	for (i = 0; i < MAX_URBS; i++) {
 		rc = smsusb_submit_urb(dev, &dev->surbs[i]);
 		if (rc < 0) {
@@ -468,6 +472,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 	/* initialize urbs */
 	for (i = 0; i < MAX_URBS; i++) {
 		dev->surbs[i].dev = dev;
+		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
+	}
+
+	for (i = 0; i < MAX_URBS; i++) {
 		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!dev->surbs[i].urb)
 			goto err_unregister_device;
-- 
2.54.0

Re: [PATCH v2] media: usb: siano: fix URB work teardown

[sashiko-bot]

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

New issues:
- [High] Calling `cancel_work_sync()` on zero-initialized `work_struct` triggers a kernel warning and potential oops on early initialization error paths.

Pre-existing issues:
- [High] Deadlock during USB disconnect due to mismatched `wait_event()` and `wake_up_interruptible()` in buffer management.
- [High] Ignored URB submission failure causes permanent resource loss and breaks the streaming pipeline.
--

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

media: usb: siano: fix URB work teardown

This patch reinitializes the URB work items once before URB allocation
and addresses teardown races that can trigger workqueue warnings. It also
adjusts the teardown sequence so that any URB submitted by an already-running
worker completes before buffers and the device are freed.

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..e3ca51072cfd5 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c

[ ... ]

> @@ -72,7 +73,8 @@ static void do_submit_urb(struct work_struct *work)
>  	struct smsusb_urb_t *surb = container_of(work, struct smsusb_urb_t, wq);
>  	struct smsusb_device_t *dev = surb->dev;
>  
> -	smsusb_submit_urb(dev, surb);
> +	if (READ_ONCE(dev->streaming))
> +		smsusb_submit_urb(dev, surb);

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
smsusb_submit_urb() here cause permanent resource loss?

If transient failures occur (e.g., -ENOMEM under memory pressure), the URB
is not queued to the USB core, and its completion handler
(smsusb_onresponse) will never trigger.

Because the URB worker is only rescheduled from the completion handler, the
URB and its associated buffer are permanently dropped from the streaming
pipeline. If this happens to all URBs, the receiver silently stalls.

>  }

[ ... ]

> @@ -177,10 +177,12 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>  {
>  	int 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);
> +		cancel_work_sync(&dev->surbs[i].wq);

[Severity: High]
Will this unconditional call to cancel_work_sync() execute on uninitialized
work items if smsusb_init_device() fails early?

If smsusb_init_device() fails during early initialization (e.g., due to
missing endpoints or smscore_register_device() returning an error), the
error path calls smsusb_term_device(), which invokes smsusb_stop_streaming().

Because this patch removed the safety check on wq.func and relocated the
INIT_WORK() loop further down in smsusb_init_device(), the work_struct
objects remain zero-initialized by kzalloc_obj().

Calling cancel_work_sync() unconditionally on a zero-initialized work_struct
modifies uninitialized fields and triggers a warning in __flush_work(),
leading to backtraces on device probe failure.

[Severity: High]
This is a pre-existing issue, but can this cancel_work_sync() hang forever
waiting for a deadlocked worker thread?

In drivers/media/common/siano/smscoreapi.c:smscore_getbuffer(), the code
uses wait_event() which puts the thread into TASK_UNINTERRUPTIBLE:

    wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));

However, smscore_putbuffer() signals waiters using wake_up_interruptible(),
which only wakes TASK_INTERRUPTIBLE tasks:

    wake_up_interruptible(&coredev->buffer_mng_waitq);

This mismatch means any worker thread blocked in smscore_getbuffer() will
hang permanently. During a USB disconnect or teardown, cancel_work_sync()
will wait forever for the deadlocked worker, resulting in an unrecoverable
hang of the USB hub thread.

> +		usb_kill_urb(dev->surbs[i].urb);
>  
>  		if (dev->surbs[i].cb) {
>  			smscore_putbuffer(dev->coredev, dev->surbs[i].cb);

[ ... ]

> @@ -468,6 +472,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>  	/* initialize urbs */
>  	for (i = 0; i < MAX_URBS; i++) {
>  		dev->surbs[i].dev = dev;
> +		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
> +	}
> +
> +	for (i = 0; i < MAX_URBS; i++) {
>  		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
>  		if (!dev->surbs[i].urb)
>  			goto err_unregister_device;

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

[PATCH v3] media: siano: fix URB work teardown

[Rohith Matam]

smsusb_onresponse() reinitializes the URB work item immediately before
scheduling it. If teardown races with a queued work item,
cancel_work_sync() can observe workqueue state with WORK_STRUCT_PWQ
still set and trip the workqueue warning reported by syzbot.

The teardown path also has related lifetime bugs. URB_FREE_BUFFER makes
USB core free a smscore-owned buffer, and a work item can submit an URB
after usb_kill_urb() has already returned. If that submission fails, the
worker keeps ownership of the smscore buffer and the streaming pipeline
loses an URB.

Initialize each work item before any error path can tear down the device,
remove URB_FREE_BUFFER, stop resubmission before killing URBs, and kill
URBs again after canceling work so any URB submitted by an already-running
worker is completed before buffers and the device are freed. Return the
smscore buffer if URB submission fails.

smscore_getbuffer() waits in TASK_UNINTERRUPTIBLE, so return buffers to
the pool before waking waiters and use wake_up() instead of
wake_up_interruptible().

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
Signed-off-by: Rohith Matam <rohithmatham@gmail.com>
---
Changes in v3:
- Initialize work items before early probe error paths can call teardown.
- Return the smscore buffer when URB submission fails.
- Wake buffer waiters after returning the buffer, and use wake_up().

Changes in v2:
- Initialize all work items before allocating URBs.
- Remove URB_FREE_BUFFER from smscore-owned buffers.
- Stop resubmission before teardown and kill URBs again after canceling work.

 drivers/media/common/siano/smscoreapi.c |  2 +-
 drivers/media/usb/siano/smsusb.c        | 37 +++++++++++++++++++------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c
index 017629e3c..e256344eb 100644
--- a/drivers/media/common/siano/smscoreapi.c
+++ b/drivers/media/common/siano/smscoreapi.c
@@ -1654,8 +1654,8 @@ EXPORT_SYMBOL_GPL(smscore_getbuffer);
  */
 void smscore_putbuffer(struct smscore_device_t *coredev,
 		struct smscore_buffer_t *cb) {
-	wake_up_interruptible(&coredev->buffer_mng_waitq);
 	list_add_locked(&cb->entry, &coredev->buffers, &coredev->bufferslock);
+	wake_up(&coredev->buffer_mng_waitq);
 }
 EXPORT_SYMBOL_GPL(smscore_putbuffer);
 
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 0fdc2e095..fab64e5fa 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -58,6 +58,7 @@ struct smsusb_device_t {
 	unsigned char in_ep;
 	unsigned char out_ep;
 	enum smsusb_state state;
+	bool streaming;
 };
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
@@ -71,8 +72,14 @@ static void do_submit_urb(struct work_struct *work)
 {
 	struct smsusb_urb_t *surb = container_of(work, struct smsusb_urb_t, wq);
 	struct smsusb_device_t *dev = surb->dev;
+	int rc;
 
-	smsusb_submit_urb(dev, surb);
+	if (!READ_ONCE(dev->streaming))
+		return;
+
+	rc = smsusb_submit_urb(dev, surb);
+	if (rc < 0 && READ_ONCE(dev->streaming))
+		pr_err("smsusb_submit_urb(...) failed\n");
 }
 
 /*
@@ -143,13 +150,15 @@ static void smsusb_onresponse(struct urb *urb)
 
 
 exit_and_resubmit:
-	INIT_WORK(&surb->wq, do_submit_urb);
-	schedule_work(&surb->wq);
+	if (READ_ONCE(dev->streaming))
+		schedule_work(&surb->wq);
 }
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
 			     struct smsusb_urb_t *surb)
 {
+	int rc;
+
 	if (!surb->cb) {
 		/* This function can sleep */
 		surb->cb = smscore_getbuffer(dev->coredev);
@@ -168,19 +177,25 @@ 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;
 
+	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);
+		cancel_work_sync(&dev->surbs[i].wq);
+		usb_kill_urb(dev->surbs[i].urb);
 
 		if (dev->surbs[i].cb) {
 			smscore_putbuffer(dev->coredev, dev->surbs[i].cb);
@@ -193,6 +208,8 @@ static int smsusb_start_streaming(struct smsusb_device_t *dev)
 {
 	int i, rc;
 
+	WRITE_ONCE(dev->streaming, true);
+
 	for (i = 0; i < MAX_URBS; i++) {
 		rc = smsusb_submit_urb(dev, &dev->surbs[i]);
 		if (rc < 0) {
@@ -401,6 +418,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 	if (!dev)
 		return -ENOMEM;
 
+	for (i = 0; i < MAX_URBS; i++) {
+		dev->surbs[i].dev = dev;
+		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
+	}
+
 	memset(&params, 0, sizeof(params));
 	usb_set_intfdata(intf, dev);
 	dev->udev = interface_to_usbdev(intf);
@@ -467,7 +489,6 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 
 	/* initialize urbs */
 	for (i = 0; i < MAX_URBS; i++) {
-		dev->surbs[i].dev = dev;
 		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!dev->surbs[i].urb)
 			goto err_unregister_device;
-- 
2.54.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential deadlock during device teardown due to unkillable wait in smscore_getbuffer().
--

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

media: siano: fix URB work teardown

This commit reinitializes URB work items before scheduling to prevent workqueue
warnings during teardown races. It also removes URB_FREE_BUFFER to fix buffer
lifetime issues and adjusts buffer management wakeups.

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..fab64e5fa6804 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c

[ ... ]

> @@ -183,12 +192,15 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>  {
>  	int 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);
> +		cancel_work_sync(&dev->surbs[i].wq);

[Severity: High]
This is a pre-existing issue, but can this sequence cause a deadlock during
teardown?

When smsusb_stop_streaming is called, it iterates over the URBs and calls
cancel_work_sync sequentially. If a worker thread executing do_submit_urb is
concurrently blocked inside smscore_getbuffer waiting for a buffer, it waits
in TASK_UNINTERRUPTIBLE without an abort condition:

drivers/media/common/siano/smscoreapi.c:smscore_getbuffer() {
        struct smscore_buffer_t *cb = NULL;

        wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));

        return cb;
}

Because cancel_work_sync is waiting for this worker to finish, the teardown
loop halts and cannot proceed to subsequent URBs to release their buffers
back to the pool via smscore_putbuffer.

Since no more buffers will be returned during teardown, does the worker
sleep forever, causing cancel_work_sync to hang indefinitely?

> +		usb_kill_urb(dev->surbs[i].urb);
>  
>  		if (dev->surbs[i].cb) {
>  			smscore_putbuffer(dev->coredev, dev->surbs[i].cb);

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

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

[Rohith Matam]

smsusb_onresponse() reinitializes the URB work item immediately before
scheduling it. If teardown races with a queued work item,
cancel_work_sync() can observe workqueue state with WORK_STRUCT_PWQ
still set and trip the workqueue warning reported by syzbot.

The teardown path also has related lifetime bugs. URB_FREE_BUFFER makes
USB core free a smscore-owned buffer, and a work item can submit an URB
after usb_kill_urb() has already returned. If that submission fails, the
worker keeps ownership of the smscore buffer and the streaming pipeline
loses an URB.

Initialize each work item before any error path can tear down the device,
remove URB_FREE_BUFFER, stop resubmission before killing URBs, and kill
URBs again after canceling work so any URB submitted by an already-running
worker is completed before buffers and the device are freed. Return the
smscore buffer if URB submission fails.

During teardown, kill all URBs first and return all held buffers before
canceling work so a worker blocked in smscore_getbuffer() can make
progress and exit. smscore_getbuffer() waits in TASK_UNINTERRUPTIBLE, so
return buffers to the pool before waking waiters and use wake_up()
instead of wake_up_interruptible().

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
Signed-off-by: Rohith Matam <rohithmatham@gmail.com>
---
Changes in v4:
- During teardown, kill all URBs and return held buffers before canceling
  workers so smscore_getbuffer() waiters can wake and exit.

Changes in v3:
- Initialize work items before early probe error paths can call teardown.
- Return the smscore buffer when URB submission fails.
- Wake buffer waiters after returning the buffer, and use wake_up().

Changes in v2:
- Initialize all work items before allocating URBs.
- Remove URB_FREE_BUFFER from smscore-owned buffers.
- Stop resubmission before teardown and kill URBs again after canceling work.

 drivers/media/common/siano/smscoreapi.c |  2 +-
 drivers/media/usb/siano/smsusb.c        | 44 ++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c
index 017629e3c..e256344eb 100644
--- a/drivers/media/common/siano/smscoreapi.c
+++ b/drivers/media/common/siano/smscoreapi.c
@@ -1654,8 +1654,8 @@ EXPORT_SYMBOL_GPL(smscore_getbuffer);
  */
 void smscore_putbuffer(struct smscore_device_t *coredev,
 		struct smscore_buffer_t *cb) {
-	wake_up_interruptible(&coredev->buffer_mng_waitq);
 	list_add_locked(&cb->entry, &coredev->buffers, &coredev->bufferslock);
+	wake_up(&coredev->buffer_mng_waitq);
 }
 EXPORT_SYMBOL_GPL(smscore_putbuffer);
 
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 0fdc2e095..c941a05a4 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -58,6 +58,7 @@ struct smsusb_device_t {
 	unsigned char in_ep;
 	unsigned char out_ep;
 	enum smsusb_state state;
+	bool streaming;
 };
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
@@ -71,8 +72,14 @@ static void do_submit_urb(struct work_struct *work)
 {
 	struct smsusb_urb_t *surb = container_of(work, struct smsusb_urb_t, wq);
 	struct smsusb_device_t *dev = surb->dev;
+	int rc;
 
-	smsusb_submit_urb(dev, surb);
+	if (!READ_ONCE(dev->streaming))
+		return;
+
+	rc = smsusb_submit_urb(dev, surb);
+	if (rc < 0 && READ_ONCE(dev->streaming))
+		pr_err("smsusb_submit_urb(...) failed\n");
 }
 
 /*
@@ -143,13 +150,15 @@ static void smsusb_onresponse(struct urb *urb)
 
 
 exit_and_resubmit:
-	INIT_WORK(&surb->wq, do_submit_urb);
-	schedule_work(&surb->wq);
+	if (READ_ONCE(dev->streaming))
+		schedule_work(&surb->wq);
 }
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
 			     struct smsusb_urb_t *surb)
 {
+	int rc;
+
 	if (!surb->cb) {
 		/* This function can sleep */
 		surb->cb = smscore_getbuffer(dev->coredev);
@@ -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;
 		}
 	}
+
+	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);
 }
 
 static int smsusb_start_streaming(struct smsusb_device_t *dev)
 {
 	int i, rc;
 
+	WRITE_ONCE(dev->streaming, true);
+
 	for (i = 0; i < MAX_URBS; i++) {
 		rc = smsusb_submit_urb(dev, &dev->surbs[i]);
 		if (rc < 0) {
@@ -401,6 +423,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 	if (!dev)
 		return -ENOMEM;
 
+	for (i = 0; i < MAX_URBS; i++) {
+		dev->surbs[i].dev = dev;
+		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
+	}
+
 	memset(&params, 0, sizeof(params));
 	usb_set_intfdata(intf, dev);
 	dev->udev = interface_to_usbdev(intf);
@@ -467,7 +494,6 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 
 	/* initialize urbs */
 	for (i = 0; i < MAX_URBS; i++) {
-		dev->surbs[i].dev = dev;
 		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!dev->surbs[i].urb)
 			goto err_unregister_device;
-- 
2.54.0

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

[sashiko-bot]

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

[PATCH v5] media: siano: fix URB work teardown

[Rohith Matam]

smsusb_onresponse() reinitializes the URB work item immediately before
scheduling it. If teardown races with a queued work item,
cancel_work_sync() can observe workqueue state with WORK_STRUCT_PWQ
still set and trip the workqueue warning reported by syzbot.

The teardown path also has related lifetime bugs. URB_FREE_BUFFER makes
USB core free a smscore-owned buffer, and a work item can submit an URB
after usb_kill_urb() has already returned. If that submission fails, the
worker keeps ownership of the smscore buffer and the streaming pipeline
loses an URB.

Initialize each work item before any error path can tear down the device,
remove URB_FREE_BUFFER, stop resubmission before killing URBs, and kill
URBs again after canceling work so any URB submitted by an already-running
worker is completed before buffers and the device are freed. Return the
smscore buffer if URB submission fails.

During teardown, wake workers blocked in smscore_getbuffer() by adding an
abortable buffer wait. Then cancel work before returning any URB-owned
buffer to avoid racing with workers that are still using surb->cb.

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
Signed-off-by: Rohith Matam <rohithmatham@gmail.com>
---
Changes in v5:
- Replace the v4 early-buffer-return ordering with an abortable smscore
  buffer wait so workers can exit before teardown returns surb->cb.

Changes in v4:
- During teardown, kill all URBs and return held buffers before canceling
  workers so smscore_getbuffer() waiters can wake and exit.

Changes in v3:
- Initialize work items before early probe error paths can call teardown.
- Return the smscore buffer when URB submission fails.
- Wake buffer waiters after returning the buffer, and use wake_up().

Changes in v2:
- Initialize all work items before allocating URBs.
- Remove URB_FREE_BUFFER from smscore-owned buffers.
- Stop resubmission before teardown and kill URBs again after canceling work.

 drivers/media/common/siano/smscoreapi.c | 14 ++++++-
 drivers/media/common/siano/smscoreapi.h |  2 +
 drivers/media/usb/siano/smsusb.c        | 55 +++++++++++++++++++------
 3 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c
index 017629e3c..23be7347d 100644
--- a/drivers/media/common/siano/smscoreapi.c
+++ b/drivers/media/common/siano/smscoreapi.c
@@ -1644,6 +1644,18 @@ struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
 }
 EXPORT_SYMBOL_GPL(smscore_getbuffer);
 
+struct smscore_buffer_t *
+smscore_getbuffer_abortable(struct smscore_device_t *coredev, bool *abort)
+{
+	struct smscore_buffer_t *cb = NULL;
+
+	wait_event(coredev->buffer_mng_waitq,
+		   READ_ONCE(*abort) || (cb = get_entry(coredev)));
+
+	return cb;
+}
+EXPORT_SYMBOL_GPL(smscore_getbuffer_abortable);
+
 /*
  * return buffer descriptor to a pool
  *
@@ -1654,8 +1666,8 @@ EXPORT_SYMBOL_GPL(smscore_getbuffer);
  */
 void smscore_putbuffer(struct smscore_device_t *coredev,
 		struct smscore_buffer_t *cb) {
-	wake_up_interruptible(&coredev->buffer_mng_waitq);
 	list_add_locked(&cb->entry, &coredev->buffers, &coredev->bufferslock);
+	wake_up(&coredev->buffer_mng_waitq);
 }
 EXPORT_SYMBOL_GPL(smscore_putbuffer);
 
diff --git a/drivers/media/common/siano/smscoreapi.h b/drivers/media/common/siano/smscoreapi.h
index d945a2d6d..f8a9e6ef7 100644
--- a/drivers/media/common/siano/smscoreapi.h
+++ b/drivers/media/common/siano/smscoreapi.h
@@ -1117,6 +1117,8 @@ extern void smscore_onresponse(struct smscore_device_t *coredev,
 
 extern
 struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev);
+struct smscore_buffer_t *
+smscore_getbuffer_abortable(struct smscore_device_t *coredev, bool *abort);
 extern void smscore_putbuffer(struct smscore_device_t *coredev,
 			      struct smscore_buffer_t *cb);
 
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 0fdc2e095..565543dbd 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -58,6 +58,8 @@ struct smsusb_device_t {
 	unsigned char in_ep;
 	unsigned char out_ep;
 	enum smsusb_state state;
+	bool streaming;
+	bool stopping;
 };
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
@@ -71,8 +73,14 @@ static void do_submit_urb(struct work_struct *work)
 {
 	struct smsusb_urb_t *surb = container_of(work, struct smsusb_urb_t, wq);
 	struct smsusb_device_t *dev = surb->dev;
+	int rc;
 
-	smsusb_submit_urb(dev, surb);
+	if (!READ_ONCE(dev->streaming))
+		return;
+
+	rc = smsusb_submit_urb(dev, surb);
+	if (rc < 0 && READ_ONCE(dev->streaming))
+		pr_err("smsusb_submit_urb(...) failed\n");
 }
 
 /*
@@ -143,19 +151,22 @@ static void smsusb_onresponse(struct urb *urb)
 
 
 exit_and_resubmit:
-	INIT_WORK(&surb->wq, do_submit_urb);
-	schedule_work(&surb->wq);
+	if (READ_ONCE(dev->streaming))
+		schedule_work(&surb->wq);
 }
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
 			     struct smsusb_urb_t *surb)
 {
+	int rc;
+
 	if (!surb->cb) {
 		/* This function can sleep */
-		surb->cb = smscore_getbuffer(dev->coredev);
+		surb->cb = smscore_getbuffer_abortable(dev->coredev,
+						       &dev->stopping);
 		if (!surb->cb) {
-			pr_err("smscore_getbuffer(...) returned NULL\n");
-			return -ENOMEM;
+			pr_debug("device is stopping\n");
+			return -ESHUTDOWN;
 		}
 	}
 
@@ -168,20 +179,33 @@ 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);
+	WRITE_ONCE(dev->stopping, true);
+	wake_up(&dev->coredev->buffer_mng_waitq);
+
+	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++)
+		cancel_work_sync(&dev->surbs[i].wq);
+
+	for (i = 0; i < MAX_URBS; i++)
+		usb_kill_urb(dev->surbs[i].urb);
+
+	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;
@@ -193,6 +217,9 @@ static int smsusb_start_streaming(struct smsusb_device_t *dev)
 {
 	int i, rc;
 
+	WRITE_ONCE(dev->stopping, false);
+	WRITE_ONCE(dev->streaming, true);
+
 	for (i = 0; i < MAX_URBS; i++) {
 		rc = smsusb_submit_urb(dev, &dev->surbs[i]);
 		if (rc < 0) {
@@ -401,6 +428,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 	if (!dev)
 		return -ENOMEM;
 
+	for (i = 0; i < MAX_URBS; i++) {
+		dev->surbs[i].dev = dev;
+		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
+	}
+
 	memset(&params, 0, sizeof(params));
 	usb_set_intfdata(intf, dev);
 	dev->udev = interface_to_usbdev(intf);
@@ -467,7 +499,6 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 
 	/* initialize urbs */
 	for (i = 0; i < MAX_URBS; i++) {
-		dev->surbs[i].dev = dev;
 		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!dev->surbs[i].urb)
 			goto err_unregister_device;
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional dereference of `dev->coredev` in `smsusb_stop_streaming()` during early initialization failure paths causes a kernel panic.
--

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

media: siano: fix URB work teardown

This commit reinitializes URB work items before scheduling to avoid
workqueue warnings on teardown. It also fixes related lifetime bugs by
adding an abortable buffer wait and ensuring any URB submitted by an
already-running worker is completed before freeing resources.

> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index 0fdc2e0950b77..565543dbd4cb1 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c

[ ... ]

> @@ -168,20 +179,33 @@ 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);
> +	WRITE_ONCE(dev->stopping, true);
> +	wake_up(&dev->coredev->buffer_mng_waitq);

[Severity: High]
Can this unconditionally dereference dev->coredev when it is NULL?

If smsusb_init_device() fails its early endpoint checks, it bails out by
calling smsusb_term_device():

drivers/media/usb/siano/smsusb.c:smsusb_init_device() {
...
	if (!dev->in_ep || !dev->out_ep || align < 0) {  /* Missing endpoints? */
		smsusb_term_device(intf);
		return -ENODEV;
	}
...
}

Because smsusb_term_device() unconditionally calls smsusb_stop_streaming(),
and dev->coredev hasn't been initialized yet during this early error path,
could this new wake_up() call trigger a NULL pointer dereference?

> +
> +	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);
>  

[ ... ]

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

[PATCH v6] media: siano: fix URB work teardown

[Rohith Matam]

smsusb_onresponse() reinitializes the URB work item immediately before
scheduling it. If teardown races with a queued work item,
cancel_work_sync() can observe workqueue state with WORK_STRUCT_PWQ
still set and trip the workqueue warning reported by syzbot.

The teardown path also has related lifetime bugs. URB_FREE_BUFFER makes
USB core free a smscore-owned buffer, and a work item can submit an URB
after usb_kill_urb() has already returned. If that submission fails, the
worker keeps ownership of the smscore buffer and the streaming pipeline
loses an URB.

Initialize each work item before any error path can tear down the device,
remove URB_FREE_BUFFER, stop resubmission before killing URBs, and kill
URBs again after canceling work so any URB submitted by an already-running
worker is completed before buffers and the device are freed. Return the
smscore buffer if URB submission fails.

During teardown, wake workers blocked in smscore_getbuffer() by adding an
abortable buffer wait. Then cancel work before returning any URB-owned
buffer to avoid racing with workers that are still using surb->cb. Guard
the wakeup because early probe failure can call teardown before coredev is
registered.

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
Signed-off-by: Rohith Matam <rohithmatham@gmail.com>
---
Changes in v6:
- Guard the buffer waitqueue wakeup for early probe failures before
  coredev has been registered.

Changes in v5:
- Replace the v4 early-buffer-return ordering with an abortable smscore
  buffer wait so workers can exit before teardown returns surb->cb.

Changes in v4:
- During teardown, kill all URBs and return held buffers before canceling
  workers so smscore_getbuffer() waiters can wake and exit.

Changes in v3:
- Initialize work items before early probe error paths can call teardown.
- Return the smscore buffer when URB submission fails.
- Wake buffer waiters after returning the buffer, and use wake_up().

Changes in v2:
- Initialize all work items before allocating URBs.
- Remove URB_FREE_BUFFER from smscore-owned buffers.
- Stop resubmission before teardown and kill URBs again after canceling work.

 drivers/media/common/siano/smscoreapi.c | 14 ++++++-
 drivers/media/common/siano/smscoreapi.h |  2 +
 drivers/media/usb/siano/smsusb.c        | 56 +++++++++++++++++++------
 3 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/drivers/media/common/siano/smscoreapi.c b/drivers/media/common/siano/smscoreapi.c
index 017629e3c..23be7347d 100644
--- a/drivers/media/common/siano/smscoreapi.c
+++ b/drivers/media/common/siano/smscoreapi.c
@@ -1644,6 +1644,18 @@ struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev)
 }
 EXPORT_SYMBOL_GPL(smscore_getbuffer);
 
+struct smscore_buffer_t *
+smscore_getbuffer_abortable(struct smscore_device_t *coredev, bool *abort)
+{
+	struct smscore_buffer_t *cb = NULL;
+
+	wait_event(coredev->buffer_mng_waitq,
+		   READ_ONCE(*abort) || (cb = get_entry(coredev)));
+
+	return cb;
+}
+EXPORT_SYMBOL_GPL(smscore_getbuffer_abortable);
+
 /*
  * return buffer descriptor to a pool
  *
@@ -1654,8 +1666,8 @@ EXPORT_SYMBOL_GPL(smscore_getbuffer);
  */
 void smscore_putbuffer(struct smscore_device_t *coredev,
 		struct smscore_buffer_t *cb) {
-	wake_up_interruptible(&coredev->buffer_mng_waitq);
 	list_add_locked(&cb->entry, &coredev->buffers, &coredev->bufferslock);
+	wake_up(&coredev->buffer_mng_waitq);
 }
 EXPORT_SYMBOL_GPL(smscore_putbuffer);
 
diff --git a/drivers/media/common/siano/smscoreapi.h b/drivers/media/common/siano/smscoreapi.h
index d945a2d6d..f8a9e6ef7 100644
--- a/drivers/media/common/siano/smscoreapi.h
+++ b/drivers/media/common/siano/smscoreapi.h
@@ -1117,6 +1117,8 @@ extern void smscore_onresponse(struct smscore_device_t *coredev,
 
 extern
 struct smscore_buffer_t *smscore_getbuffer(struct smscore_device_t *coredev);
+struct smscore_buffer_t *
+smscore_getbuffer_abortable(struct smscore_device_t *coredev, bool *abort);
 extern void smscore_putbuffer(struct smscore_device_t *coredev,
 			      struct smscore_buffer_t *cb);
 
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 0fdc2e095..98ae94e19 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -58,6 +58,8 @@ struct smsusb_device_t {
 	unsigned char in_ep;
 	unsigned char out_ep;
 	enum smsusb_state state;
+	bool streaming;
+	bool stopping;
 };
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
@@ -71,8 +73,14 @@ static void do_submit_urb(struct work_struct *work)
 {
 	struct smsusb_urb_t *surb = container_of(work, struct smsusb_urb_t, wq);
 	struct smsusb_device_t *dev = surb->dev;
+	int rc;
 
-	smsusb_submit_urb(dev, surb);
+	if (!READ_ONCE(dev->streaming))
+		return;
+
+	rc = smsusb_submit_urb(dev, surb);
+	if (rc < 0 && READ_ONCE(dev->streaming))
+		pr_err("smsusb_submit_urb(...) failed\n");
 }
 
 /*
@@ -143,19 +151,22 @@ static void smsusb_onresponse(struct urb *urb)
 
 
 exit_and_resubmit:
-	INIT_WORK(&surb->wq, do_submit_urb);
-	schedule_work(&surb->wq);
+	if (READ_ONCE(dev->streaming))
+		schedule_work(&surb->wq);
 }
 
 static int smsusb_submit_urb(struct smsusb_device_t *dev,
 			     struct smsusb_urb_t *surb)
 {
+	int rc;
+
 	if (!surb->cb) {
 		/* This function can sleep */
-		surb->cb = smscore_getbuffer(dev->coredev);
+		surb->cb = smscore_getbuffer_abortable(dev->coredev,
+						       &dev->stopping);
 		if (!surb->cb) {
-			pr_err("smscore_getbuffer(...) returned NULL\n");
-			return -ENOMEM;
+			pr_debug("device is stopping\n");
+			return -ESHUTDOWN;
 		}
 	}
 
@@ -168,20 +179,34 @@ 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);
+	WRITE_ONCE(dev->stopping, true);
+	if (dev->coredev)
+		wake_up(&dev->coredev->buffer_mng_waitq);
+
+	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++)
+		cancel_work_sync(&dev->surbs[i].wq);
+
+	for (i = 0; i < MAX_URBS; i++)
+		usb_kill_urb(dev->surbs[i].urb);
+
+	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;
@@ -193,6 +218,9 @@ static int smsusb_start_streaming(struct smsusb_device_t *dev)
 {
 	int i, rc;
 
+	WRITE_ONCE(dev->stopping, false);
+	WRITE_ONCE(dev->streaming, true);
+
 	for (i = 0; i < MAX_URBS; i++) {
 		rc = smsusb_submit_urb(dev, &dev->surbs[i]);
 		if (rc < 0) {
@@ -401,6 +429,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 	if (!dev)
 		return -ENOMEM;
 
+	for (i = 0; i < MAX_URBS; i++) {
+		dev->surbs[i].dev = dev;
+		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
+	}
+
 	memset(&params, 0, sizeof(params));
 	usb_set_intfdata(intf, dev);
 	dev->udev = interface_to_usbdev(intf);
@@ -467,7 +500,6 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
 
 	/* initialize urbs */
 	for (i = 0; i < MAX_URBS; i++) {
-		dev->surbs[i].dev = dev;
 		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!dev->surbs[i].urb)
 			goto err_unregister_device;
-- 
2.54.0