linux-kernel - Re: [PATCH v4] usbip: Don't submit special requests twice

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]

Date: Sun, 5 May 2024 23:31:44 +0800
From: Hongren Zheng <i@...ithal.me>
To: Simon Holesch <simon@...esch.de>
Cc: Valentina Manea <valentina.manea.m@...il.com>,
	Shuah Khan <shuah@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Shuah Khan <skhan@...uxfoundation.org>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] usbip: Don't submit special requests twice

On Sun, Dec 17, 2023 at 08:30:40PM +0100, Simon Holesch wrote:
> Skip submitting URBs, when identical requests were already sent in
> tweak_special_requests(). Instead call the completion handler directly
> to return the result of the URB.

Reproduced the behavior and this patch fixed the bahavior

> 
> Even though submitting those requests twice should be harmless, there
> are USB devices that react poorly to some duplicated requests.
> 
> One example is the ChipIdea controller implementation in U-Boot: The
> second SET_CONFIURATION request makes U-Boot disable and re-enable all
> endpoints. Re-enabling an endpoint in the ChipIdea controller, however,
> was broken until U-Boot commit b272c8792502 ("usb: ci: Fix gadget
> reinit").
> 
> Signed-off-by: Simon Holesch <simon@...esch.de>
> Acked-by: Shuah Khan <skhan@...uxfoundation.org>
> ---

>  /*
> @@ -468,6 +477,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  	int support_sg = 1;
>  	int np = 0;
>  	int ret, i;
> +	int is_tweaked;
>  
>  	if (pipe == -1)
>  		return;
> @@ -580,8 +590,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  		priv->urbs[i]->pipe = pipe;
>  		priv->urbs[i]->complete = stub_complete;
>  
> -		/* no need to submit an intercepted request, but harmless? */
> -		tweak_special_requests(priv->urbs[i]);
> +		is_tweaked = tweak_special_requests(priv->urbs[i]);

One question though, if there are mutiple urbs and one of them is
SET CONFIGURATION, then all of them would not be submitted,
as is_tweaked is a *global* flag instead of a per-urb flag.

Now it is assumed that when the urb is SET CONFIGURATION then
num_urbs is 1. I assume it just happens to be the case and I do
not know if it holds for all scenario.

>  
>  		masking_bogus_flags(priv->urbs[i]);
>  	}
> @@ -594,22 +603,32 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>  
>  	/* urb is now ready to submit */
>  	for (i = 0; i < priv->num_urbs; i++) {
> -		ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
> +		if (!is_tweaked) {
> +			ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
>  
> -		if (ret == 0)
> -			usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> -					pdu->base.seqnum);
> -		else {
> -			dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> -			usbip_dump_header(pdu);
> -			usbip_dump_urb(priv->urbs[i]);
> +			if (ret == 0)
> +				usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> +						pdu->base.seqnum);
> +			else {
> +				dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> +				usbip_dump_header(pdu);
> +				usbip_dump_urb(priv->urbs[i]);
>  
> +				/*
> +				 * Pessimistic.
> +				 * This connection will be discarded.
> +				 */
> +				usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> +				break;
> +			}
> +		} else {
>  			/*
> -			 * Pessimistic.
> -			 * This connection will be discarded.
> +			 * An identical URB was already submitted in
> +			 * tweak_special_requests(). Skip submitting this URB to not
> +			 * duplicate the request.
>  			 */
> -			usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> -			break;
> +			priv->urbs[i]->status = 0;
> +			stub_complete(priv->urbs[i]);
>  		}
>  	}
>  
> -- 
> 2.43.0
>