On Tue, Jul 25, 2023 at 12:26 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > On Sat, Jul 22, 2023 at 07:21:23PM -0700, syzbot wrote: > > Hello, > > > > syzbot has tested the proposed patch and the reproducer did not trigger any issue: > > > > Reported-and-tested-by: syzbot+18996170f8096c6174d0@xxxxxxxxxxxxxxxxxxxxxxxxx > > Here's a revised version of the patch, with some mistakes fixed. If > this works, I'll split it into three parts and submit them. > > Alan Stern > > #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc3 > > Index: usb-devel/drivers/usb/core/hub.c > =================================================================== > --- usb-devel.orig/drivers/usb/core/hub.c > +++ usb-devel/drivers/usb/core/hub.c > @@ -2671,12 +2671,17 @@ int usb_authorize_device(struct usb_devi > } > > if (usb_dev->wusb) { > - result = usb_get_device_descriptor(usb_dev, sizeof(usb_dev->descriptor)); > - if (result < 0) { > + struct usb_device_descriptor *descr; > + > + descr = usb_get_device_descriptor(usb_dev); > + if (IS_ERR(descr)) { > + result = PTR_ERR(descr); > dev_err(&usb_dev->dev, "can't re-read device descriptor for " > "authorization: %d\n", result); > goto error_device_descriptor; > } > + usb_dev->descriptor = *descr; Hmm, in your first patch you rejected diffs to the descriptor here, which might be necessary - since we don't re-initialize the device so can get a similar issue if the bad usb device decides to change bNumConfigurations to cause a buffer overrun. (This also stuck out to me as an exception to the "descriptors should be immutable" comment later in the patch. > + kfree(descr); > } > > usb_dev->authorized = 1; > @@ -4718,6 +4723,67 @@ static int hub_enable_device(struct usb_ > return hcd->driver->enable_device(hcd, udev); > } > > +/* > + * Get the bMaxPacketSize0 value during initialization by reading the > + * device's device descriptor. Since we don't already know this value, > + * the transfer is unsafe and it ignores I/O errors, only testing for > + * reasonable received values. > + * > + * For "old scheme" initialization size will be 8, so we read just the > + * start of the device descriptor, which should work okay regardless of > + * the actual bMaxPacketSize0 value. For "new scheme" initialization, > + * size will be 64 (and buf will point to a sufficiently large buffer), > + * which might not be kosher according to the USB spec but it's what > + * Windows does and what many devices expect. > + * > + * Returns bMaxPacketSize0 or a negative error code. > + */ > +static int get_bMaxPacketSize0(struct usb_device *udev, > + struct usb_device_descriptor *buf, int size, bool first_time) > +{ > + int i, rc; > + > + /* > + * Retry on all errors; some devices are flakey. > + * 255 is for WUSB devices, we actually need to use > + * 512 (WUSB1.0[4.8.1]). > + */ > + for (i = 0; i < GET_MAXPACKET0_TRIES; ++i) { > + /* Start with invalid values in case the transfer fails */ > + buf->bDescriptorType = buf->bMaxPacketSize0 = 0; > + rc = usb_control_msg(udev, usb_rcvaddr0pipe(), > + USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, > + USB_DT_DEVICE << 8, 0, > + buf, size, > + initial_descriptor_timeout); > + switch (buf->bMaxPacketSize0) { > + case 8: case 16: case 32: case 64: case 255: > + if (buf->bDescriptorType == USB_DT_DEVICE) { > + rc = buf->bMaxPacketSize0; > + break; > + } > + fallthrough; > + default: > + if (rc >= 0) > + rc = -EPROTO; > + break; > + } > + > + /* > + * Some devices time out if they are powered on > + * when already connected. They need a second > + * reset, so return early. But only on the first > + * attempt, lest we get into a time-out/reset loop. > + */ > + if (rc > 0 || (rc == -ETIMEDOUT && first_time && > + udev->speed > USB_SPEED_FULL)) > + break; > + } > + return rc; > +} > + > +#define GET_DESCRIPTOR_BUFSIZE 64 > + > /* Reset device, (re)assign address, get device descriptor. > * Device connection must be stable, no more debouncing needed. > * Returns device in USB_STATE_ADDRESS, except on error. > @@ -4727,10 +4793,17 @@ static int hub_enable_device(struct usb_ > * the port lock. For a newly detected device that is not accessible > * through any global pointers, it's not necessary to lock the device, > * but it is still necessary to lock the port. > + * > + * For a newly detected device, @dev_descr must be NULL. The device > + * descriptor retrieved from the device will then be stored in > + * @udev->descriptor. For an already existing device, @dev_descr > + * must be non-NULL. The device descriptor will be stored there, > + * not in @udev->descriptor, because descriptors for registered > + * devices are meant to be immutable. > */ > static int > hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, > - int retry_counter) > + int retry_counter, struct usb_device_descriptor *dev_descr) > { > struct usb_device *hdev = hub->hdev; > struct usb_hcd *hcd = bus_to_hcd(hdev->bus); > @@ -4742,6 +4815,13 @@ hub_port_init(struct usb_hub *hub, struc > int devnum = udev->devnum; > const char *driver_name; > bool do_new_scheme; > + const bool initial = !dev_descr; > + int maxp0; > + struct usb_device_descriptor *buf, *descr; > + > + buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO); > + if (!buf) > + return -ENOMEM; > > /* root hub ports have a slightly longer reset period > * (from USB 2.0 spec, section 7.1.7.5) > @@ -4774,32 +4854,34 @@ hub_port_init(struct usb_hub *hub, struc > } > oldspeed = udev->speed; > > - /* USB 2.0 section 5.5.3 talks about ep0 maxpacket ... > - * it's fixed size except for full speed devices. > - * For Wireless USB devices, ep0 max packet is always 512 (tho > - * reported as 0xff in the device descriptor). WUSB1.0[4.8.1]. > - */ > - switch (udev->speed) { > - case USB_SPEED_SUPER_PLUS: > - case USB_SPEED_SUPER: > - case USB_SPEED_WIRELESS: /* fixed at 512 */ > - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512); > - break; > - case USB_SPEED_HIGH: /* fixed at 64 */ > - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); > - break; > - case USB_SPEED_FULL: /* 8, 16, 32, or 64 */ > - /* to determine the ep0 maxpacket size, try to read > - * the device descriptor to get bMaxPacketSize0 and > - * then correct our initial guess. > - */ > - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); > - break; > - case USB_SPEED_LOW: /* fixed at 8 */ > - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8); > - break; > - default: > - goto fail; > + if (initial) { > + /* USB 2.0 section 5.5.3 talks about ep0 maxpacket ... > + * it's fixed size except for full speed devices. > + * For Wireless USB devices, ep0 max packet is always 512 (tho > + * reported as 0xff in the device descriptor). WUSB1.0[4.8.1]. > + */ > + switch (udev->speed) { > + case USB_SPEED_SUPER_PLUS: > + case USB_SPEED_SUPER: > + case USB_SPEED_WIRELESS: /* fixed at 512 */ > + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512); > + break; > + case USB_SPEED_HIGH: /* fixed at 64 */ > + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); > + break; > + case USB_SPEED_FULL: /* 8, 16, 32, or 64 */ > + /* to determine the ep0 maxpacket size, try to read > + * the device descriptor to get bMaxPacketSize0 and > + * then correct our initial guess. > + */ > + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); > + break; > + case USB_SPEED_LOW: /* fixed at 8 */ > + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8); > + break; > + default: > + goto fail; > + } > } > > if (udev->speed == USB_SPEED_WIRELESS) > @@ -4822,22 +4904,24 @@ hub_port_init(struct usb_hub *hub, struc > if (udev->speed < USB_SPEED_SUPER) > dev_info(&udev->dev, > "%s %s USB device number %d using %s\n", > - (udev->config) ? "reset" : "new", speed, > + (initial ? "new" : "reset"), speed, > devnum, driver_name); > > - /* Set up TT records, if needed */ > - if (hdev->tt) { > - udev->tt = hdev->tt; > - udev->ttport = hdev->ttport; > - } else if (udev->speed != USB_SPEED_HIGH > - && hdev->speed == USB_SPEED_HIGH) { > - if (!hub->tt.hub) { > - dev_err(&udev->dev, "parent hub has no TT\n"); > - retval = -EINVAL; > - goto fail; > + if (initial) { > + /* Set up TT records, if needed */ > + if (hdev->tt) { > + udev->tt = hdev->tt; > + udev->ttport = hdev->ttport; > + } else if (udev->speed != USB_SPEED_HIGH > + && hdev->speed == USB_SPEED_HIGH) { > + if (!hub->tt.hub) { > + dev_err(&udev->dev, "parent hub has no TT\n"); > + retval = -EINVAL; > + goto fail; > + } > + udev->tt = &hub->tt; > + udev->ttport = port1; > } > - udev->tt = &hub->tt; > - udev->ttport = port1; > } > > /* Why interleave GET_DESCRIPTOR and SET_ADDRESS this way? > @@ -4861,9 +4945,6 @@ hub_port_init(struct usb_hub *hub, struc > } > > if (do_new_scheme) { > - struct usb_device_descriptor *buf; > - int r = 0; > - > retval = hub_enable_device(udev); > if (retval < 0) { > dev_err(&udev->dev, > @@ -4872,52 +4953,14 @@ hub_port_init(struct usb_hub *hub, struc > goto fail; > } > > -#define GET_DESCRIPTOR_BUFSIZE 64 > - buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO); > - if (!buf) { > - retval = -ENOMEM; > - continue; > - } > - > - /* Retry on all errors; some devices are flakey. > - * 255 is for WUSB devices, we actually need to use > - * 512 (WUSB1.0[4.8.1]). > - */ > - for (operations = 0; operations < GET_MAXPACKET0_TRIES; > - ++operations) { > - buf->bMaxPacketSize0 = 0; > - r = usb_control_msg(udev, usb_rcvaddr0pipe(), > - USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, > - USB_DT_DEVICE << 8, 0, > - buf, GET_DESCRIPTOR_BUFSIZE, > - initial_descriptor_timeout); > - switch (buf->bMaxPacketSize0) { > - case 8: case 16: case 32: case 64: case 255: > - if (buf->bDescriptorType == > - USB_DT_DEVICE) { > - r = 0; > - break; > - } > - fallthrough; > - default: > - if (r == 0) > - r = -EPROTO; > - break; > - } > - /* > - * Some devices time out if they are powered on > - * when already connected. They need a second > - * reset. But only on the first attempt, > - * lest we get into a time out/reset loop > - */ > - if (r == 0 || (r == -ETIMEDOUT && > - retries == 0 && > - udev->speed > USB_SPEED_FULL)) > - break; > + maxp0 = get_bMaxPacketSize0(udev, buf, > + GET_DESCRIPTOR_BUFSIZE, retries == 0); > + if (maxp0 > 0 && !initial && > + maxp0 != udev->descriptor.bMaxPacketSize0) { > + dev_err(&udev->dev, "device reset changed ep0 maxpacket size!\n"); > + retval = -ENODEV; > + goto fail; > } > - udev->descriptor.bMaxPacketSize0 = > - buf->bMaxPacketSize0; > - kfree(buf); > > retval = hub_port_reset(hub, port1, udev, delay, false); > if (retval < 0) /* error or disconnect */ > @@ -4928,14 +4971,13 @@ hub_port_init(struct usb_hub *hub, struc > retval = -ENODEV; > goto fail; > } > - if (r) { > - if (r != -ENODEV) > + if (maxp0 < 0) { > + if (maxp0 != -ENODEV) > dev_err(&udev->dev, "device descriptor read/64, error %d\n", > - r); > - retval = -EMSGSIZE; > + maxp0); > + retval = maxp0; > continue; > } > -#undef GET_DESCRIPTOR_BUFSIZE > } > > /* > @@ -4981,18 +5023,22 @@ hub_port_init(struct usb_hub *hub, struc > break; > } > > - retval = usb_get_device_descriptor(udev, 8); > - if (retval < 8) { > + /* !do_new_scheme || wusb */ > + maxp0 = get_bMaxPacketSize0(udev, buf, 8, retries == 0); > + if (maxp0 < 0) { > + retval = maxp0; > if (retval != -ENODEV) > dev_err(&udev->dev, > "device descriptor read/8, error %d\n", > retval); > - if (retval >= 0) > - retval = -EMSGSIZE; > } else { > u32 delay; > > - retval = 0; > + if (!initial && maxp0 != udev->descriptor.bMaxPacketSize0) { > + dev_err(&udev->dev, "device reset changed ep0 maxpacket size!\n"); > + retval = -ENODEV; > + goto fail; > + } > > delay = udev->parent->hub_delay; > udev->hub_delay = min_t(u32, delay, > @@ -5010,27 +5056,10 @@ hub_port_init(struct usb_hub *hub, struc > if (retval) > goto fail; > > - /* > - * Some superspeed devices have finished the link training process > - * and attached to a superspeed hub port, but the device descriptor > - * got from those devices show they aren't superspeed devices. Warm > - * reset the port attached by the devices can fix them. > - */ > - if ((udev->speed >= USB_SPEED_SUPER) && > - (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) { > - dev_err(&udev->dev, "got a wrong device descriptor, " > - "warm reset device\n"); > - hub_port_reset(hub, port1, udev, > - HUB_BH_RESET_TIME, true); > - retval = -EINVAL; > - goto fail; > - } > - > - if (udev->descriptor.bMaxPacketSize0 == 0xff || > - udev->speed >= USB_SPEED_SUPER) > + if (maxp0 == 0xff || udev->speed >= USB_SPEED_SUPER) > i = 512; > else > - i = udev->descriptor.bMaxPacketSize0; > + i = maxp0; > if (usb_endpoint_maxp(&udev->ep0.desc) != i) { > if (udev->speed == USB_SPEED_LOW || > !(i == 8 || i == 16 || i == 32 || i == 64)) { > @@ -5046,13 +5075,33 @@ hub_port_init(struct usb_hub *hub, struc > usb_ep0_reinit(udev); > } > > - retval = usb_get_device_descriptor(udev, USB_DT_DEVICE_SIZE); > - if (retval < (signed)sizeof(udev->descriptor)) { > + descr = usb_get_device_descriptor(udev); > + if (IS_ERR(descr)) { > + retval = PTR_ERR(descr); > if (retval != -ENODEV) > dev_err(&udev->dev, "device descriptor read/all, error %d\n", > retval); > - if (retval >= 0) > - retval = -ENOMSG; > + goto fail; > + } > + if (initial) > + udev->descriptor = *descr; > + else > + *dev_descr = *descr; > + kfree(descr); > + > + /* > + * Some superspeed devices have finished the link training process > + * and attached to a superspeed hub port, but the device descriptor > + * got from those devices show they aren't superspeed devices. Warm > + * reset the port attached by the devices can fix them. > + */ > + if ((udev->speed >= USB_SPEED_SUPER) && > + (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) { > + dev_err(&udev->dev, "got a wrong device descriptor, " > + "warm reset device\n"); > + hub_port_reset(hub, port1, udev, > + HUB_BH_RESET_TIME, true); > + retval = -EINVAL; > goto fail; > } > > @@ -5078,6 +5127,7 @@ fail: > hub_port_disable(hub, port1, 0); > update_devnum(udev, devnum); /* for disconnect processing */ > } > + kfree(buf); > return retval; > } > > @@ -5158,7 +5208,7 @@ hub_power_remaining(struct usb_hub *hub) > > > static int descriptors_changed(struct usb_device *udev, > - struct usb_device_descriptor *old_device_descriptor, > + struct usb_device_descriptor *new_device_descriptor, > struct usb_host_bos *old_bos) > { > int changed = 0; > @@ -5169,8 +5219,8 @@ static int descriptors_changed(struct us > int length; > char *buf; > > - if (memcmp(&udev->descriptor, old_device_descriptor, > - sizeof(*old_device_descriptor)) != 0) > + if (memcmp(&udev->descriptor, new_device_descriptor, > + sizeof(*new_device_descriptor)) != 0) > return 1; > > if ((old_bos && !udev->bos) || (!old_bos && udev->bos)) > @@ -5348,7 +5398,7 @@ static void hub_port_connect(struct usb_ > } > > /* reset (non-USB 3.0 devices) and get descriptor */ > - status = hub_port_init(hub, udev, port1, i); > + status = hub_port_init(hub, udev, port1, i, NULL); > if (status < 0) > goto loop; > > @@ -5495,9 +5545,8 @@ static void hub_port_connect_change(stru > { > struct usb_port *port_dev = hub->ports[port1 - 1]; > struct usb_device *udev = port_dev->child; > - struct usb_device_descriptor descriptor; > + struct usb_device_descriptor *descr; > int status = -ENODEV; > - int retval; > > dev_dbg(&port_dev->dev, "status %04x, change %04x, %s\n", portstatus, > portchange, portspeed(hub, portstatus)); > @@ -5524,23 +5573,20 @@ static void hub_port_connect_change(stru > * changed device descriptors before resuscitating the > * device. > */ > - descriptor = udev->descriptor; > - retval = usb_get_device_descriptor(udev, > - sizeof(udev->descriptor)); > - if (retval < 0) { > + descr = usb_get_device_descriptor(udev); > + if (IS_ERR(descr)) { > dev_dbg(&udev->dev, > - "can't read device descriptor %d\n", > - retval); > + "can't read device descriptor %ld\n", > + PTR_ERR(descr)); > } else { > - if (descriptors_changed(udev, &descriptor, > + if (descriptors_changed(udev, descr, > udev->bos)) { > dev_dbg(&udev->dev, > "device descriptor has changed\n"); > - /* for disconnect() calls */ > - udev->descriptor = descriptor; > } else { > status = 0; /* Nothing to do */ > } > + kfree(descr); > } > #ifdef CONFIG_PM > } else if (udev->state == USB_STATE_SUSPENDED && > @@ -5982,7 +6028,7 @@ static int usb_reset_and_verify_device(s > struct usb_device *parent_hdev = udev->parent; > struct usb_hub *parent_hub; > struct usb_hcd *hcd = bus_to_hcd(udev->bus); > - struct usb_device_descriptor descriptor = udev->descriptor; > + struct usb_device_descriptor descriptor; > struct usb_host_bos *bos; > int i, j, ret = 0; > int port1 = udev->portnum; > @@ -6018,7 +6064,7 @@ static int usb_reset_and_verify_device(s > /* ep0 maxpacket size may change; let the HCD know about it. > * Other endpoints will be handled by re-enumeration. */ > usb_ep0_reinit(udev); > - ret = hub_port_init(parent_hub, udev, port1, i); > + ret = hub_port_init(parent_hub, udev, port1, i, &descriptor); Looks like this is the only caller that passes &descriptor, and it just checks that it didn't change. Would it make sense to put the enitre descriptors_changed stanza in hub_port_init, for the re-init case? > if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV) > break; > } > @@ -6030,7 +6076,6 @@ static int usb_reset_and_verify_device(s > /* Device might have changed firmware (DFU or similar) */ > if (descriptors_changed(udev, &descriptor, bos)) { > dev_info(&udev->dev, "device firmware changed\n"); > - udev->descriptor = descriptor; /* for disconnect() calls */ > goto re_enumerate; > } > > Index: usb-devel/drivers/usb/core/hcd.c > =================================================================== > --- usb-devel.orig/drivers/usb/core/hcd.c > +++ usb-devel/drivers/usb/core/hcd.c > @@ -983,6 +983,7 @@ static int register_root_hub(struct usb_ > { > struct device *parent_dev = hcd->self.controller; > struct usb_device *usb_dev = hcd->self.root_hub; > + struct usb_device_descriptor *descr; > const int devnum = 1; > int retval; > > @@ -994,13 +995,16 @@ static int register_root_hub(struct usb_ > mutex_lock(&usb_bus_idr_lock); > > usb_dev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); > - retval = usb_get_device_descriptor(usb_dev, USB_DT_DEVICE_SIZE); > - if (retval != sizeof usb_dev->descriptor) { > + descr = usb_get_device_descriptor(usb_dev); > + if (IS_ERR(descr)) { > + retval = PTR_ERR(descr); > mutex_unlock(&usb_bus_idr_lock); > dev_dbg (parent_dev, "can't read %s device descriptor %d\n", > dev_name(&usb_dev->dev), retval); > - return (retval < 0) ? retval : -EMSGSIZE; > + return retval; > } > + usb_dev->descriptor = *descr; > + kfree(descr); > > if (le16_to_cpu(usb_dev->descriptor.bcdUSB) >= 0x0201) { > retval = usb_get_bos_descriptor(usb_dev); > Index: usb-devel/drivers/usb/core/message.c > =================================================================== > --- usb-devel.orig/drivers/usb/core/message.c > +++ usb-devel/drivers/usb/core/message.c > @@ -1040,40 +1040,35 @@ char *usb_cache_string(struct usb_device > EXPORT_SYMBOL_GPL(usb_cache_string); > > /* > - * usb_get_device_descriptor - (re)reads the device descriptor (usbcore) > - * @dev: the device whose device descriptor is being updated > - * @size: how much of the descriptor to read > + * usb_get_device_descriptor - read the device descriptor > + * @udev: the device whose device descriptor should be read > * > * Context: task context, might sleep. > * > - * Updates the copy of the device descriptor stored in the device structure, > - * which dedicates space for this purpose. > - * > * Not exported, only for use by the core. If drivers really want to read > * the device descriptor directly, they can call usb_get_descriptor() with > * type = USB_DT_DEVICE and index = 0. > * > - * This call is synchronous, and may not be used in an interrupt context. > - * > - * Return: The number of bytes received on success, or else the status code > - * returned by the underlying usb_control_msg() call. > + * Returns: a pointer to a dynamically allocated usb_device_descriptor > + * structure (which the caller must deallocate), or an ERR_PTR value. > */ > -int usb_get_device_descriptor(struct usb_device *dev, unsigned int size) > +struct usb_device_descriptor *usb_get_device_descriptor(struct usb_device *udev) > { > struct usb_device_descriptor *desc; > int ret; > > - if (size > sizeof(*desc)) > - return -EINVAL; > desc = kmalloc(sizeof(*desc), GFP_NOIO); > if (!desc) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > + > + ret = usb_get_descriptor(udev, USB_DT_DEVICE, 0, desc, sizeof(*desc)); > + if (ret == sizeof(*desc)) > + return desc; > > - ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, size); > if (ret >= 0) > - memcpy(&dev->descriptor, desc, size); > + ret = -EMSGSIZE; > kfree(desc); > - return ret; > + return ERR_PTR(ret); > } > > /* > Index: usb-devel/drivers/usb/core/usb.h > =================================================================== > --- usb-devel.orig/drivers/usb/core/usb.h > +++ usb-devel/drivers/usb/core/usb.h > @@ -43,8 +43,8 @@ extern bool usb_endpoint_is_ignored(stru > struct usb_endpoint_descriptor *epd); > extern int usb_remove_device(struct usb_device *udev); > > -extern int usb_get_device_descriptor(struct usb_device *dev, > - unsigned int size); > +extern struct usb_device_descriptor *usb_get_device_descriptor( > + struct usb_device *udev); > extern int usb_set_isoch_delay(struct usb_device *dev); > extern int usb_get_bos_descriptor(struct usb_device *dev); > extern void usb_release_bos_descriptor(struct usb_device *dev);
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
