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

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 40f71e7cd3c6 Merge tag 'net-6.4-rc7' of git://git.kernel.o..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1581445b280000
kernel config: https://syzkaller.appspot.com/x/.config?x=ac246111fb601aec
dashboard link: https://syzkaller.appspot.com/bug?extid=18996170f8096c6174d0
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15d23487280000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16613ed3280000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/30922ad38c58/disk-40f71e7c.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/3bd12e7503b8/vmlinux-40f71e7c.xz
kernel image: https://storage.googleapis.com/syzbot-assets/1dcd340b18d4/bzImage-40f71e7c.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+189961...@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: slab-out-of-bounds in read_descriptors+0x263/0x280 drivers/usb/core/sysfs.c:883
Read of size 8 at addr ffff88801e78b8c8 by task udevd/5011

CPU: 0 PID: 5011 Comm: udevd Not tainted 6.4.0-rc6-syzkaller-00195-g40f71e7cd3c6 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/27/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
print_address_description.constprop.0+0x2c/0x3c0 mm/kasan/report.c:351
print_report mm/kasan/report.c:462 [inline]
kasan_report+0x11c/0x130 mm/kasan/report.c:572
read_descriptors+0x263/0x280 drivers/usb/core/sysfs.c:883
sysfs_kf_bin_read+0x19a/0x270 fs/sysfs/file.c:97
kernfs_file_read_iter fs/kernfs/file.c:251 [inline]
kernfs_fop_read_iter+0x387/0x690 fs/kernfs/file.c:280
call_read_iter include/linux/fs.h:1862 [inline]
new_sync_read fs/read_write.c:389 [inline]
vfs_read+0x4b1/0x8a0 fs/read_write.c:470
ksys_read+0x12b/0x250 fs/read_write.c:613
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f07c7916b6a
Code: 00 3d 00 00 41 00 75 0d 50 48 8d 3d 2d 08 0a 00 e8 ea 7d 01 00 31 c0 e9 07 ff ff ff 64 8b 04 25 18 00 00 00 85 c0 75 1b 0f 05 <48> 3d 00 f0 ff ff 76 6c 48 8b 15 8f a2 0d 00 f7 d8 64 89 02 48 83
RSP: 002b:00007ffdf34973d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f07c7916b6a
RDX: 0000000000010011 RSI: 00007ffdf3497407 RDI: 0000000000000008
RBP: 0000000000000008 R08: 0000000000000003 R09: f4f13e10193fbafe
R10: 0000000000000000 R11: 0000000000000246 R12: 000055be37470e10
R13: 00007ffdf34a7ae8 R14: 00007ffdf34a8138 R15: 00007ffdf3497407
</TASK>

Allocated by task 758:
kasan_save_stack+0x22/0x40 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
____kasan_kmalloc mm/kasan/common.c:374 [inline]
____kasan_kmalloc mm/kasan/common.c:333 [inline]
__kasan_kmalloc+0xa2/0xb0 mm/kasan/common.c:383
kasan_kmalloc include/linux/kasan.h:196 [inline]
__do_kmalloc_node mm/slab_common.c:966 [inline]
__kmalloc+0x5e/0x190 mm/slab_common.c:979
kmalloc include/linux/slab.h:563 [inline]
kzalloc include/linux/slab.h:680 [inline]
usb_get_configuration+0x1f7/0x5170 drivers/usb/core/config.c:887
usb_enumerate_device drivers/usb/core/hub.c:2407 [inline]
usb_new_device+0x12b0/0x19d0 drivers/usb/core/hub.c:2545
hub_port_connect drivers/usb/core/hub.c:5407 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
port_event drivers/usb/core/hub.c:5711 [inline]
hub_event+0x2d9e/0x4e40 drivers/usb/core/hub.c:5793
process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
kthread+0x344/0x440 kernel/kthread.c:379
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308

The buggy address belongs to the object at ffff88801e78b8c0
which belongs to the cache kmalloc-8 of size 8
The buggy address is located 0 bytes to the right of
allocated 8-byte region [ffff88801e78b8c0, ffff88801e78b8c8)

The buggy address belongs to the physical page:
page:ffffea000079e2c0 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1e78b
anon flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xffffffff()
raw: 00fff00000000200 ffff888012441280 0000000000000000 dead000000000001
raw: 0000000000000000 0000000000660066 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY), pid 1, tgid 1 (swapper/0), ts 8298345549, free_ts 8292702290
set_page_owner include/linux/page_owner.h:31 [inline]
post_alloc_hook+0x2db/0x350 mm/page_alloc.c:1731
prep_new_page mm/page_alloc.c:1738 [inline]
get_page_from_freelist+0xf41/0x2c00 mm/page_alloc.c:3502
__alloc_pages+0x1cb/0x4a0 mm/page_alloc.c:4768
alloc_page_interleave+0x1e/0x200 mm/mempolicy.c:2112
alloc_pages+0x233/0x270 mm/mempolicy.c:2274
alloc_slab_page mm/slub.c:1851 [inline]
allocate_slab+0x25f/0x390 mm/slub.c:1998
new_slab mm/slub.c:2051 [inline]
___slab_alloc+0xa91/0x1400 mm/slub.c:3192
__slab_alloc.constprop.0+0x56/0xa0 mm/slub.c:3291
__slab_alloc_node mm/slub.c:3344 [inline]
slab_alloc_node mm/slub.c:3441 [inline]
__kmem_cache_alloc_node+0x136/0x320 mm/slub.c:3490
__do_kmalloc_node mm/slab_common.c:965 [inline]
__kmalloc_node_track_caller+0x4f/0x1a0 mm/slab_common.c:986
kstrdup+0x3f/0x70 mm/util.c:62
kstrdup_const+0x57/0x80 mm/util.c:85
kvasprintf_const+0x10c/0x190 lib/kasprintf.c:48
kobject_set_name_vargs+0x5a/0x150 lib/kobject.c:267
dev_set_name+0xbf/0xf0 drivers/base/core.c:3429
tty_register_device_attr+0x301/0x7d0 drivers/tty/tty_io.c:3243
page last free stack trace:
reset_page_owner include/linux/page_owner.h:24 [inline]
free_pages_prepare mm/page_alloc.c:1302 [inline]
free_unref_page_prepare+0x62e/0xcb0 mm/page_alloc.c:2564
free_unref_page+0x33/0x370 mm/page_alloc.c:2659
vfree+0x180/0x7e0 mm/vmalloc.c:2798
delayed_vfree_work+0x57/0x70 mm/vmalloc.c:2719
process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
kthread+0x344/0x440 kernel/kthread.c:379
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308

Memory state around the buggy address:
ffff88801e78b780: 00 fc fc fc fc fa fc fc fc fc fa fc fc fc fc fa
ffff88801e78b800: fc fc fc fc 00 fc fc fc fc fa fc fc fc fc fa fc
>ffff88801e78b880: fc fc fc fa fc fc fc fc 00 fc fc fc fc 00 fc fc
^
ffff88801e78b900: fc fc 00 fc fc fc fc fa fc fc fc fc 00 fc fc fc
ffff88801e78b980: fc 00 fc fc fc fc fa fc fc fc fc 00 fc fc fc fc
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Khazhy Kumykov]

"src = udev->rawdescriptors[cfgno]" (so, just reading rawdescriptors)

kzmalloc(length) -> this length derived from dev->descriptor.bNumConfigurations

The corresponding kfree is in usb_destroy_configuration (makes sense)
- we also set rawdescriptors to NULL here. If this race was happening,
I'd also expect some sort of null deref report...

Stumbled upon https://lore.kernel.org/all/1599201467-11000-1-git...@hisilicon.com/T/,
which suggests that we can, instead, race with a descriptor change,
which sounds plausible - descriptor changes, bNumConfigurations no
longer lines up with our kmalloc... so we may run past the end of it.

Looking at hub_port_connect_change(), we seem to read directly into
udev->descriptor, check if it changed, and if it did, set
udev->descriptor back to the old one...? If we have an ongoing sysfs
read, which directly touches udev->descriptor, there might be
trouble...

I see this is called in both hub_port_connect_change() and
usb_reset_and_verify_device()... which both seem to lock the port_dev?
("port_dev->status_lock"). This looks like a different lock than
usb_lock_device_interruptible would grab, maybe the code has changed
since that was reported in 2020. But it seems to suggest we want to
grab this lock in sysfs to safely read from udev->descriptor.

(I'm not clear on when the sysfs gets added/removed, since it happens
in usb_bus_notify()..., the above two functions that touch
udev->descriptor don't look like they send the
BUS_NOTIFY_ADD/DEL_DEVICE to me, so the race seems plausible)

Huh, why did our page get vfree'd, when it was kmalloc'd? Maybe the
memory was reused multiple times before generating this report...?

[Khazhy Kumykov]

Ah yeah, the syzbot C repro does something like this, it has a virtual
usb and keeps changing the descs -> which may end up calling
hub_port_connect_change()

>
> Looking at hub_port_connect_change(), we seem to read directly into
> udev->descriptor, check if it changed, and if it did, set
> udev->descriptor back to the old one...? If we have an ongoing sysfs
> read, which directly touches udev->descriptor, there might be
> trouble...
>
> I see this is called in both hub_port_connect_change() and
> usb_reset_and_verify_device()... which both seem to lock the port_dev?
> ("port_dev->status_lock"). This looks like a different lock than
> usb_lock_device_interruptible would grab, maybe the code has changed
> since that was reported in 2020. But it seems to suggest we want to
> grab this lock in sysfs to safely read from udev->descriptor.
>
> (I'm not clear on when the sysfs gets added/removed, since it happens
> in usb_bus_notify()..., the above two functions that touch
> udev->descriptor don't look like they send the
> BUS_NOTIFY_ADD/DEL_DEVICE to me, so the race seems plausible)

Ah yeah - in hub_port_connect_change() we call hub_port_connect() if
the descriptor changed, which notifies us of device remove *after* we
already directly messed with udev->descriptor for a potentially live
device.

I do see there's several sysfs files that directly read
udev->descriptor with no locking - should these all need to grab the
port_dev->status_lock?

[Alan Stern]

Yes, that sounds right.

The problem-causing commit is 45bf39f8df7f ("USB: core: Don't hold
device lock while reading the "descriptors" sysfs file"). When writing
the commit message I only considered changes to the rawdescriptors; it
didn't occur to me that the device descriptor might also change, which
would be just as dangerous.

> > Looking at hub_port_connect_change(), we seem to read directly into
> > udev->descriptor, check if it changed, and if it did, set
> > udev->descriptor back to the old one...? If we have an ongoing sysfs
> > read, which directly touches udev->descriptor, there might be
> > trouble...
> >
> > I see this is called in both hub_port_connect_change() and
> > usb_reset_and_verify_device()... which both seem to lock the port_dev?
> > ("port_dev->status_lock"). This looks like a different lock than
> > usb_lock_device_interruptible would grab, maybe the code has changed
> > since that was reported in 2020. But it seems to suggest we want to
> > grab this lock in sysfs to safely read from udev->descriptor.
> >
> > (I'm not clear on when the sysfs gets added/removed, since it happens
> > in usb_bus_notify()..., the above two functions that touch
> > udev->descriptor don't look like they send the
> > BUS_NOTIFY_ADD/DEL_DEVICE to me, so the race seems plausible)
>
> Ah yeah - in hub_port_connect_change() we call hub_port_connect() if
> the descriptor changed, which notifies us of device remove *after* we
> already directly messed with udev->descriptor for a potentially live
> device.
>
> I do see there's several sysfs files that directly read
> udev->descriptor with no locking - should these all need to grab the
> port_dev->status_lock?

I suppose some of them should. (For others, the caller will already
hold the device lock.)

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.

Alan Stern

[Khazhy Kumykov]

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. :)

>
> Alan Stern

[Alan Stern]

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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+189961...@syzkaller.appspotmail.com

Tested on:

commit: fdf0eaf1 Linux 6.5-rc2
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc2
console output: https://syzkaller.appspot.com/x/log.txt?x=158187d1a80000
kernel config: https://syzkaller.appspot.com/x/.config?x=77b9a3cf8f44c6da
dashboard link: https://syzkaller.appspot.com/bug?extid=18996170f8096c6174d0
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
patch: https://syzkaller.appspot.com/x/patch.diff?x=1120305ea80000

Note: testing is done by a robot and is best-effort only.

[Alan Stern]

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+189961...@syzkaller.appspotmail.com

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

+ if (initial) {

@@ -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);
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);

[Khazhy Kumykov]

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.

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?

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+189961...@syzkaller.appspotmail.com

Tested on:

commit: 6eaae198 Linux 6.5-rc3
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc3
console output: https://syzkaller.appspot.com/x/log.txt?x=13d77b76a80000
kernel config: https://syzkaller.appspot.com/x/.config?x=c7b1aac4a6659b6d

dashboard link: https://syzkaller.appspot.com/bug?extid=18996170f8096c6174d0
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=1339684aa80000

[Alan Stern]

On Tue, Jul 25, 2023 at 01:42:01PM -0700, Khazhy Kumykov wrote:
> On Tue, Jul 25, 2023 at 12:26 PM Alan Stern <st...@rowland.harvard.edu> wrote:

> > @@ -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.

I removed that part of the previous patch because there's no point to
it. None of this code ever gets executed; the usb_dev->wusb test can
never succeed because the kernel doesn't support wireless USB any more.
(I asked Greg KH about that in a separate email thread:
<https://lore.kernel.org/linux-usb/2a21cefa-99a7-497c...@rowland.harvard.edu/#r>.)

A later patch will remove all of the wireless USB stuff. The only real
reason for leaving this much of the code there now is to prevent
compilation errors and keep the form looking right.

> > @@ -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?

The descriptors_changed check has to be _somewhere_, either here or
there. I don't see what difference it makes whether the check is done
in this routine or in hub_port_init. Since it doesn't matter, why
change the existing code?

Alan Stern

[Khazhy Kumykov]

Ah ok, cool.

>
> > > @@ -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?
>
> The descriptors_changed check has to be _somewhere_, either here or
> there. I don't see what difference it makes whether the check is done
> in this routine or in hub_port_init. Since it doesn't matter, why
> change the existing code?

No strong feelings, but it lets us remove the variable in
usb_reset_and_verify_device() and directly check on the malloc'd copy,
instead of copying back up to here.

Overall, looks good to my naive eyes.

CVE-2023-37453 was filed for this syzbot report, I'm not sure how that
system gets tracked, but might be good to mention for folks looking
for a fix.

Thanks,
Khazhy

[Greg KH]

Who filed CVEs for random syzbot reports? That's crazy, and no, we
aren't going to track it as CVEs mean nothing for the kernel as I've
said many times.

thanks,

greg k-h

[Alan Stern]

Greg:

I'm putting together a series of three patches to deal with this
problem. The first two do some preparatory work, and the bug found by
syzbot actually gets fixed by the third patch.

In view of this, how should I tag the patches? They should all get into
the -stable trees eventually, but only the third one really deserves a
Fixes: or Reported-by: tag.

What's your advice?

Alan Stern

[Greg KH]

Sounds correct, just tag the last one with a reported-by: Same for the
fixes: tag, we deal with this in the stable tree a lot, and can figure
out the dependancies like this for patch series.

thanks,

greg k-h