LinuxLists.cc - [syzbot] [usb?] KASAN: slab-out-of-bounds Read in read

[permalink] [raw]

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

On Mon, Jun 19, 2023 at 7:56 PM syzbot
<[email protected]> wrote:
>
> 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: [email protected]
>
> ==================================================================
> 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

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

> 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]

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/[email protected]/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)

> 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
Huh, why did our page get vfree'd, when it was kmalloc'd? Maybe the
memory was reused multiple times before generating this report...?
> 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 [email protected].
>
> 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

Attachments:

2023-07-21 18:54:19

[permalink] [raw]

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

On Fri, Jul 21, 2023 at 11:10 AM Khazhy Kumykov <[email protected]> wrote:
>
> On Mon, Jun 19, 2023 at 7:56 PM syzbot
> <[email protected]> wrote:
> >
> > 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: [email protected]
> >
> > ==================================================================
> > 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
>
> "src = udev->rawdescriptors[cfgno]" (so, just reading rawdescriptors)
>
> > 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]
>
> 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/[email protected]/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.
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?

>
> > 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
> Huh, why did our page get vfree'd, when it was kmalloc'd? Maybe the
> memory was reused multiple times before generating this report...?
> > 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 [email protected].
> >
> > 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

Attachments:

2023-07-21 19:37:27

[permalink] [raw]

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

On Fri, Jul 21, 2023 at 11:23:10AM -0700, Khazhy Kumykov wrote:
> On Fri, Jul 21, 2023 at 11:10 AM Khazhy Kumykov <[email protected]> wrote:
> >
> > On Mon, Jun 19, 2023 at 7:56 PM syzbot
> > <[email protected]> wrote:
> > >
> > > 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: [email protected]
> > >
> > > ==================================================================
> > > 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
> >
> > "src = udev->rawdescriptors[cfgno]" (so, just reading rawdescriptors)
> >
> > > 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]
> >
> > 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/[email protected]/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.
> 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()

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

2023-07-21 23:58:02

[permalink] [raw]

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

On Fri, Jul 21, 2023 at 11:56 AM Alan Stern <[email protected]> wrote:
>
> On Fri, Jul 21, 2023 at 11:23:10AM -0700, Khazhy Kumykov wrote:
> > On Fri, Jul 21, 2023 at 11:10 AM Khazhy Kumykov <[email protected]> wrote:
> > >
> > > On Mon, Jun 19, 2023 at 7:56 PM syzbot
> > > <[email protected]> wrote:
> > > >
> > > > 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: [email protected]
> > > >
> > > > ==================================================================
> > > > 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
> > >
> > > "src = udev->rawdescriptors[cfgno]" (so, just reading rawdescriptors)
> > >
> > > > 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]
> > >
> > > 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/[email protected]/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.
> > 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()
>
> 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.

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

Attachments:

2023-07-23 02:51:51

by syzbot

[permalink] [raw]

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

Hello,

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

Reported-and-tested-by: [email protected]

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.

2023-07-23 04:05:19

[permalink] [raw]

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

On Fri, Jul 21, 2023 at 03:40:01PM -0700, Khazhy Kumykov wrote:
> On Fri, Jul 21, 2023 at 11:56 AM Alan Stern <[email protected]> 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;
}

2023-07-25 19:37:11

[permalink] [raw]

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

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: [email protected]

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

2023-07-25 21:18:51

[permalink] [raw]

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

On Tue, Jul 25, 2023 at 12:26 PM Alan Stern <[email protected]> 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: [email protected]
>
> 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);

Attachments:

2023-07-25 21:35:21

by syzbot

[permalink] [raw]

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

Hello,

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

Reported-and-tested-by: [email protected]

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

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

2023-07-25 21:53:19

[permalink] [raw]

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

On Tue, Jul 25, 2023 at 01:42:01PM -0700, Khazhy Kumykov wrote:
> On Tue, Jul 25, 2023 at 12:26 PM Alan Stern <[email protected]> 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/[email protected]/#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

2023-07-25 22:04:13