Skip to content

Commit

Permalink
USB: usbcore: Fix slab-out-of-bounds bug during device reset
Browse files Browse the repository at this point in the history
commit 3dd550a upstream.

The syzbot fuzzer provoked a slab-out-of-bounds error in the USB core:

BUG: KASAN: slab-out-of-bounds in memcmp+0xa6/0xb0 lib/string.c:904
Read of size 1 at addr ffff8881d175bed6 by task kworker/0:3/2746

CPU: 0 PID: 2746 Comm: kworker/0:3 Not tainted 5.3.0-rc5+ #28
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0xca/0x13e lib/dump_stack.c:113
  print_address_description+0x6a/0x32c mm/kasan/report.c:351
  __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
  kasan_report+0xe/0x12 mm/kasan/common.c:612
  memcmp+0xa6/0xb0 lib/string.c:904
  memcmp include/linux/string.h:400 [inline]
  descriptors_changed drivers/usb/core/hub.c:5579 [inline]
  usb_reset_and_verify_device+0x564/0x1300 drivers/usb/core/hub.c:5729
  usb_reset_device+0x4c1/0x920 drivers/usb/core/hub.c:5898
  rt2x00usb_probe+0x53/0x7af
drivers/net/wireless/ralink/rt2x00/rt2x00usb.c:806

The error occurs when the descriptors_changed() routine (called during
a device reset) attempts to compare the old and new BOS and capability
descriptors.  The length it uses for the comparison is the
wTotalLength value stored in BOS descriptor, but this value is not
necessarily the same as the length actually allocated for the
descriptors.  If it is larger the routine will call memcmp() with a
length that is too big, thus reading beyond the end of the allocated
region and leading to this fault.

The kernel reads the BOS descriptor twice: first to get the total
length of all the capability descriptors, and second to read it along
with all those other descriptors.  A malicious (or very faulty) device
may send different values for the BOS descriptor fields each time.
The memory area will be allocated using the wTotalLength value read
the first time, but stored within it will be the value read the second
time.

To prevent this possibility from causing any errors, this patch
modifies the BOS descriptor after it has been read the second time:
It sets the wTotalLength field to the actual length of the descriptors
that were read in and validated.  Then the memcpy() call, or any other
code using these descriptors, will be able to rely on wTotalLength
being valid.

Reported-and-tested-by: syzbot+35f4d916c623118d576e@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/Pine.LNX.4.44L0.1909041154260.1722-100000@iolanthe.rowland.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
AlanStern authored and gregkh committed Sep 21, 2019
1 parent 4d856f7 commit 9f8dd40
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions drivers/usb/core/config.c
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ int usb_get_bos_descriptor(struct usb_device *dev)
struct usb_bos_descriptor *bos;
struct usb_dev_cap_header *cap;
struct usb_ssp_cap_descriptor *ssp_cap;
unsigned char *buffer;
unsigned char *buffer, *buffer0;
int length, total_len, num, i, ssac;
__u8 cap_type;
int ret;
Expand Down Expand Up @@ -966,10 +966,12 @@ int usb_get_bos_descriptor(struct usb_device *dev)
ret = -ENOMSG;
goto err;
}

buffer0 = buffer;
total_len -= length;
buffer += length;

for (i = 0; i < num; i++) {
buffer += length;
cap = (struct usb_dev_cap_header *)buffer;

if (total_len < sizeof(*cap) || total_len < cap->bLength) {
Expand All @@ -983,8 +985,6 @@ int usb_get_bos_descriptor(struct usb_device *dev)
break;
}

total_len -= length;

if (cap->bDescriptorType != USB_DT_DEVICE_CAPABILITY) {
dev_warn(ddev, "descriptor type invalid, skip\n");
continue;
Expand Down Expand Up @@ -1019,7 +1019,11 @@ int usb_get_bos_descriptor(struct usb_device *dev)
default:
break;
}

total_len -= length;
buffer += length;
}
dev->bos->desc->wTotalLength = cpu_to_le16(buffer - buffer0);

return 0;

Expand Down

0 comments on commit 9f8dd40

Please sign in to comment.