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:

quoted

On Mon, May 24, 2021 at 10:47:36AM -0400, Alan Stern wrote:

quoted

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.

quoted

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.

quoted

quoted

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