<div dir="ltr">I'm not a professional developer. I'm convinced you're much more suited to create the fixes than I am :)<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 3, 2020 at 5:40 PM Tom Rini <<a href="mailto:trini@konsulko.com">trini@konsulko.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Nov 02, 2020 at 02:53:24PM -0300, Enrique Nissim wrote:<br>
<br>
> Hi everyone,<br>
> <br>
> My name is Enrique Nissim... After exchanging some emails with Marek and<br>
> Tom, I'm sharing here what I believe could be a security issue in the USB<br>
> stack of uboot. Specifically, in the function that parses the USB HUB<br>
> Descriptors during enumeration:<br>
> <br>
> <a href="https://github.com/u-boot/u-boot/blob/c2279d784e35fa25ee3a9fa28a74a1ba545f8c1e/common/usb_hub.c#L613" rel="noreferrer" target="_blank">https://github.com/u-boot/u-boot/blob/c2279d784e35fa25ee3a9fa28a74a1ba545f8c1e/common/usb_hub.c#L613</a><br>
> <br>
> File: common\usb_hub.c<br>
> 613: static int usb_hub_configure(struct usb_device *dev)<br>
> 614: {<br>
> 615: int i, length;<br>
> 616: ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ);<br>
> 617: unsigned char *bitmap;<br>
> 618: short hubCharacteristics;<br>
> 619: struct usb_hub_descriptor *descriptor;<br>
> 620: struct usb_hub_device *hub;<br>
> 621: struct usb_hub_status *hubsts;<br>
> 622: int ret;<br>
> 623:<br>
> 624: hub = usb_get_hub_device(dev);<br>
> 625: if (hub == NULL)<br>
> 626: return -ENOMEM;<br>
> 627: hub->pusb_dev = dev;<br>
> 628:<br>
> 629: /* Get the the hub descriptor */<br>
> 630: ret = usb_get_hub_descriptor(dev, buffer, 4);<br>
> 631: if (ret < 0) {<br>
> 632: debug("usb_hub_configure: failed to get hub " \<br>
> 633:      "descriptor, giving up %lX\n", dev->status);<br>
> 634: return ret;<br>
> 635: }<br>
> 636: descriptor = (struct usb_hub_descriptor *)buffer;<br>
> 637:<br>
> 638: length = min_t(int, descriptor->bLength,<br>
> 639:       sizeof(struct usb_hub_descriptor));<br>
> 640:<br>
> 641: ret = usb_get_hub_descriptor(dev, buffer, length);<br>
> 642: if (ret < 0) {<br>
> 643: debug("usb_hub_configure: failed to get hub " \<br>
> 644:      "descriptor 2nd giving up %lX\n", dev->status);<br>
> 645: return ret;<br>
> 646: }<br>
> 647: memcpy((unsigned char *)&hub->desc, buffer, length);<br>
> 648: /* adjust 16bit values */<br>
> 649: put_unaligned(le16_to_cpu(get_unaligned(<br>
> 650: &descriptor->wHubCharacteristics)),<br>
> 651: &hub->desc.wHubCharacteristics);<br>
> 652: /* set the bitmap */<br>
> 653: bitmap = (unsigned char *)&hub->desc.u.hs.DeviceRemovable[0];<br>
> 654: /* devices not removable by default */<br>
> 655: memset(bitmap, 0xff, (USB_MAXCHILDREN+1+7)/8);<br>
> 656: bitmap = (unsigned char *)&hub->desc.u.hs.PortPowerCtrlMask[0];<br>
> 657: memset(bitmap, 0xff, (USB_MAXCHILDREN+1+7)/8); /* PowerMask = 1B */<br>
> 658:<br>
> 659: for (i = 0; i < ((hub->desc.bNbrPorts + 1 + 7)/8); i++)<br>
> 660: hub->desc.u.hs.DeviceRemovable[i] =<br>
> 661: descriptor->u.hs.DeviceRemovable[i];<br>
> 662:<br>
> 663: for (i = 0; i < ((hub->desc.bNbrPorts + 1 + 7)/8); i++)<br>
> 664: hub->desc.u.hs.PortPowerCtrlMask[i] =<br>
> 665: descriptor->u.hs.PortPowerCtrlMask[i];<br>
> 666:<br>
> 667: dev->maxchild = descriptor->bNbrPorts;<br>
> 668: debug("%d ports detected\n", dev->maxchild);<br>
> 669:<br>
> 670: hubCharacteristics = get_unaligned(&hub->desc.wHubCharacteristics);<br>
> <br>
> <br>
> The function starts by allocating a buffer on the stack of 512 bytes in<br>
> size. This memory isn't initialized in any way, which means it contains<br>
> `random` garbage values from the stack.<br>
> <br>
> There are two problems in this function:<br>
> <br>
> 1) The first time the hub descriptor is retrieved (line 630), a device<br>
> could provide a bLength value of 1, in which case the length variable will<br>
> be assigned 1 and after retrieving the descriptor again and copying the<br>
> buffer content into hub->desc, uses of further members like<br>
> `descriptor->u.hs.DeviceRemovable[i]`,<br>
> `descriptor->u.hs.PortPowerCtrlMask[i]`, or `descriptor->bNbrPorts` will<br>
> all be out of bounds in terms and contain garbage values from the stack.<br>
> <br>
> 2) By providing a full hub descriptor and setting bNbrPorts to 0xf0, the<br>
> for loops on lines 659 and 663 will be copying data out of bounds the<br>
> hub->desc structure, thus corrupting memory.<br>
> <br>
> I've checked the master branch of the usb core driver for Linux and I saw<br>
> there are more checks around this same value:<br>
> <a href="https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L1411" rel="noreferrer" target="_blank">https://github.com/torvalds/linux/blob/master/drivers/usb/core/hub.c#L1411</a><br>
<br>
Good catch.  Can you send a patch?  Thanks!<br>
<br>
-- <br>
Tom<br>
</blockquote></div>