USB: Fix: Don't skip endpoint descriptors with maxpacket=0

Commit Message

Alan Stern Jan. 6, 2020, 3:43 p.m. UTC

It turns out that even though endpoints with a maxpacket length of 0
aren't useful for data transfer, the descriptors do serve other
purposes.  In particular, skipping them will also skip over other
class-specific descriptors for classes such as UVC.  This unexpected
side effect has caused some UVC cameras to stop working.

In addition, the USB spec requires that when isochronous endpoint
descriptors are present in an interface's altsetting 0 (which is true
on some devices), the maxpacket size _must_ be set to 0.  Warning
about such things seems like a bad idea.

This patch updates an earlier commit which would log a warning and
skip these endpoint descriptors.  Now we only log a warning, and we
don't even do that for isochronous endpoints in altsetting 0.

We don't need to worry about preventing endpoints with maxpacket = 0
from ever being used for data transfers; usb_submit_urb() already
checks for this.

Reported-and-tested-by: Roger Whittaker <[e-mail]>
Fixes: d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
Signed-off-by: Alan Stern <[e-mail]>
CC: Laurent Pinchart <[e-mail]>
Link: https://marc.info/?l=linux-usb&m=157790377329882&w=2

---


[as1928]


 drivers/usb/core/config.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Johan Hovold Jan. 6, 2020, 4:03 p.m. UTC | #1

On Mon, Jan 06, 2020 at 10:43:42AM -0500, Alan Stern wrote:
> It turns out that even though endpoints with a maxpacket length of 0
> aren't useful for data transfer, the descriptors do serve other
> purposes.  In particular, skipping them will also skip over other
> class-specific descriptors for classes such as UVC.  This unexpected
> side effect has caused some UVC cameras to stop working.
> 
> In addition, the USB spec requires that when isochronous endpoint
> descriptors are present in an interface's altsetting 0 (which is true
> on some devices), the maxpacket size _must_ be set to 0.  Warning
> about such things seems like a bad idea.
> 
> This patch updates an earlier commit which would log a warning and
> skip these endpoint descriptors.  Now we only log a warning, and we
> don't even do that for isochronous endpoints in altsetting 0.
> 
> We don't need to worry about preventing endpoints with maxpacket = 0
> from ever being used for data transfers; usb_submit_urb() already
> checks for this.
> 
> Reported-and-tested-by: Roger Whittaker <[e-mail]>
> Fixes: d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
> Signed-off-by: Alan Stern <[e-mail]>
> CC: Laurent Pinchart <[e-mail]>
> Link: https://marc.info/?l=linux-usb&m=157790377329882&w=2

Acked-by: Johan Hovold <[e-mail]>

We also need

Cc: stable <[e-mail]>

as d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length") ended up
being (auto- ?) selected for stable.

Johan

Laurent Pinchart Jan. 6, 2020, 4:13 p.m. UTC | #2

Hi Alan,

Thank you for the patch.

On Mon, Jan 06, 2020 at 10:43:42AM -0500, Alan Stern wrote:
> It turns out that even though endpoints with a maxpacket length of 0
> aren't useful for data transfer, the descriptors do serve other
> purposes.  In particular, skipping them will also skip over other
> class-specific descriptors for classes such as UVC.  This unexpected
> side effect has caused some UVC cameras to stop working.
> 
> In addition, the USB spec requires that when isochronous endpoint
> descriptors are present in an interface's altsetting 0 (which is true
> on some devices), the maxpacket size _must_ be set to 0.  Warning
> about such things seems like a bad idea.
> 
> This patch updates an earlier commit which would log a warning and
> skip these endpoint descriptors.  Now we only log a warning, and we
> don't even do that for isochronous endpoints in altsetting 0.
> 
> We don't need to worry about preventing endpoints with maxpacket = 0
> from ever being used for data transfers; usb_submit_urb() already
> checks for this.
> 
> Reported-and-tested-by: Roger Whittaker <[e-mail]>
> Fixes: d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
> Signed-off-by: Alan Stern <[e-mail]>
> CC: Laurent Pinchart <[e-mail]>
> Link: https://marc.info/?l=linux-usb&m=157790377329882&w=2

The patch looks good to me, so

Reviewed-by: Laurent Pinchart <[e-mail]>

But shouldn't we also warn if maxp != 0 && usb_endpoint_xfer_isoc(d) &&
asnum == 0 ?

> ---
> 
> 
> [as1928]
> 
> 
>  drivers/usb/core/config.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> Index: usb-devel/drivers/usb/core/config.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/config.c
> +++ usb-devel/drivers/usb/core/config.c
> @@ -346,12 +346,16 @@ static int usb_parse_endpoint(struct dev
>  			endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
>  	}
>  
> -	/* Validate the wMaxPacketSize field */
> +	/*
> +	 * Validate the wMaxPacketSize field.
> +	 * Some devices have isochronous endpoints in altsetting 0;
> +	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
> +	 * (see the end of section 5.6.3), so don't warn about them.
> +	 */
>  	maxp = usb_endpoint_maxp(&endpoint->desc);
> -	if (maxp == 0) {
> -		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has wMaxPacketSize 0, skipping\n",
> +	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
> +		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
>  		    cfgno, inum, asnum, d->bEndpointAddress);
> -		goto skip_to_next_endpoint_or_interface_descriptor;
>  	}
>  
>  	/* Find the highest legal maxpacket size for this endpoint */

Alan Stern Jan. 6, 2020, 4:17 p.m. UTC | #3

On Mon, 6 Jan 2020, Johan Hovold wrote:

> On Mon, Jan 06, 2020 at 10:43:42AM -0500, Alan Stern wrote:
> > It turns out that even though endpoints with a maxpacket length of 0
> > aren't useful for data transfer, the descriptors do serve other
> > purposes.  In particular, skipping them will also skip over other
> > class-specific descriptors for classes such as UVC.  This unexpected
> > side effect has caused some UVC cameras to stop working.
> > 
> > In addition, the USB spec requires that when isochronous endpoint
> > descriptors are present in an interface's altsetting 0 (which is true
> > on some devices), the maxpacket size _must_ be set to 0.  Warning
> > about such things seems like a bad idea.
> > 
> > This patch updates an earlier commit which would log a warning and
> > skip these endpoint descriptors.  Now we only log a warning, and we
> > don't even do that for isochronous endpoints in altsetting 0.
> > 
> > We don't need to worry about preventing endpoints with maxpacket = 0
> > from ever being used for data transfers; usb_submit_urb() already
> > checks for this.
> > 
> > Reported-and-tested-by: Roger Whittaker <[e-mail]>
> > Fixes: d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
> > Signed-off-by: Alan Stern <[e-mail]>
> > CC: Laurent Pinchart <[e-mail]>
> > Link: https://marc.info/?l=linux-usb&m=157790377329882&w=2
> 
> Acked-by: Johan Hovold <[e-mail]>
> 
> We also need
> 
> Cc: stable <[e-mail]>
> 
> as d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length") ended up
> being (auto- ?) selected for stable.

Absolutely -- I had intended to add that CC: but it slipped my mind 
when the email was being prepared.

Alan Stern

Alan Stern Jan. 6, 2020, 4:21 p.m. UTC | #4

On Mon, 6 Jan 2020, Laurent Pinchart wrote:

> Hi Alan,
> 
> Thank you for the patch.
> 
> On Mon, Jan 06, 2020 at 10:43:42AM -0500, Alan Stern wrote:
> > It turns out that even though endpoints with a maxpacket length of 0
> > aren't useful for data transfer, the descriptors do serve other
> > purposes.  In particular, skipping them will also skip over other
> > class-specific descriptors for classes such as UVC.  This unexpected
> > side effect has caused some UVC cameras to stop working.
> > 
> > In addition, the USB spec requires that when isochronous endpoint
> > descriptors are present in an interface's altsetting 0 (which is true
> > on some devices), the maxpacket size _must_ be set to 0.  Warning
> > about such things seems like a bad idea.
> > 
> > This patch updates an earlier commit which would log a warning and
> > skip these endpoint descriptors.  Now we only log a warning, and we
> > don't even do that for isochronous endpoints in altsetting 0.
> > 
> > We don't need to worry about preventing endpoints with maxpacket = 0
> > from ever being used for data transfers; usb_submit_urb() already
> > checks for this.
> > 
> > Reported-and-tested-by: Roger Whittaker <[e-mail]>
> > Fixes: d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
> > Signed-off-by: Alan Stern <[e-mail]>
> > CC: Laurent Pinchart <[e-mail]>
> > Link: https://marc.info/?l=linux-usb&m=157790377329882&w=2
> 
> The patch looks good to me, so
> 
> Reviewed-by: Laurent Pinchart <[e-mail]>
> 
> But shouldn't we also warn if maxp != 0 && usb_endpoint_xfer_isoc(d) &&
> asnum == 0 ?

We could, but that's a different kind of issue.  That would be more a
question of not adhering to the standard than of possibly failing to
work.

In theory the USBCV tests already check for this.  Manufacturers that 
don't run those tests probably also won't care if the Linux kernel 
complains.  But as far as I know, none of our drivers will malfunction 
if there's an isochronous endpoint in altsetting 0 with maxpacket > 0.

Alan Stern

Greg KH Jan. 6, 2020, 7:12 p.m. UTC | #5

On Mon, Jan 06, 2020 at 11:17:12AM -0500, Alan Stern wrote:
> On Mon, 6 Jan 2020, Johan Hovold wrote:
> 
> > On Mon, Jan 06, 2020 at 10:43:42AM -0500, Alan Stern wrote:
> > > It turns out that even though endpoints with a maxpacket length of 0
> > > aren't useful for data transfer, the descriptors do serve other
> > > purposes.  In particular, skipping them will also skip over other
> > > class-specific descriptors for classes such as UVC.  This unexpected
> > > side effect has caused some UVC cameras to stop working.
> > > 
> > > In addition, the USB spec requires that when isochronous endpoint
> > > descriptors are present in an interface's altsetting 0 (which is true
> > > on some devices), the maxpacket size _must_ be set to 0.  Warning
> > > about such things seems like a bad idea.
> > > 
> > > This patch updates an earlier commit which would log a warning and
> > > skip these endpoint descriptors.  Now we only log a warning, and we
> > > don't even do that for isochronous endpoints in altsetting 0.
> > > 
> > > We don't need to worry about preventing endpoints with maxpacket = 0
> > > from ever being used for data transfers; usb_submit_urb() already
> > > checks for this.
> > > 
> > > Reported-and-tested-by: Roger Whittaker <[e-mail]>
> > > Fixes: d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
> > > Signed-off-by: Alan Stern <[e-mail]>
> > > CC: Laurent Pinchart <[e-mail]>
> > > Link: https://marc.info/?l=linux-usb&m=157790377329882&w=2
> > 
> > Acked-by: Johan Hovold <[e-mail]>
> > 
> > We also need
> > 
> > Cc: stable <[e-mail]>
> > 
> > as d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length") ended up
> > being (auto- ?) selected for stable.
> 
> Absolutely -- I had intended to add that CC: but it slipped my mind 
> when the email was being prepared.

I'll catch this when it hits Linus's tree.

thanks,

greg k-h

Patch

Index: usb-devel/drivers/usb/core/config.c
===================================================================
--- usb-devel.orig/drivers/usb/core/config.c
+++ usb-devel/drivers/usb/core/config.c
@@ -346,12 +346,16 @@  static int usb_parse_endpoint(struct dev
 			endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
 	}
 
-	/* Validate the wMaxPacketSize field */
+	/*
+	 * Validate the wMaxPacketSize field.
+	 * Some devices have isochronous endpoints in altsetting 0;
+	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
+	 * (see the end of section 5.6.3), so don't warn about them.
+	 */
 	maxp = usb_endpoint_maxp(&endpoint->desc);
-	if (maxp == 0) {
-		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has wMaxPacketSize 0, skipping\n",
+	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
+		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
 		    cfgno, inum, asnum, d->bEndpointAddress);
-		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
 	/* Find the highest legal maxpacket size for this endpoint */