On Mon, May 24, 2021 at 10:47:36AM -0400, Alan Stern wrote: > On Mon, May 24, 2021 at 01:39:44PM +0200, Johan Hovold wrote: > > 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@xxxxx> > > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > > > > > --- > > > > > > 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). > > Yes. How nitpicky the checking needs to be for control transfers with > no data stage is an open question. (And it is unfortunate that the > warning message is somewhat misleading for this case.) > > > 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). > > There actually is a reason for this. If a host controller driver > determines the transfer's direction from the pipe value, we want it to > get the correct value. The spec says that transfers with no data stage > should be treated like OUT transfers (that is, the handshake stage > consists of a zero-length IN transaction), so usb_sndpipectrl() is what > should be used always. Right, and that's partly why I think it seems reasonable to always use usb_sndctrlpipe() for these transfers. But see below. > > Requiring this seems reasonable, but I did find a couple of media > > drivers (and syszbot reported another) that did "zero-length" reads. > > Do you think the check should be weakened for this case (i.e., ignore > the direction bit in bRequestType when wLength is 0)? So far it seems > that the number of places getting this wrong isn't prohibitively large. In a sense the request-type direction bit is already ignored when wLength is zero. The question is if we should ignore the direction bit of the pipe argument, or rather allow it to be IN, when wLength is zero. With the above check now merged, the following transfer triggers the warning: usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), 0, USB_DIR_IN | USB_TYPE_VENDOR, 0x0020, CMD_I2C_DA_RD, NULL, 0, 1000); This request was used by a media driver to determine if a certain i2c register was accessible by attempting to read it without really caring about its value. I changed the above to actually read the value, but this is an example where allowing usb_rcvctrlpipe() might otherwise make sense was it not for the possibility that some HCD could get confused. Changing the above to use usb_sndctrlpipe() while either keeping USB_DIR_IN or dropping USB_DIR_IN (for an I2C read request) does not seem right. The latter could potentially even confuse some firmware even if the direction bit is supposed to be ignored. So far this is the only example I've found where changing to usb_sndctrlpipe() and USB_DIR_OUT isn't obviously correct, but on the other hand just reading the register in question is straight-forward enough and does not require any exceptions in usb_submit_urb(). We could perhaps even go the other way and strengthen the check to warn if USB_DIR_IN is set when wLength is zero... > PS: Another check we could add is to make sure that the > transfer_buffer_length value agrees with wLength. Should I add such a > check? That sounds sensible as some of the HCDs only appears to check transfer_buffer_length when handling the data stage and a mismatch could amount to undefined behaviour (OUT) or perhaps even buffer overruns (IN). Judging from a quick check we don't seem to have any such cases currently so this could be implemented as a submission failure rather than another warning. Johan
