uboot USB stack security issue during parsing of usb hub descriptor

[Enrique Nissim]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.denx.de/pipermail/u-boot-custodians/attachments/20201102/6b37468e/attachment.htm>