[PATCH] Fix invalid USB descriptor endless loop.

[PATCH] Fix invalid USB descriptor endless loop.

[Melki Christian (consultant)]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

Hi,

I discovered that on some PC's the USB stack would produce an invalid descriptor upon query without an error.
I don't know why this is the case, maybe broken hardware but I seriously doubt it.
GRUB doesn't handle TT's at all, Clearing TT's or resetting them. Maybe thats a case for stuck transactions?
The descriptor would contain 0 in length, or atleast the code would think that offset was the length
and cause an endless loop.
Maybe this type of parsing is completely avoidable but for now I just added a break condition.
GRUB should not hang on faulty devices.

BR,
Christian

[-- Attachment #2: usb-invalid-desc.patch --]
[-- Type: application/octet-stream, Size: 1951 bytes --]

Index: grub-core/bus/usb/usb.c
===================================================================
--- grub-core/bus/usb/usb.c	(revision 5260)
+++ grub-core/bus/usb/usb.c	(revision 5261)
@@ -148,6 +148,7 @@
       int pos;
       int currif;
       char *data;
+      struct grub_usb_desc *desc;
 
       /* First just read the first 4 bytes of the configuration
 	 descriptor, after that it is known how many bytes really have
@@ -174,18 +175,35 @@
       /* Read all interfaces.  */
       for (currif = 0; currif < dev->config[i].descconf->numif; currif++)
 	{
-	  while (pos < config.totallen
-		 && ((struct grub_usb_desc *)&data[pos])->type
-		 != GRUB_USB_DESCRIPTOR_INTERFACE)
-	    pos += ((struct grub_usb_desc *)&data[pos])->length;
+	  while (pos < config.totallen)
+            {
+              desc = (struct grub_usb_desc *)&data[pos];
+              if (desc->type == GRUB_USB_DESCRIPTOR_INTERFACE)
+                break;
+              if (!desc->length)
+                {
+                  err = GRUB_USB_ERR_BADDEVICE;
+                  goto fail;
+                }
+              pos += desc->length;
+            }
+
 	  dev->config[i].interf[currif].descif
 	    = (struct grub_usb_desc_if *) &data[pos];
 	  pos += dev->config[i].interf[currif].descif->length;
 
-	  while (pos < config.totallen
-		 && ((struct grub_usb_desc *)&data[pos])->type
-		 != GRUB_USB_DESCRIPTOR_ENDPOINT)
-	    pos += ((struct grub_usb_desc *)&data[pos])->length;
+	  while (pos < config.totallen)
+            {
+              desc = (struct grub_usb_desc *)&data[pos];
+              if (desc->type == GRUB_USB_DESCRIPTOR_ENDPOINT)
+                break;
+              if (!desc->length)
+                {
+                  err = GRUB_USB_ERR_BADDEVICE;
+                  goto fail;
+                }
+              pos += desc->length;
+            }
 
 	  /* Point to the first endpoint.  */
 	  dev->config[i].interf[currif].descendp

Re: [PATCH] Fix invalid USB descriptor endless loop.

[Aleš Nesrsta]

Hi Christian,
sorry for delay, I am too busy in last time.

 > Hi,
 >
 > I discovered that on some PC's the USB stack would produce an invalid 
 > descriptor upon query without an error.
 > I don't know why this is the case, maybe broken hardware but I
 > seriously doubt it.
 > GRUB doesn't handle TT's at all, Clearing TT's or resetting them.
 > Maybe thats a case for stuck transactions?
 > The descriptor would contain 0 in length, or atleast the code would
 > think that offset was the length
 > and cause an endless loop.

I have minimal practical experiences with TT's.
Theoretically, AFAIK, it should normally work without special 
intervention - but of course the praxis could be different or I possibly 
missed something important...


> Maybe this type of parsing is completely avoidable but for now I just added a break condition.
> GRUB should not hang on faulty devices.

I cannot comment it (mainly the first sentence) - I am not original 
author of the most of GRUB USB code and this part of usb.c I didn't 
touched/studied yet (with some small exceptions) - I believed it works 
fine...:-)
I have no time currently to think about this situation more deeply, but 
from my point of view, if this situation can really happen and cannot be 
avoided by another way, this patch could be OK.


But it will be very good to know also the reason why the broken 
descriptor is read from device and no another error is indicated.
Which devices are affected? And which descriptor(s) exactly? Are these 
devices working well under Windows? Are affected still the same devices 
and in every case (at every connect)? And on every port of PC? Could you 
send detailed Linux lsusb output of "broken" devices? Or possibly whole 
detailed lsusb output, to be able to see the way how the device is 
connected to USB controller.

BR, Ales


>
> BR,
> Christian
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [PATCH] Fix invalid USB descriptor endless loop.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

Committed, thanks.
On 09.09.2013 08:22, Melki Christian (consultant) wrote:
> Hi,
> 
> I discovered that on some PC's the USB stack would produce an invalid descriptor upon query without an error.
> I don't know why this is the case, maybe broken hardware but I seriously doubt it.
> GRUB doesn't handle TT's at all, Clearing TT's or resetting them. Maybe thats a case for stuck transactions?
> The descriptor would contain 0 in length, or atleast the code would think that offset was the length
> and cause an endless loop.
> Maybe this type of parsing is completely avoidable but for now I just added a break condition.
> GRUB should not hang on faulty devices.
> 
> BR,
> Christian
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]