On Tue, May 25, 2021 at 11:12:08AM -0400, Alan Stern wrote: > 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. Right, it just looks a bit weird to use usb_sndctrlpipe() with USB_DIR_IN, but that should be fine especially as such cases appears to be rare. > > 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. I agree, let's just allow 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. Sounds good. Johan
