Re: [PATCH 1/2] usbcore: Reject excessively long uninterruptible timeouts

[Oliver Neukum <oneukum@suse.com>]

On 09.02.26 20:59, Alan Stern wrote:

> Note that this will affect the timeouts accepted by the
> USBDEVFS_CONTROL and USBDEVFS_BULK ioctls in usbfs, since they rely on
> the synchronous message API.

That means you cannot just change the semantics of the call.

> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Link: https://lore.kernel.org/linux-usb/3acfe838-6334-4f6d-be7c-4bb01704b33d@rowland.harvard.edu/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> CC: stable@vger.kernel.org
> 
> ---
> 
>   drivers/usb/core/message.c |   23 ++++++++++++++---------
>   include/linux/usb.h        |    3 +++
>   2 files changed, 17 insertions(+), 9 deletions(-)
> 
> Index: usb-devel/drivers/usb/core/message.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/message.c
> +++ usb-devel/drivers/usb/core/message.c
> @@ -46,6 +46,9 @@ static void usb_api_blocking_completion(
>    * is NOT interruptible. Many device driver i/o requests should be
>    * interruptible and therefore these drivers should implement their
>    * own interruptible routines.
> + *
> + * Because the wait is uninterruptible, we enforce a maximum limit on
> + * the length of the timeout.
>    */
>   static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)

If you are touching this at all, we might look into the fundamental question
of why a timeout can be negative at all.

>   {
> @@ -56,11 +59,17 @@ static int usb_start_wait_urb(struct urb
>   	init_completion(&ctx.done);
>   	urb->context = &ctx;
>   	urb->actual_length = 0;
> +
> +	if (timeout <= 0 || timeout > USB_MAX_SYNCHRONOUS_TIMEOUT) {

You are rejecting values that used to be acceptable. That is unavoidable,
but somebody was surely foolish enough to pass 0 from user space in every
case and got away with it until now.

It would seem to me that you need to at least convert 0 to
USB_MAX_SYNCHRONOUS_TIMEOUT, if not outright capping the
timeout instead of erroring out.

	Regards
		Oliver