On Tue, May 25, 2021 at 02:40:17PM +0200, Johan Hovold wrote: > On Mon, May 24, 2021 at 10:47:36AM -0400, Alan Stern wrote: > > 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(). Okay, yes. This seems like a sufficiently unusual edge case that we don't need to add special code to cater for it. In fact, the direction bit in the pipe for a control transfer is never exposed to the USB device. All the device sees is bRequestType and the data/status packet tokens (IN or OUT), which are dictated by the USB protocol. So the fact that we insist on usb_sndctrlpipe for what will ultimately become an I2C read request is unimportant. > We could perhaps even go the other way and strengthen the check to warn > if USB_DIR_IN is set when wLength is zero... Given that the spec says the direction bit is ignored when wLength is zero, I think we shouldn't do this. > > 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. All right; I'll make the submission fail with a -EBADR (invalid request descriptor) error; that seems like a good choice of an obscure and otherwise unused value to match this case. But I'll put in a debugging message, so that anyone who wants to know if this is occurring will have a way to find out. Alan Stern
