Re: usb:core: possible bug in wMaxPacketSize validation in config.c?

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Dec 09, 2021 at 08:53:37AM +0100, Pavel Hofman wrote:
> Hi,
> 
> in
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/core/config.c#L409
> the initial value of maxp is obtained using function usb_endpoint_maxp.
> 
> maxp = usb_endpoint_maxp(&endpoint->desc);
> 
> This function https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/usb/ch9.h#L647
> returns only the bits 0 - 10 of the wMaxPacketSize field, i.e. dropping the
> high-bandwidth bits 11 and 12. Yet the subsequent code extracts these bits
> from maxp into variable i
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/core/config.c#L427
> , clears them in maxp, and re-sets back in one of the further checks
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/core/config.c#L445
> 
> IMO that means the code requires that initial value of maxp contains the
> additional-transactions bits. IMO the code should be fixed with this trivial
> patch (tested on my build):
> 
> 
> ===================================================================
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> --- a/drivers/usb/core/config.c	(revision
> 018dd9dd80ab5f3bd988911b1f10255029ffa52d)
> +++ b/drivers/usb/core/config.c	(date 1638972286064)
> @@ -406,7 +406,7 @@
>  	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
>  	 * (see the end of section 5.6.3), so don't warn about them.
>  	 */
> -	maxp = usb_endpoint_maxp(&endpoint->desc);
> +	maxp = endpoint->desc.wMaxPacketSize;
>  	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
>  		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has
> invalid wMaxPacketSize 0\n",
>  		    cfgno, inum, asnum, d->bEndpointAddress);
> 
> 
> =========================
> 
> I can send a proper patch should the change be approved.

Please always just send a real patch, that makes it easier to discuss.

Anyway, what problem is this solving?  Do you have a device where the
data is calculated incorrectly?  What value in a device is being
declared incorrect because of the existing code?

thanks,

greg k-h