Re: [PATCH v2] USB: core: WARN if pipe direction != setup packet direction

[Johan Hovold <johan@kernel.org>]

On Fri, May 21, 2021 at 10:16:23PM -0400, Alan Stern wrote:
> When a control URB is submitted, the direction indicated by URB's pipe
> member is supposed to match the direction indicated by the setup
> packet's bRequestType member.  A mismatch could lead to trouble,
> depending on which field the host controller drivers use for
> determining the actual direction.
> 
> This shouldn't ever happen; it would represent a careless bug in a
> kernel driver somewhere.  This patch adds a dev_WARN_ONCE to let
> people know about the potential problem.
> 
> Suggested-by: "Geoffrey D. Bennett" <g@b4.vu>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> v2: Use dev_WARN_ONCE instead of dev_WARN
> 
> 
> [as1960b]
> 
> 
>  drivers/usb/core/urb.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: usb-devel/drivers/usb/core/urb.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/urb.c
> +++ usb-devel/drivers/usb/core/urb.c
> @@ -407,6 +407,9 @@ int usb_submit_urb(struct urb *urb, gfp_
>  			return -ENOEXEC;
>  		is_out = !(setup->bRequestType & USB_DIR_IN) ||
>  				!setup->wLength;
> +		dev_WARN_ONCE(&dev->dev, (usb_pipeout(urb->pipe) != is_out),
> +				"BOGUS control dir, pipe %x doesn't match bRequestType %x\n",
> +				urb->pipe, setup->bRequestType);

Note that the above will trigger for requests without a data stage also
when the pipe and request type agree in case the direction is IN (due to
the !wLength check).

According to the spec the direction bit should just be ignored for such
requests, but we now mandate that usb_sndpipectrl() is always used (i.e.
even when USB_DIR_IN is set).

Requiring this seems reasonable, but I did find a couple of media
drivers (and syszbot reported another) that did "zero-length" reads.

Johan