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

[Johan Hovold <johan@kernel.org>]

On Fri, May 21, 2021 at 10:03:26AM +0200, Johan Hovold wrote:
> On Thu, May 20, 2021 at 04:20:56PM -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 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>
> > 
> > ---
> > 
> > 
> > [as1960]
> > 
> > 
> >  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;
> > +		if (usb_pipeout(urb->pipe) != is_out)
> > +			dev_WARN(&dev->dev, "BOGUS control dir, pipe %x doesn't match bRequestType %x\n",
> > +					urb->pipe, setup->bRequestType);
> >  	} else {
> >  		is_out = usb_endpoint_dir_out(&ep->desc);
> >  	}
> 
> While I agree with intention here, I'm worried that this will start
> flooding the logs of users.
> 
> So first, this should probably be rate limited.

This could actually be done using WARN_ON_ONCE() as we don't have to
worry about syzbot fuzzing descriptors here (all control endpoints are
bidirectional).

> Second, did you try to estimate how many call sites that get this wrong?
> I always felt a bit pedantic when pointing out that the pipe direction
> should match the request type to driver author's during review when (in
> almost all cases?) this hasn't really mattered. I fear we may have
> accumulated a fairly large number of these mismatches over the years but
> I haven't verified that.

I did a quick review of all ctrlpipe-macro uses in usb/misc and
usb/serial and found two instances.

A simple grep pattern looking for explicit USB_DIR/ctrlpipe mismatches
caught another five tree wide (not including the
sound/usb/mixer_scarlett_gen2.c which Geoffrey reported), but there are
likely more of those out there as the request type is often not that
explicit.

I've prepared patches for the above, excluding the sound driver Geoffrey
said he was fixing.

Johan