On Fri, Jul 21, 2023 at 03:40:01PM -0700, Khazhy Kumykov wrote: > On Fri, Jul 21, 2023 at 11:56 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > On the other hand, it would almost certainly be simpler if > > hub_port_connect_change() and the other places calling > > usb_get_device_descriptor() would read into a temporary buffer instead > > of directly into udev->descriptor. Do you think the problem could be > > solved this way? It would be cleaner in the end. > > Simpler... It'll probably be cleaner in the end, but we're > snapshotting and resetting udev->descriptor several call frames above > where we're calling usb_get_device_descriptor in the case of > usb_reset_and_verify_device().. For hub_port_connect_change() it > should be straightforward - use the on-stack descriptor as the buf for > usb_get_descriptor(), and bail out like we do already. > > For usb_reset_and_verify_device... we're calling hub_port_init, which > is directly modifying a bunch of the usb struct, fetches the > descriptor, validates it, and we rely on the return here to decide > whether or not to simulate a disconnect... > > I'd personally lean to reverting 45bf39f8df7f, but I'm not that > familiar with the code here. :) Let's see what syzbot has to say about this patch... Alan Stern #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc2 drivers/usb/core/hcd.c | 3 drivers/usb/core/hub.c | 167 +++++++++++++++++++++++++-------------------- drivers/usb/core/message.c | 37 --------- drivers/usb/core/usb.h | 8 +- 4 files changed, 104 insertions(+), 111 deletions(-) Index: usb-devel/drivers/usb/core/hcd.c =================================================================== --- usb-devel.orig/drivers/usb/core/hcd.c +++ usb-devel/drivers/usb/core/hcd.c @@ -994,7 +994,8 @@ 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); + retval = usb_get_device_descriptor(usb_dev, + &usb_dev->descriptor, USB_DT_DEVICE_SIZE); if (retval != sizeof usb_dev->descriptor) { mutex_unlock(&usb_bus_idr_lock); dev_dbg (parent_dev, "can't read %s device descriptor %d\n", Index: usb-devel/drivers/usb/core/message.c =================================================================== --- usb-devel.orig/drivers/usb/core/message.c +++ usb-devel/drivers/usb/core/message.c @@ -1040,43 +1040,6 @@ 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 - * - * 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. - */ -int usb_get_device_descriptor(struct usb_device *dev, unsigned int size) -{ - struct usb_device_descriptor *desc; - int ret; - - if (size > sizeof(*desc)) - return -EINVAL; - desc = kmalloc(sizeof(*desc), GFP_NOIO); - if (!desc) - return -ENOMEM; - - ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, size); - if (ret >= 0) - memcpy(&dev->descriptor, desc, size); - kfree(desc); - return ret; -} - -/* * usb_set_isoch_delay - informs the device of the packet transmit delay * @dev: the device whose delay is to be informed * Context: task context, might sleep 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,6 @@ 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 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); @@ -57,6 +55,12 @@ extern int usb_generic_driver_suspend(st extern int usb_generic_driver_resume(struct usb_device *udev, pm_message_t msg); +static inline int usb_get_device_descriptor(struct usb_device *dev, + struct usb_device_descriptor *desc, unsigned int size) +{ + return usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, size); +} + static inline unsigned usb_get_max_power(struct usb_device *udev, struct usb_host_config *c) { 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,19 @@ int usb_authorize_device(struct usb_devi } if (usb_dev->wusb) { - result = usb_get_device_descriptor(usb_dev, sizeof(usb_dev->descriptor)); + struct usb_device_descriptor desc; + + result = usb_get_device_descriptor(usb_dev, &desc, sizeof(desc)); if (result < 0) { dev_err(&usb_dev->dev, "can't re-read device descriptor for " "authorization: %d\n", result); goto error_device_descriptor; } + if (memcmp(&usb_dev->descriptor, &desc, sizeof(desc)) != 0) { + dev_err(&usb_dev->dev, "device descriptor changed before authorization: %d\n", + result); + goto error_device_descriptor; + } } usb_dev->authorized = 1; @@ -4730,7 +4737,7 @@ static int hub_enable_device(struct usb_ */ 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 +4749,12 @@ hub_port_init(struct usb_hub *hub, struc int devnum = udev->devnum; const char *driver_name; bool do_new_scheme; + const bool reinit = !!dev_descr; + union { + struct usb_device_descriptor d; +#define GET_DESCRIPTOR_BUFSIZE 64 + u8 raw[GET_DESCRIPTOR_BUFSIZE]; + } buf; /* root hub ports have a slightly longer reset period * (from USB 2.0 spec, section 7.1.7.5) @@ -4774,32 +4787,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 (!reinit) { + /* 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 +4837,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, + (reinit ? "reset" : "new"), 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 (!reinit) { + /* 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,7 +4878,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); @@ -4872,28 +4888,21 @@ 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; + buf.d.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, + buf.raw, GET_DESCRIPTOR_BUFSIZE, initial_descriptor_timeout); - switch (buf->bMaxPacketSize0) { + switch (buf.d.bMaxPacketSize0) { case 8: case 16: case 32: case 64: case 255: - if (buf->bDescriptorType == + if (buf.d.bDescriptorType == USB_DT_DEVICE) { r = 0; break; @@ -4915,9 +4924,15 @@ hub_port_init(struct usb_hub *hub, struc udev->speed > USB_SPEED_FULL)) break; } - udev->descriptor.bMaxPacketSize0 = - buf->bMaxPacketSize0; - kfree(buf); + if (!reinit) { + udev->descriptor.bMaxPacketSize0 = + buf.d.bMaxPacketSize0; + } else if (udev->descriptor.bMaxPacketSize0 != + buf.d.bMaxPacketSize0) { + dev_err(&udev->dev, "device reset changed ep0 maxpacket size!\n"); + retval = -ENODEV; + goto fail; + } retval = hub_port_reset(hub, port1, udev, delay, false); if (retval < 0) /* error or disconnect */ @@ -4981,7 +4996,8 @@ hub_port_init(struct usb_hub *hub, struc break; } - retval = usb_get_device_descriptor(udev, 8); + /* !do_new_scheme || wusb */ + retval = usb_get_device_descriptor(udev, &buf.d, 8); if (retval < 8) { if (retval != -ENODEV) dev_err(&udev->dev, @@ -4992,6 +5008,15 @@ hub_port_init(struct usb_hub *hub, struc } else { u32 delay; + if (!reinit) { + udev->descriptor.bMaxPacketSize0 = + buf.d.bMaxPacketSize0; + } else if (udev->descriptor.bMaxPacketSize0 != + buf.d.bMaxPacketSize0) { + dev_err(&udev->dev, "device reset changed ep0 maxpacket size!\n"); + retval = -ENODEV; + goto fail; + } retval = 0; delay = udev->parent->hub_delay; @@ -5046,8 +5071,8 @@ 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)) { + retval = usb_get_device_descriptor(udev, &buf.d, sizeof(buf.d)); + if (retval < (signed)sizeof(buf.d)) { if (retval != -ENODEV) dev_err(&udev->dev, "device descriptor read/all, error %d\n", retval); @@ -5055,6 +5080,10 @@ hub_port_init(struct usb_hub *hub, struc retval = -ENOMSG; goto fail; } + if (!reinit) + udev->descriptor = buf.d; + else + *dev_descr = buf.d; usb_detect_quirks(udev); @@ -5158,7 +5187,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 +5198,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 +5377,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; @@ -5524,9 +5553,8 @@ 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)); + retval = usb_get_device_descriptor(udev, &descriptor, + sizeof(descriptor)); if (retval < 0) { dev_dbg(&udev->dev, "can't read device descriptor %d\n", @@ -5536,8 +5564,6 @@ static void hub_port_connect_change(stru udev->bos)) { dev_dbg(&udev->dev, "device descriptor has changed\n"); - /* for disconnect() calls */ - udev->descriptor = descriptor; } else { status = 0; /* Nothing to do */ } @@ -5982,7 +6008,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 +6044,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); if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV) break; } @@ -6030,7 +6056,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; }
