linux-kernel - Re: [PATCH] usb: image: mdc800: kill download URB on timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63677ac4-76ba-4ed4-87cc-b2d3171f26cc@rowland.harvard.edu>
Date: Mon, 9 Feb 2026 09:44:17 -0500
From: Alan Stern <stern@...land.harvard.edu>
To: Ziyi Guo <n7l8m4@...orthwestern.edu>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: image: mdc800: kill download URB on timeout

On Mon, Feb 09, 2026 at 06:01:13AM +0000, Ziyi Guo wrote:
> mdc800_device_read() submits download_urb and waits for completion.
> If the timeout fires and the device has not responded, the function
> returns without killing the URB, leaving it active.
> 
> A subsequent read() resubmits the same URB while it is still
> in-flight, triggering the WARN in usb_submit_urb():
> 
>   "URB submitted while active"
> 
> Add usb_kill_urb() on the download timeout error path, mirroring
> how the write path in the same driver already handles its URB
> timeout (line 863).
> 
> Similar to
> - commit 372c93131998 ("USB: yurex: fix control-URB timeout handling")
> - commit b98d5000c505 ("media: rc: iguanair: handle timeouts")
> 
> Signed-off-by: Ziyi Guo <n7l8m4@...orthwestern.edu>
> ---
>  drivers/usb/image/mdc800.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/image/mdc800.c b/drivers/usb/image/mdc800.c
> index 7b7e1554ea20..c4df637b2a03 100644
> --- a/drivers/usb/image/mdc800.c
> +++ b/drivers/usb/image/mdc800.c
> @@ -736,6 +736,7 @@ static ssize_t mdc800_device_read (struct file *file, char __user *buf, size_t l
>  				mdc800->downloaded = 0;
>  				if (mdc800->download_urb->status != 0)
>  				{
> +					usb_kill_urb(mdc800->download_urb);
>  					dev_err(&mdc800->dev->dev,
>  						"request download-bytes fails "
>  						"(status=%i)\n",

This code is not correct because it doesn't check to see whether the URB 
completed normally or timed out.  The usb_kill_urb() call should not be 
issued if the URB completed normally.

Also, the code should not access mdc800->download_urb->status until 
after the URB completes.  The code should be structured like this:

			retval = wait_event_timeout(mdc800->download_wait,
				    mdc800->downloaded,
				    msecs_to_jiffies(TO_DOWNLOAD_GET_READY));
			if (!retval)
				usb_kill_urb(mdc800->download_urb);
			mdc800->downloaded = 0;
			if (mdc800->download_urb->status != 0) {
				...

Alan Stern