uboot USB stack security issue during parsing of usb hub descriptor

[Enrique Nissim]

I'm not a professional developer. I'm convinced you're much more suited to
create the fixes than I am :)

On Tue, Nov 3, 2020 at 5:40 PM Tom Rini <trini at konsulko.com> wrote:

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