Re: [syzbot] [usb?] KASAN: slab-out-of-bounds Read in read_descriptors (3)

From: Alan Stern
Date: Sat Jul 22 2023 - 22:01:19 EST

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;
}