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

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

[Alan Stern]

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);
 	}

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

[Johan Hovold]

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.

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.

Johan

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

[Greg KH]

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.

That's a good idea, this could get very noisy very quickly if one driver
got it wrong.

thanks,

greg k-h

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

[Johan Hovold]

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

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

[Alan Stern]

On Fri, May 21, 2021 at 03:17:46PM +0200, Johan Hovold wrote:
> On Fri, May 21, 2021 at 10:03:26AM +0200, Johan Hovold wrote:
> > 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.

Many thanks for making the effort to do this.

Alan Stern

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

[Alan Stern]

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@b4.vu>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

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);
 	} else {
 		is_out = usb_endpoint_dir_out(&ep->desc);
 	}

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

[Johan Hovold]

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@b4.vu>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> ---
> 
> v2: Use dev_WARN_ONCE instead of dev_WARN

Reviewed-by: Johan Hovold <johan@kernel.org>

>  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);
>  	} else {
>  		is_out = usb_endpoint_dir_out(&ep->desc);
>  	}

Johan

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

[Johan Hovold]

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@b4.vu>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> 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).

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).

Requiring this seems reasonable, but I did find a couple of media
drivers (and syszbot reported another) that did "zero-length" reads.

Johan

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

[Alan Stern]

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@b4.vu>
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > ---
> > 
> > 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.

> 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.

Alan Stern

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?

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

[Johan Hovold]

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@b4.vu>
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > 
> > > ---
> > > 
> > > 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

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

[Alan Stern]

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

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

[Johan Hovold]

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

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

[Alan Stern]

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:

> > +		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);

> 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.

So change it to dev_WARN_ONCE()?  You and Greg think that will be good 
enough?

> Second, did you try to estimate how many call sites that get this wrong?

No.  I couldn't think of a good way to search for them.  My guess is 
that the number isn't terribly big, but I don't know.

> 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.

Alan Stern

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

[Johan Hovold]

On Fri, May 21, 2021 at 10:38:23AM -0400, Alan Stern wrote:
> 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:
> 
> > > +		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);
> 
> > 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.
> 
> So change it to dev_WARN_ONCE()?  You and Greg think that will be good 
> enough?

I think so, yes. In the unlikely event that there are more than one
driver in use that gets this wrong in a system it'll only take a little
longer to root them all out.

Johan