* KASAN: use-after-free Read in usb_udc_uevent
@ 2020-06-02 13:21 syzbot
2022-07-20 18:03 ` [syzbot] " syzbot
0 siblings, 1 reply; 16+ messages in thread
From: syzbot @ 2020-06-02 13:21 UTC (permalink / raw)
To: balbi, gregkh, linux-kernel, linux-usb, rogerq, stern,
syzkaller-bugs, zhengdejin5
Hello,
syzbot found the following crash on:
HEAD commit: ffeb595d Merge tag 'powerpc-5.7-6' of git://git.kernel.org..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17f7f2b1100000
kernel config: https://syzkaller.appspot.com/x/.config?x=9ac87c7c2ba15abf
dashboard link: https://syzkaller.appspot.com/bug?extid=b0de012ceb1e2a97891b
compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in usb_udc_uevent+0xac/0x120 drivers/usb/gadget/udc/core.c:1593
Read of size 8 at addr ffff888091a7c050 by task systemd-udevd/14223
CPU: 1 PID: 14223 Comm: systemd-udevd Not tainted 5.7.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1e9/0x30e lib/dump_stack.c:118
print_address_description+0x74/0x5c0 mm/kasan/report.c:382
__kasan_report+0x103/0x1a0 mm/kasan/report.c:511
kasan_report+0x4d/0x80 mm/kasan/common.c:625
Allocated by task 14178:
save_stack mm/kasan/common.c:49 [inline]
set_track mm/kasan/common.c:57 [inline]
__kasan_kmalloc+0x114/0x160 mm/kasan/common.c:495
kmem_cache_alloc_trace+0x234/0x300 mm/slab.c:3551
kmalloc include/linux/slab.h:555 [inline]
kzalloc include/linux/slab.h:669 [inline]
dev_new drivers/usb/gadget/legacy/raw_gadget.c:182 [inline]
raw_open+0x87/0x500 drivers/usb/gadget/legacy/raw_gadget.c:372
misc_open+0x346/0x3c0 drivers/char/misc.c:141
chrdev_open+0x498/0x580 fs/char_dev.c:414
do_dentry_open+0x82e/0x10b0 fs/open.c:797
do_open fs/namei.c:3229 [inline]
path_openat+0x2790/0x38b0 fs/namei.c:3346
do_filp_open+0x191/0x3a0 fs/namei.c:3373
do_sys_openat2+0x463/0x770 fs/open.c:1148
do_sys_open fs/open.c:1164 [inline]
ksys_open include/linux/syscalls.h:1386 [inline]
__do_sys_open fs/open.c:1170 [inline]
__se_sys_open fs/open.c:1168 [inline]
__x64_sys_open+0x1af/0x1e0 fs/open.c:1168
do_syscall_64+0xf3/0x1b0 arch/x86/entry/common.c:295
entry_SYSCALL_64_after_hwframe+0x49/0xb3
Freed by task 14174:
save_stack mm/kasan/common.c:49 [inline]
set_track mm/kasan/common.c:57 [inline]
kasan_set_free_info mm/kasan/common.c:317 [inline]
__kasan_slab_free+0x125/0x190 mm/kasan/common.c:456
__cache_free mm/slab.c:3426 [inline]
kfree+0x10a/0x220 mm/slab.c:3757
raw_release+0x130/0x1e0 drivers/usb/gadget/legacy/raw_gadget.c:411
__fput+0x2ed/0x750 fs/file_table.c:280
task_work_run+0x147/0x1d0 kernel/task_work.c:123
exit_task_work include/linux/task_work.h:22 [inline]
do_exit+0x5ef/0x1f80 kernel/exit.c:796
do_group_exit+0x15e/0x2c0 kernel/exit.c:894
get_signal+0x13cf/0x1d60 kernel/signal.c:2739
do_signal+0x33/0x610 arch/x86/kernel/signal.c:784
exit_to_usermode_loop arch/x86/entry/common.c:161 [inline]
prepare_exit_to_usermode+0x32a/0x600 arch/x86/entry/common.c:196
entry_SYSCALL_64_after_hwframe+0x49/0xb3
The buggy address belongs to the object at ffff888091a7c000
which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 80 bytes inside of
4096-byte region [ffff888091a7c000, ffff888091a7d000)
The buggy address belongs to the page:
page:ffffea0002469f00 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 head:ffffea0002469f00 order:1 compound_mapcount:0
flags: 0xfffe0000010200(slab|head)
raw: 00fffe0000010200 ffffea0001043588 ffffea0001258b88 ffff8880aa402000
raw: 0000000000000000 ffff888091a7c000 0000000100000001 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff888091a7bf00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff888091a7bf80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888091a7c000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888091a7c080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888091a7c100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This bug 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 syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [syzbot] KASAN: use-after-free Read in usb_udc_uevent 2020-06-02 13:21 KASAN: use-after-free Read in usb_udc_uevent syzbot @ 2022-07-20 18:03 ` syzbot 2022-07-21 13:58 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: syzbot @ 2022-07-20 18:03 UTC (permalink / raw) To: andriy.shevchenko, balbi, gregkh, linux-kernel, linux-usb, rogerq, stern, syzkaller-bugs, zhengdejin5 syzbot has found a reproducer for the following issue on: HEAD commit: cb71b93c2dc3 Add linux-next specific files for 20220628 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=172591aa080000 kernel config: https://syzkaller.appspot.com/x/.config?x=badbc1adb2d582eb dashboard link: https://syzkaller.appspot.com/bug?extid=b0de012ceb1e2a97891b 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=13ab4d62080000 IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com ================================================================== BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 Read of size 8 at addr ffff888078ce2050 by task udevd/2968 CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 print_address_description mm/kasan/report.c:317 [inline] print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 dev_uevent+0x290/0x770 drivers/base/core.c:2424 uevent_show+0x1b8/0x380 drivers/base/core.c:2480 dev_attr_show+0x4b/0x90 drivers/base/core.c:2183 sysfs_kf_seq_show+0x219/0x3d0 fs/sysfs/file.c:59 seq_read_iter+0x4f5/0x1280 fs/seq_file.c:230 kernfs_fop_read_iter+0x506/0x6e0 fs/kernfs/file.c:235 call_read_iter include/linux/fs.h:2182 [inline] new_sync_read+0x314/0x560 fs/read_write.c:401 vfs_read+0x492/0x5d0 fs/read_write.c:482 ksys_read+0x127/0x250 fs/read_write.c:620 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 RIP: 0033:0x7f996d1258fe Code: c0 e9 e6 fe ff ff 50 48 8d 3d 0e c7 09 00 e8 c9 cf 01 00 66 0f 1f 84 00 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 0f 05 <48> 3d 00 f0 ff ff 77 5a c3 66 0f 1f 84 00 00 00 00 00 48 83 ec 28 RSP: 002b:00007ffcfcbe6368 EFLAGS: 00000246 ORIG_RAX: 0000000000000000 RAX: ffffffffffffffda RBX: 000055ca13b297a0 RCX: 00007f996d1258fe RDX: 0000000000001000 RSI: 000055ca13b5a4e0 RDI: 000000000000000c RBP: 00007f996d1f2380 R08: 000000000000000c R09: 00007f996d1f5a60 R10: 0000000000000800 R11: 0000000000000246 R12: 000055ca13b297a0 R13: 0000000000000d68 R14: 00007f996d1f1780 R15: 0000000000000d68 </TASK> Allocated by task 4797: kasan_save_stack+0x1e/0x40 mm/kasan/common.c:38 kasan_set_track mm/kasan/common.c:45 [inline] set_alloc_info mm/kasan/common.c:436 [inline] ____kasan_kmalloc mm/kasan/common.c:515 [inline] ____kasan_kmalloc mm/kasan/common.c:474 [inline] __kasan_kmalloc+0xa9/0xd0 mm/kasan/common.c:524 kmalloc include/linux/slab.h:600 [inline] kzalloc include/linux/slab.h:733 [inline] dev_new drivers/usb/gadget/legacy/raw_gadget.c:191 [inline] raw_open+0x87/0x500 drivers/usb/gadget/legacy/raw_gadget.c:385 misc_open+0x376/0x4a0 drivers/char/misc.c:143 chrdev_open+0x266/0x770 fs/char_dev.c:414 do_dentry_open+0x4a1/0x11f0 fs/open.c:878 do_open fs/namei.c:3520 [inline] path_openat+0x1c71/0x2930 fs/namei.c:3653 do_filp_open+0x1aa/0x400 fs/namei.c:3680 do_sys_openat2+0x16d/0x4c0 fs/open.c:1308 do_sys_open fs/open.c:1324 [inline] __do_sys_openat fs/open.c:1340 [inline] __se_sys_openat fs/open.c:1335 [inline] __x64_sys_openat+0x13f/0x1f0 fs/open.c:1335 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 Freed by task 4797: kasan_save_stack+0x1e/0x40 mm/kasan/common.c:38 kasan_set_track+0x21/0x30 mm/kasan/common.c:45 kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:370 ____kasan_slab_free mm/kasan/common.c:366 [inline] ____kasan_slab_free+0x166/0x1c0 mm/kasan/common.c:328 kasan_slab_free include/linux/kasan.h:200 [inline] slab_free_hook mm/slub.c:1754 [inline] slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1780 slab_free mm/slub.c:3534 [inline] kfree+0xe2/0x4d0 mm/slub.c:4562 kref_put include/linux/kref.h:65 [inline] raw_release+0x219/0x290 drivers/usb/gadget/legacy/raw_gadget.c:424 __fput+0x277/0x9d0 fs/file_table.c:317 task_work_run+0xdd/0x1a0 kernel/task_work.c:177 exit_task_work include/linux/task_work.h:38 [inline] do_exit+0xaf1/0x29f0 kernel/exit.c:795 do_group_exit+0xd2/0x2f0 kernel/exit.c:925 get_signal+0x2542/0x2600 kernel/signal.c:2857 arch_do_signal_or_restart+0x82/0x2300 arch/x86/kernel/signal.c:869 exit_to_user_mode_loop kernel/entry/common.c:166 [inline] exit_to_user_mode_prepare+0x15f/0x250 kernel/entry/common.c:201 __syscall_exit_to_user_mode_work kernel/entry/common.c:283 [inline] syscall_exit_to_user_mode+0x19/0x50 kernel/entry/common.c:294 do_syscall_64+0x42/0xb0 arch/x86/entry/common.c:86 entry_SYSCALL_64_after_hwframe+0x46/0xb0 The buggy address belongs to the object at ffff888078ce2000 which belongs to the cache kmalloc-4k of size 4096 The buggy address is located 80 bytes inside of 4096-byte region [ffff888078ce2000, ffff888078ce3000) The buggy address belongs to the physical page: page:ffffea0001e33800 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x78ce0 head:ffffea0001e33800 order:3 compound_mapcount:0 compound_pincount:0 flags: 0xfff00000010200(slab|head|node=0|zone=1|lastcpupid=0x7ff) raw: 00fff00000010200 0000000000000000 dead000000000001 ffff888011842140 raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000 page dumped because: kasan: bad access detected page_owner tracks the page as allocated page last allocated via order 3, migratetype Unmovable, gfp_mask 0xd20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC), pid 3618, tgid 3618 (syz-executor.2), ts 66675676094, free_ts 28120915618 prep_new_page mm/page_alloc.c:2535 [inline] get_page_from_freelist+0x210d/0x3a30 mm/page_alloc.c:4282 __alloc_pages+0x1c7/0x510 mm/page_alloc.c:5506 alloc_pages+0x1aa/0x310 mm/mempolicy.c:2280 alloc_slab_page mm/slub.c:1824 [inline] allocate_slab+0x27e/0x3d0 mm/slub.c:1969 new_slab mm/slub.c:2029 [inline] ___slab_alloc+0x89d/0xef0 mm/slub.c:3031 __slab_alloc.constprop.0+0x4d/0xa0 mm/slub.c:3118 slab_alloc_node mm/slub.c:3209 [inline] slab_alloc mm/slub.c:3251 [inline] kmem_cache_alloc_trace+0x323/0x3e0 mm/slub.c:3282 kmalloc include/linux/slab.h:600 [inline] kzalloc include/linux/slab.h:733 [inline] kobject_uevent_env+0x230/0x1640 lib/kobject_uevent.c:524 netdev_queue_add_kobject net/core/net-sysfs.c:1677 [inline] netdev_queue_update_kobjects+0x3d1/0x4e0 net/core/net-sysfs.c:1718 register_queue_kobjects net/core/net-sysfs.c:1779 [inline] netdev_register_kobject+0x330/0x400 net/core/net-sysfs.c:2019 register_netdevice+0xd9d/0x15e0 net/core/dev.c:10065 bond_newlink drivers/net/bonding/bond_netlink.c:560 [inline] bond_newlink+0x47/0xa0 drivers/net/bonding/bond_netlink.c:550 rtnl_newlink_create net/core/rtnetlink.c:3363 [inline] __rtnl_newlink+0x1087/0x17e0 net/core/rtnetlink.c:3580 rtnl_newlink+0x64/0xa0 net/core/rtnetlink.c:3593 rtnetlink_rcv_msg+0x43a/0xc90 net/core/rtnetlink.c:6089 netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2501 page last free stack trace: reset_page_owner include/linux/page_owner.h:24 [inline] free_pages_prepare mm/page_alloc.c:1453 [inline] free_pcp_prepare+0x5e4/0xd20 mm/page_alloc.c:1503 free_unref_page_prepare mm/page_alloc.c:3383 [inline] free_unref_page+0x19/0x4d0 mm/page_alloc.c:3479 __unfreeze_partials+0x17c/0x1a0 mm/slub.c:2548 qlink_free mm/kasan/quarantine.c:168 [inline] qlist_free_all+0x6a/0x170 mm/kasan/quarantine.c:187 kasan_quarantine_reduce+0x180/0x200 mm/kasan/quarantine.c:294 __kasan_slab_alloc+0xa2/0xc0 mm/kasan/common.c:446 kasan_slab_alloc include/linux/kasan.h:224 [inline] slab_post_alloc_hook mm/slab.h:736 [inline] kmem_cache_alloc_bulk+0x383/0x730 mm/slub.c:3735 mt_alloc_bulk lib/maple_tree.c:151 [inline] mas_alloc_nodes+0x2b0/0x6b0 lib/maple_tree.c:1244 mas_preallocate+0xff/0x2d0 lib/maple_tree.c:5662 __vma_adjust+0x226/0x1900 mm/mmap.c:765 vma_adjust include/linux/mm.h:2678 [inline] __split_vma+0x295/0x530 mm/mmap.c:2305 split_vma+0x9f/0xe0 mm/mmap.c:2335 mprotect_fixup+0x746/0x960 mm/mprotect.c:613 do_mprotect_pkey+0x70f/0xa80 mm/mprotect.c:781 __do_sys_mprotect mm/mprotect.c:808 [inline] __se_sys_mprotect mm/mprotect.c:805 [inline] __x64_sys_mprotect+0x74/0xb0 mm/mprotect.c:805 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 Memory state around the buggy address: ffff888078ce1f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc ffff888078ce1f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >ffff888078ce2000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ^ ffff888078ce2080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ffff888078ce2100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ================================================================== ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [syzbot] KASAN: use-after-free Read in usb_udc_uevent 2022-07-20 18:03 ` [syzbot] " syzbot @ 2022-07-21 13:58 ` Alan Stern 2022-07-21 14:26 ` syzbot 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2022-07-21 13:58 UTC (permalink / raw) To: syzbot Cc: andriy.shevchenko, balbi, gregkh, linux-kernel, linux-usb, rogerq, syzkaller-bugs, zhengdejin5 On Wed, Jul 20, 2022 at 11:03:24AM -0700, syzbot wrote: > syzbot has found a reproducer for the following issue on: > > HEAD commit: cb71b93c2dc3 Add linux-next specific files for 20220628 > git tree: linux-next > console output: https://syzkaller.appspot.com/x/log.txt?x=172591aa080000 > kernel config: https://syzkaller.appspot.com/x/.config?x=badbc1adb2d582eb > dashboard link: https://syzkaller.appspot.com/bug?extid=b0de012ceb1e2a97891b > 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=13ab4d62080000 > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com > > ================================================================== > BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 > Read of size 8 at addr ffff888078ce2050 by task udevd/2968 > > CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > print_address_description mm/kasan/report.c:317 [inline] > print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 > kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 > usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 > dev_uevent+0x290/0x770 drivers/base/core.c:2424 > uevent_show+0x1b8/0x380 drivers/base/core.c:2480 It looks like the usb_udc_uevent call races with gadget removal. The problem is that usb_udc_uevent accesses udc->driver but does not hold the udc_lock mutex (which protects this field) while doing so. Alan Stern #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ cb71b93c2dc3 Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -1728,13 +1728,14 @@ static int usb_udc_uevent(struct device return ret; } - if (udc->driver) { + mutex_lock(&udc_lock); + if (udc->driver) ret = add_uevent_var(env, "USB_UDC_DRIVER=%s", udc->driver->function); - if (ret) { - dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); - return ret; - } + mutex_unlock(&udc_lock); + if (ret) { + dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); + return ret; } return 0; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [syzbot] KASAN: use-after-free Read in usb_udc_uevent 2022-07-21 13:58 ` Alan Stern @ 2022-07-21 14:26 ` syzbot 2022-07-21 15:07 ` [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: syzbot @ 2022-07-21 14:26 UTC (permalink / raw) To: andriy.shevchenko, balbi, gregkh, linux-kernel, linux-usb, rogerq, stern, syzkaller-bugs, zhengdejin5 Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com Tested on: commit: cb71b93c Add linux-next specific files for 20220628 git tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ console output: https://syzkaller.appspot.com/x/log.txt?x=1124f6d6080000 kernel config: https://syzkaller.appspot.com/x/.config?x=badbc1adb2d582eb dashboard link: https://syzkaller.appspot.com/bug?extid=b0de012ceb1e2a97891b compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 patch: https://syzkaller.appspot.com/x/patch.diff?x=177cc2ba080000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-07-21 14:26 ` syzbot @ 2022-07-21 15:07 ` Alan Stern 2022-07-27 12:17 ` Greg KH [not found] ` <CGME20220808145736eucas1p234e56422bd7973d7f0676e74f03ba405@eucas1p2.samsung.com> 0 siblings, 2 replies; 16+ messages in thread From: Alan Stern @ 2022-07-21 15:07 UTC (permalink / raw) To: Greg KH, Felipe Balbi; +Cc: USB mailing list, syzkaller-bugs The syzbot fuzzer found a race between uevent callbacks and gadget driver unregistration that can cause a use-after-free bug: --------------------------------------------------------------- BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 Read of size 8 at addr ffff888078ce2050 by task udevd/2968 CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 print_address_description mm/kasan/report.c:317 [inline] print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 dev_uevent+0x290/0x770 drivers/base/core.c:2424 --------------------------------------------------------------- The bug occurs because usb_udc_uevent() dereferences udc->driver but does so without acquiring the udc_lock mutex, which protects this field. If the gadget driver is unbound from the udc concurrently with uevent processing, the driver structure may be accessed after it has been deallocated. To prevent the race, we make sure that the routine holds the mutex around the racing accesses. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com CC: stable@vger.kernel.org Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com> --- As far as I can tell, this bug has always been present. However, the udc_lock mutex used by the patch was added in commit fc274c1e9973 ("USB: gadget: Add a new bus for gadgets"), so this patch won't apply to trees which don't include that commit or a backport of it. I don't know what tag, if any, can express this requirement. [as1983] drivers/usb/gadget/udc/core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -1728,13 +1728,14 @@ static int usb_udc_uevent(struct device return ret; } - if (udc->driver) { + mutex_lock(&udc_lock); + if (udc->driver) ret = add_uevent_var(env, "USB_UDC_DRIVER=%s", udc->driver->function); - if (ret) { - dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); - return ret; - } + mutex_unlock(&udc_lock); + if (ret) { + dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); + return ret; } return 0; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-07-21 15:07 ` [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() Alan Stern @ 2022-07-27 12:17 ` Greg KH [not found] ` <CGME20220808145736eucas1p234e56422bd7973d7f0676e74f03ba405@eucas1p2.samsung.com> 1 sibling, 0 replies; 16+ messages in thread From: Greg KH @ 2022-07-27 12:17 UTC (permalink / raw) To: Alan Stern; +Cc: Felipe Balbi, USB mailing list, syzkaller-bugs On Thu, Jul 21, 2022 at 11:07:10AM -0400, Alan Stern wrote: > The syzbot fuzzer found a race between uevent callbacks and gadget > driver unregistration that can cause a use-after-free bug: > > --------------------------------------------------------------- > BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 > drivers/usb/gadget/udc/core.c:1732 > Read of size 8 at addr ffff888078ce2050 by task udevd/2968 > > CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google > 06/29/2022 > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > print_address_description mm/kasan/report.c:317 [inline] > print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 > kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 > usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 > dev_uevent+0x290/0x770 drivers/base/core.c:2424 > --------------------------------------------------------------- > > The bug occurs because usb_udc_uevent() dereferences udc->driver but > does so without acquiring the udc_lock mutex, which protects this > field. If the gadget driver is unbound from the udc concurrently with > uevent processing, the driver structure may be accessed after it has > been deallocated. > > To prevent the race, we make sure that the routine holds the mutex > around the racing accesses. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com > CC: stable@vger.kernel.org > Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com> > > --- > > As far as I can tell, this bug has always been present. However, the > udc_lock mutex used by the patch was added in commit fc274c1e9973 > ("USB: gadget: Add a new bus for gadgets"), so this patch won't apply > to trees which don't include that commit or a backport of it. I don't > know what tag, if any, can express this requirement. As per the stable_kernel_rules document, you can say: cc: stable@vger.kernel.org # fc274c1e9973 and I should hopefully figure it out :) I'll add that here and see how well it works... thanks, greg k-h ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <CGME20220808145736eucas1p234e56422bd7973d7f0676e74f03ba405@eucas1p2.samsung.com>]
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() [not found] ` <CGME20220808145736eucas1p234e56422bd7973d7f0676e74f03ba405@eucas1p2.samsung.com> @ 2022-08-08 14:57 ` Marek Szyprowski 2022-08-08 20:26 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Marek Szyprowski @ 2022-08-08 14:57 UTC (permalink / raw) To: Alan Stern, Greg KH, Felipe Balbi; +Cc: USB mailing list, syzkaller-bugs Hi Alan, On 21.07.2022 17:07, Alan Stern wrote: > The syzbot fuzzer found a race between uevent callbacks and gadget > driver unregistration that can cause a use-after-free bug: > > --------------------------------------------------------------- > BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 > drivers/usb/gadget/udc/core.c:1732 > Read of size 8 at addr ffff888078ce2050 by task udevd/2968 > > CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google > 06/29/2022 > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > print_address_description mm/kasan/report.c:317 [inline] > print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 > kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 > usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 > dev_uevent+0x290/0x770 drivers/base/core.c:2424 > --------------------------------------------------------------- > > The bug occurs because usb_udc_uevent() dereferences udc->driver but > does so without acquiring the udc_lock mutex, which protects this > field. If the gadget driver is unbound from the udc concurrently with > uevent processing, the driver structure may be accessed after it has > been deallocated. > > To prevent the race, we make sure that the routine holds the mutex > around the racing accesses. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com > CC: stable@vger.kernel.org > Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it fixes the issue by introducing another one. It doesn't look very probable, but it would be nice to fix it to make the lock dependency checker happy. ====================================================== WARNING: possible circular locking dependency detected 5.19.0-rc7+ #12510 Not tainted ------------------------------------------------------ udevadm/312 is trying to acquire lock: ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0 but task is already holding lock: ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (kn->active#4){++++}-{0:0}: lock_acquire+0x68/0x84 __kernfs_remove+0x268/0x380 kernfs_remove_by_name_ns+0x58/0xac sysfs_remove_file_ns+0x18/0x24 device_del+0x15c/0x440 device_link_drop_managed+0xa8/0xe0 device_links_driver_bound+0x1b8/0x230 driver_bound+0x68/0xc0 really_probe.part.0+0x1f8/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __driver_attach+0x104/0x1b0 bus_for_each_dev+0x70/0xd0 driver_attach+0x24/0x30 bus_add_driver+0x154/0x204 driver_register+0x78/0x130 __platform_driver_register+0x28/0x34 simple_pm_bus_driver_init+0x1c/0x28 do_one_initcall+0x74/0x400 kernel_init_freeable+0x2f4/0x37c kernel_init+0x28/0x130 ret_from_fork+0x10/0x20 -> #2 (device_links_lock){+.+.}-{3:3}: lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 device_link_remove+0x3c/0xa0 _regulator_put.part.0+0x168/0x190 regulator_put+0x3c/0x54 devm_regulator_release+0x14/0x20 release_nodes+0x5c/0x90 devres_release_all+0x8c/0xe0 device_unbind_cleanup+0x18/0x70 really_probe.part.0+0x174/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach_async_helper+0xb0/0xd4 async_run_entry_fn+0x34/0xd0 process_one_work+0x288/0x6bc worker_thread+0x74/0x450 kthread+0x118/0x11c ret_from_fork+0x10/0x20 -> #1 (regulator_list_mutex){+.+.}-{3:3}: lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 regulator_lock_dependent+0x54/0x284 regulator_enable+0x34/0x80 phy_power_on+0x24/0x130 __dwc2_lowlevel_hw_enable+0x100/0x130 dwc2_lowlevel_hw_enable+0x18/0x40 dwc2_hsotg_udc_start+0x6c/0x2f0 gadget_bind_driver+0x124/0x1f4 really_probe.part.0+0x9c/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach+0xa8/0x1c0 device_initial_probe+0x14/0x20 bus_probe_device+0x9c/0xa4 device_add+0x3a0/0x890 usb_add_gadget+0x170/0x200 usb_add_gadget_udc+0x94/0xd4 dwc2_driver_probe+0x580/0x78c platform_probe+0x68/0xe0 really_probe.part.0+0x9c/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach+0xa8/0x1c0 device_initial_probe+0x14/0x20 bus_probe_device+0x9c/0xa4 device_add+0x3a0/0x890 of_device_add+0x48/0x6c of_platform_device_create_pdata+0x98/0x100 of_platform_bus_create+0x17c/0x37c of_platform_populate+0x58/0xec dwc3_meson_g12a_probe+0x314/0x5d0 platform_probe+0x68/0xe0 really_probe.part.0+0x9c/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach+0xa8/0x1c0 device_initial_probe+0x14/0x20 bus_probe_device+0x9c/0xa4 deferred_probe_work_func+0x88/0xc4 process_one_work+0x288/0x6bc worker_thread+0x74/0x450 kthread+0x118/0x11c ret_from_fork+0x10/0x20 -> #0 (udc_lock){+.+.}-{3:3}: __lock_acquire+0x1298/0x20cc lock_acquire.part.0+0xe0/0x230 lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 usb_udc_uevent+0x54/0xe0 dev_uevent+0xb8/0x1ec uevent_show+0x8c/0x114 dev_attr_show+0x20/0x60 sysfs_kf_seq_show+0xa8/0x120 kernfs_seq_show+0x2c/0x40 seq_read_iter+0x1bc/0x4b0 kernfs_fop_read_iter+0x140/0x1d0 new_sync_read+0xd4/0x150 vfs_read+0x190/0x1dc ksys_read+0x68/0xfc __arm64_sys_read+0x20/0x30 invoke_syscall+0x48/0x114 el0_svc_common.constprop.0+0x60/0x11c do_el0_svc_compat+0x1c/0x50 el0_svc_compat+0x58/0x100 el0t_32_sync_handler+0x90/0x140 el0t_32_sync+0x190/0x194 other info that might help us debug this: Chain exists of: udc_lock --> device_links_lock --> kn->active#4 Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(kn->active#4); lock(device_links_lock); lock(kn->active#4); lock(udc_lock); *** DEADLOCK *** 3 locks held by udevadm/312: #0: ffff000001ab80a0 (&p->lock){+.+.}-{3:3}, at: seq_read_iter+0x60/0x4b0 #1: ffff00003cdf7e88 (&of->mutex){+.+.}-{3:3}, at: kernfs_seq_start+0x2c/0xe0 #2: ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 stack backtrace: CPU: 2 PID: 312 Comm: udevadm Not tainted 5.19.0-rc7+ #12510 Hardware name: Khadas VIM3 (DT) Call trace: dump_backtrace.part.0+0xd0/0xe0 show_stack+0x18/0x6c dump_stack_lvl+0x8c/0xb8 dump_stack+0x18/0x34 print_circular_bug+0x1f8/0x200 check_noncircular+0x130/0x144 __lock_acquire+0x1298/0x20cc lock_acquire.part.0+0xe0/0x230 lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 usb_udc_uevent+0x54/0xe0 dev_uevent+0xb8/0x1ec uevent_show+0x8c/0x114 dev_attr_show+0x20/0x60 sysfs_kf_seq_show+0xa8/0x120 kernfs_seq_show+0x2c/0x40 seq_read_iter+0x1bc/0x4b0 kernfs_fop_read_iter+0x140/0x1d0 new_sync_read+0xd4/0x150 vfs_read+0x190/0x1dc ksys_read+0x68/0xfc __arm64_sys_read+0x20/0x30 invoke_syscall+0x48/0x114 el0_svc_common.constprop.0+0x60/0x11c do_el0_svc_compat+0x1c/0x50 el0_svc_compat+0x58/0x100 el0t_32_sync_handler+0x90/0x140 el0t_32_sync+0x190/0x194 > --- > > As far as I can tell, this bug has always been present. However, the > udc_lock mutex used by the patch was added in commit fc274c1e9973 > ("USB: gadget: Add a new bus for gadgets"), so this patch won't apply > to trees which don't include that commit or a backport of it. I don't > know what tag, if any, can express this requirement. > > > [as1983] > > > drivers/usb/gadget/udc/core.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > Index: usb-devel/drivers/usb/gadget/udc/core.c > =================================================================== > --- usb-devel.orig/drivers/usb/gadget/udc/core.c > +++ usb-devel/drivers/usb/gadget/udc/core.c > @@ -1728,13 +1728,14 @@ static int usb_udc_uevent(struct device > return ret; > } > > - if (udc->driver) { > + mutex_lock(&udc_lock); > + if (udc->driver) > ret = add_uevent_var(env, "USB_UDC_DRIVER=%s", > udc->driver->function); > - if (ret) { > - dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); > - return ret; > - } > + mutex_unlock(&udc_lock); > + if (ret) { > + dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); > + return ret; > } > > return 0; > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-08 14:57 ` Marek Szyprowski @ 2022-08-08 20:26 ` Alan Stern 2022-08-09 6:29 ` Marek Szyprowski 2022-08-10 19:33 ` Alan Stern 0 siblings, 2 replies; 16+ messages in thread From: Alan Stern @ 2022-08-08 20:26 UTC (permalink / raw) To: Marek Szyprowski Cc: Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > Hi Alan, Hi. > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > fixes the issue by introducing another one. It doesn't look very > probable, but it would be nice to fix it to make the lock dependency > checker happy. Indeed. > ====================================================== > WARNING: possible circular locking dependency detected > 5.19.0-rc7+ #12510 Not tainted > ------------------------------------------------------ > udevadm/312 is trying to acquire lock: > ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0 > > but task is already holding lock: > ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #3 (kn->active#4){++++}-{0:0}: > lock_acquire+0x68/0x84 > __kernfs_remove+0x268/0x380 > kernfs_remove_by_name_ns+0x58/0xac > sysfs_remove_file_ns+0x18/0x24 > device_del+0x15c/0x440 > device_link_drop_managed+0xa8/0xe0 > device_links_driver_bound+0x1b8/0x230 > driver_bound+0x68/0xc0 > really_probe.part.0+0x1f8/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __driver_attach+0x104/0x1b0 > bus_for_each_dev+0x70/0xd0 > driver_attach+0x24/0x30 > bus_add_driver+0x154/0x204 > driver_register+0x78/0x130 > __platform_driver_register+0x28/0x34 > simple_pm_bus_driver_init+0x1c/0x28 > do_one_initcall+0x74/0x400 > kernel_init_freeable+0x2f4/0x37c > kernel_init+0x28/0x130 > ret_from_fork+0x10/0x20 > > -> #2 (device_links_lock){+.+.}-{3:3}: > lock_acquire+0x68/0x84 > __mutex_lock+0x9c/0x430 > mutex_lock_nested+0x38/0x64 > device_link_remove+0x3c/0xa0 > _regulator_put.part.0+0x168/0x190 > regulator_put+0x3c/0x54 > devm_regulator_release+0x14/0x20 > release_nodes+0x5c/0x90 > devres_release_all+0x8c/0xe0 > device_unbind_cleanup+0x18/0x70 > really_probe.part.0+0x174/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach_async_helper+0xb0/0xd4 > async_run_entry_fn+0x34/0xd0 > process_one_work+0x288/0x6bc > worker_thread+0x74/0x450 > kthread+0x118/0x11c > ret_from_fork+0x10/0x20 > > -> #1 (regulator_list_mutex){+.+.}-{3:3}: > lock_acquire+0x68/0x84 > __mutex_lock+0x9c/0x430 > mutex_lock_nested+0x38/0x64 > regulator_lock_dependent+0x54/0x284 > regulator_enable+0x34/0x80 > phy_power_on+0x24/0x130 > __dwc2_lowlevel_hw_enable+0x100/0x130 > dwc2_lowlevel_hw_enable+0x18/0x40 > dwc2_hsotg_udc_start+0x6c/0x2f0 > gadget_bind_driver+0x124/0x1f4 > really_probe.part.0+0x9c/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xa8/0x1c0 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xa4 > device_add+0x3a0/0x890 > usb_add_gadget+0x170/0x200 > usb_add_gadget_udc+0x94/0xd4 > dwc2_driver_probe+0x580/0x78c > platform_probe+0x68/0xe0 > really_probe.part.0+0x9c/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xa8/0x1c0 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xa4 > device_add+0x3a0/0x890 > of_device_add+0x48/0x6c > of_platform_device_create_pdata+0x98/0x100 > of_platform_bus_create+0x17c/0x37c > of_platform_populate+0x58/0xec > dwc3_meson_g12a_probe+0x314/0x5d0 > platform_probe+0x68/0xe0 > really_probe.part.0+0x9c/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xa8/0x1c0 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xa4 > deferred_probe_work_func+0x88/0xc4 > process_one_work+0x288/0x6bc > worker_thread+0x74/0x450 > kthread+0x118/0x11c > ret_from_fork+0x10/0x20 > > -> #0 (udc_lock){+.+.}-{3:3}: > __lock_acquire+0x1298/0x20cc > lock_acquire.part.0+0xe0/0x230 > lock_acquire+0x68/0x84 > __mutex_lock+0x9c/0x430 > mutex_lock_nested+0x38/0x64 > usb_udc_uevent+0x54/0xe0 > dev_uevent+0xb8/0x1ec > uevent_show+0x8c/0x114 > dev_attr_show+0x20/0x60 > sysfs_kf_seq_show+0xa8/0x120 > kernfs_seq_show+0x2c/0x40 > seq_read_iter+0x1bc/0x4b0 > kernfs_fop_read_iter+0x140/0x1d0 > new_sync_read+0xd4/0x150 > vfs_read+0x190/0x1dc > ksys_read+0x68/0xfc > __arm64_sys_read+0x20/0x30 > invoke_syscall+0x48/0x114 > el0_svc_common.constprop.0+0x60/0x11c > do_el0_svc_compat+0x1c/0x50 > el0_svc_compat+0x58/0x100 > el0t_32_sync_handler+0x90/0x140 > el0t_32_sync+0x190/0x194 > > other info that might help us debug this: > > Chain exists of: > udc_lock --> device_links_lock --> kn->active#4 Peter, shouldn't this say: udc_lock --> regulator_list_mutex --> device_links_lock --> kn->active#4 ? Is that a bug in the lockdep reporting? It looks like this is a bad interaction between the udc_lock and the sysfs/kernfs locking. If a lock such as udc_lock is held (or is part of a chain) while a sysfs file is removed, the file's ->show or ->store routine better not acquire that lock. I suspect the problem is that udc_lock is held for too long. Probably it should be released during the calls to udc->driver->bind and udc->driver->unbind. Getting this right will require some careful study. Marek, if I send you a patch later, will you be able to test it? Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-08 20:26 ` Alan Stern @ 2022-08-09 6:29 ` Marek Szyprowski 2022-08-10 19:33 ` Alan Stern 1 sibling, 0 replies; 16+ messages in thread From: Marek Szyprowski @ 2022-08-09 6:29 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs Hi Alan, On 08.08.2022 22:26, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: >> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: >> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it >> fixes the issue by introducing another one. It doesn't look very >> probable, but it would be nice to fix it to make the lock dependency >> checker happy. > Indeed. > >> ====================================================== >> WARNING: possible circular locking dependency detected >> 5.19.0-rc7+ #12510 Not tainted >> ------------------------------------------------------ >> udevadm/312 is trying to acquire lock: >> ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0 >> >> but task is already holding lock: >> ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 >> >> which lock already depends on the new lock. >> >> >> the existing dependency chain (in reverse order) is: >> >> -> #3 (kn->active#4){++++}-{0:0}: >> lock_acquire+0x68/0x84 >> __kernfs_remove+0x268/0x380 >> kernfs_remove_by_name_ns+0x58/0xac >> sysfs_remove_file_ns+0x18/0x24 >> device_del+0x15c/0x440 >> device_link_drop_managed+0xa8/0xe0 >> device_links_driver_bound+0x1b8/0x230 >> driver_bound+0x68/0xc0 >> really_probe.part.0+0x1f8/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __driver_attach+0x104/0x1b0 >> bus_for_each_dev+0x70/0xd0 >> driver_attach+0x24/0x30 >> bus_add_driver+0x154/0x204 >> driver_register+0x78/0x130 >> __platform_driver_register+0x28/0x34 >> simple_pm_bus_driver_init+0x1c/0x28 >> do_one_initcall+0x74/0x400 >> kernel_init_freeable+0x2f4/0x37c >> kernel_init+0x28/0x130 >> ret_from_fork+0x10/0x20 >> >> -> #2 (device_links_lock){+.+.}-{3:3}: >> lock_acquire+0x68/0x84 >> __mutex_lock+0x9c/0x430 >> mutex_lock_nested+0x38/0x64 >> device_link_remove+0x3c/0xa0 >> _regulator_put.part.0+0x168/0x190 >> regulator_put+0x3c/0x54 >> devm_regulator_release+0x14/0x20 >> release_nodes+0x5c/0x90 >> devres_release_all+0x8c/0xe0 >> device_unbind_cleanup+0x18/0x70 >> really_probe.part.0+0x174/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach_async_helper+0xb0/0xd4 >> async_run_entry_fn+0x34/0xd0 >> process_one_work+0x288/0x6bc >> worker_thread+0x74/0x450 >> kthread+0x118/0x11c >> ret_from_fork+0x10/0x20 >> >> -> #1 (regulator_list_mutex){+.+.}-{3:3}: >> lock_acquire+0x68/0x84 >> __mutex_lock+0x9c/0x430 >> mutex_lock_nested+0x38/0x64 >> regulator_lock_dependent+0x54/0x284 >> regulator_enable+0x34/0x80 >> phy_power_on+0x24/0x130 >> __dwc2_lowlevel_hw_enable+0x100/0x130 >> dwc2_lowlevel_hw_enable+0x18/0x40 >> dwc2_hsotg_udc_start+0x6c/0x2f0 >> gadget_bind_driver+0x124/0x1f4 >> really_probe.part.0+0x9c/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach+0xa8/0x1c0 >> device_initial_probe+0x14/0x20 >> bus_probe_device+0x9c/0xa4 >> device_add+0x3a0/0x890 >> usb_add_gadget+0x170/0x200 >> usb_add_gadget_udc+0x94/0xd4 >> dwc2_driver_probe+0x580/0x78c >> platform_probe+0x68/0xe0 >> really_probe.part.0+0x9c/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach+0xa8/0x1c0 >> device_initial_probe+0x14/0x20 >> bus_probe_device+0x9c/0xa4 >> device_add+0x3a0/0x890 >> of_device_add+0x48/0x6c >> of_platform_device_create_pdata+0x98/0x100 >> of_platform_bus_create+0x17c/0x37c >> of_platform_populate+0x58/0xec >> dwc3_meson_g12a_probe+0x314/0x5d0 >> platform_probe+0x68/0xe0 >> really_probe.part.0+0x9c/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach+0xa8/0x1c0 >> device_initial_probe+0x14/0x20 >> bus_probe_device+0x9c/0xa4 >> deferred_probe_work_func+0x88/0xc4 >> process_one_work+0x288/0x6bc >> worker_thread+0x74/0x450 >> kthread+0x118/0x11c >> ret_from_fork+0x10/0x20 >> >> -> #0 (udc_lock){+.+.}-{3:3}: >> __lock_acquire+0x1298/0x20cc >> lock_acquire.part.0+0xe0/0x230 >> lock_acquire+0x68/0x84 >> __mutex_lock+0x9c/0x430 >> mutex_lock_nested+0x38/0x64 >> usb_udc_uevent+0x54/0xe0 >> dev_uevent+0xb8/0x1ec >> uevent_show+0x8c/0x114 >> dev_attr_show+0x20/0x60 >> sysfs_kf_seq_show+0xa8/0x120 >> kernfs_seq_show+0x2c/0x40 >> seq_read_iter+0x1bc/0x4b0 >> kernfs_fop_read_iter+0x140/0x1d0 >> new_sync_read+0xd4/0x150 >> vfs_read+0x190/0x1dc >> ksys_read+0x68/0xfc >> __arm64_sys_read+0x20/0x30 >> invoke_syscall+0x48/0x114 >> el0_svc_common.constprop.0+0x60/0x11c >> do_el0_svc_compat+0x1c/0x50 >> el0_svc_compat+0x58/0x100 >> el0t_32_sync_handler+0x90/0x140 >> el0t_32_sync+0x190/0x194 >> >> other info that might help us debug this: >> >> Chain exists of: >> udc_lock --> device_links_lock --> kn->active#4 > Peter, shouldn't this say: > > udc_lock --> regulator_list_mutex --> device_links_lock --> kn->active#4 > > ? Is that a bug in the lockdep reporting? > > It looks like this is a bad interaction between the udc_lock and the > sysfs/kernfs locking. If a lock such as udc_lock is held (or is part of > a chain) while a sysfs file is removed, the file's ->show or ->store > routine better not acquire that lock. > > I suspect the problem is that udc_lock is held for too long. Probably it > should be released during the calls to udc->driver->bind and > udc->driver->unbind. > > Getting this right will require some careful study. Marek, if I send you > a patch later, will you be able to test it? Yes, sure. I'm available till 12th August, then I'm going for holidays. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-08 20:26 ` Alan Stern 2022-08-09 6:29 ` Marek Szyprowski @ 2022-08-10 19:33 ` Alan Stern 2022-08-11 7:31 ` Marek Szyprowski 2022-09-01 19:22 ` Francesco Dolcini 1 sibling, 2 replies; 16+ messages in thread From: Alan Stern @ 2022-08-10 19:33 UTC (permalink / raw) To: Marek Szyprowski Cc: Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > > Hi Alan, > > Hi. > > > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > > fixes the issue by introducing another one. It doesn't look very > > probable, but it would be nice to fix it to make the lock dependency > > checker happy. > > Indeed. > I suspect the problem is that udc_lock is held for too long. Probably it > should be released during the calls to udc->driver->bind and > udc->driver->unbind. > > Getting this right will require some careful study. Marek, if I send you > a patch later, will you be able to test it? Here's a patch for you to try, when you have the chance. It reduces the scope of udc_lock to cover only the fields it's supposed to protect and changes the locking in a few other places. There's still the possibility of a locking cycle, because udc_lock is held in the ->disconnect pathway. It's very hard to know whether that might cause any trouble; it depends on how the function drivers handle disconnections. Alan Stern Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad ret = gadget->ops->pullup(gadget, 0); if (!ret) { gadget->connected = 0; - gadget->udc->driver->disconnect(gadget); + mutex_lock(&udc_lock); + if (gadget->udc->driver) + gadget->udc->driver->disconnect(gadget); + mutex_unlock(&udc_lock); } out: @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev usb_gadget_udc_set_speed(udc, driver->max_speed); - mutex_lock(&udc_lock); ret = driver->bind(udc->gadget, driver); if (ret) goto err_bind; @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev goto err_start; usb_gadget_enable_async_callbacks(udc); usb_udc_connect_control(udc); - mutex_unlock(&udc_lock); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev dev_err(&udc->dev, "failed to start %s: %d\n", driver->function, ret); + mutex_lock(&udc_lock); udc->driver = NULL; driver->is_bound = false; mutex_unlock(&udc_lock); @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - mutex_lock(&udc_lock); usb_gadget_disconnect(gadget); usb_gadget_disable_async_callbacks(udc); if (gadget->irq) @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct udc->driver->unbind(gadget); usb_gadget_udc_stop(udc); + mutex_lock(&udc_lock); driver->is_bound = false; udc->driver = NULL; mutex_unlock(&udc_lock); @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct struct usb_udc *udc = container_of(dev, struct usb_udc, dev); ssize_t ret; - mutex_lock(&udc_lock); + device_lock(&udc->gadget->dev); if (!udc->driver) { dev_err(dev, "soft-connect without a gadget driver\n"); ret = -EOPNOTSUPP; @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct ret = n; out: - mutex_unlock(&udc_lock); + device_unlock(&udc->gadget->dev); return ret; } static DEVICE_ATTR_WO(soft_connect); @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi char *buf) { struct usb_udc *udc = container_of(dev, struct usb_udc, dev); - struct usb_gadget_driver *drv = udc->driver; + struct usb_gadget_driver *drv; + int rc = 0; - if (!drv || !drv->function) - return 0; - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_lock(&udc_lock); + drv = udc->driver; + if (drv && drv->function) + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_unlock(&udc_lock); + return rc; } static DEVICE_ATTR_RO(function); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-10 19:33 ` Alan Stern @ 2022-08-11 7:31 ` Marek Szyprowski 2022-08-11 16:06 ` Alan Stern 2022-09-01 19:22 ` Francesco Dolcini 1 sibling, 1 reply; 16+ messages in thread From: Marek Szyprowski @ 2022-08-11 7:31 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs Hi Alan, On 10.08.2022 21:33, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: >> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: >>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: >>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it >>> fixes the issue by introducing another one. It doesn't look very >>> probable, but it would be nice to fix it to make the lock dependency >>> checker happy. >> Indeed. >> I suspect the problem is that udc_lock is held for too long. Probably it >> should be released during the calls to udc->driver->bind and >> udc->driver->unbind. >> >> Getting this right will require some careful study. Marek, if I send you >> a patch later, will you be able to test it? > Here's a patch for you to try, when you have the chance. It reduces the > scope of udc_lock to cover only the fields it's supposed to protect and > changes the locking in a few other places. > > There's still the possibility of a locking cycle, because udc_lock is > held in the ->disconnect pathway. It's very hard to know whether that > might cause any trouble; it depends on how the function drivers handle > disconnections. It looks this fixed the issue I've reported. I've checked it on all my test systems and none reported any issue related to the udc. Feel free to add: Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Alan Stern > > > > Index: usb-devel/drivers/usb/gadget/udc/core.c > =================================================================== > --- usb-devel.orig/drivers/usb/gadget/udc/core.c > +++ usb-devel/drivers/usb/gadget/udc/core.c > @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad > ret = gadget->ops->pullup(gadget, 0); > if (!ret) { > gadget->connected = 0; > - gadget->udc->driver->disconnect(gadget); > + mutex_lock(&udc_lock); > + if (gadget->udc->driver) > + gadget->udc->driver->disconnect(gadget); > + mutex_unlock(&udc_lock); > } > > out: > @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev > > usb_gadget_udc_set_speed(udc, driver->max_speed); > > - mutex_lock(&udc_lock); > ret = driver->bind(udc->gadget, driver); > if (ret) > goto err_bind; > @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev > goto err_start; > usb_gadget_enable_async_callbacks(udc); > usb_udc_connect_control(udc); > - mutex_unlock(&udc_lock); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev > dev_err(&udc->dev, "failed to start %s: %d\n", > driver->function, ret); > > + mutex_lock(&udc_lock); > udc->driver = NULL; > driver->is_bound = false; > mutex_unlock(&udc_lock); > @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > - mutex_lock(&udc_lock); > usb_gadget_disconnect(gadget); > usb_gadget_disable_async_callbacks(udc); > if (gadget->irq) > @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct > udc->driver->unbind(gadget); > usb_gadget_udc_stop(udc); > > + mutex_lock(&udc_lock); > driver->is_bound = false; > udc->driver = NULL; > mutex_unlock(&udc_lock); > @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > ssize_t ret; > > - mutex_lock(&udc_lock); > + device_lock(&udc->gadget->dev); > if (!udc->driver) { > dev_err(dev, "soft-connect without a gadget driver\n"); > ret = -EOPNOTSUPP; > @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct > > ret = n; > out: > - mutex_unlock(&udc_lock); > + device_unlock(&udc->gadget->dev); > return ret; > } > static DEVICE_ATTR_WO(soft_connect); > @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi > char *buf) > { > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > - struct usb_gadget_driver *drv = udc->driver; > + struct usb_gadget_driver *drv; > + int rc = 0; > > - if (!drv || !drv->function) > - return 0; > - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_lock(&udc_lock); > + drv = udc->driver; > + if (drv && drv->function) > + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_unlock(&udc_lock); > + return rc; > } > static DEVICE_ATTR_RO(function); > > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-11 7:31 ` Marek Szyprowski @ 2022-08-11 16:06 ` Alan Stern 2022-08-26 6:30 ` Marek Szyprowski 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2022-08-11 16:06 UTC (permalink / raw) To: Marek Szyprowski Cc: Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs On Thu, Aug 11, 2022 at 09:31:34AM +0200, Marek Szyprowski wrote: > Hi Alan, > > On 10.08.2022 21:33, Alan Stern wrote: > > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > >> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > >>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > >>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > >>> fixes the issue by introducing another one. It doesn't look very > >>> probable, but it would be nice to fix it to make the lock dependency > >>> checker happy. > >> Indeed. > >> I suspect the problem is that udc_lock is held for too long. Probably it > >> should be released during the calls to udc->driver->bind and > >> udc->driver->unbind. > >> > >> Getting this right will require some careful study. Marek, if I send you > >> a patch later, will you be able to test it? > > Here's a patch for you to try, when you have the chance. It reduces the > > scope of udc_lock to cover only the fields it's supposed to protect and > > changes the locking in a few other places. > > > > There's still the possibility of a locking cycle, because udc_lock is > > held in the ->disconnect pathway. It's very hard to know whether that > > might cause any trouble; it depends on how the function drivers handle > > disconnections. > > It looks this fixed the issue I've reported. I've checked it on all my > test systems and none reported any issue related to the udc. > > Feel free to add: > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Thanks for the quick testing. I'll submit the patch when the current merge window ends. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-11 16:06 ` Alan Stern @ 2022-08-26 6:30 ` Marek Szyprowski 2022-08-26 14:50 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Marek Szyprowski @ 2022-08-26 6:30 UTC (permalink / raw) To: Alan Stern Cc: Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs On 11.08.2022 18:06, Alan Stern wrote: > On Thu, Aug 11, 2022 at 09:31:34AM +0200, Marek Szyprowski wrote: >> On 10.08.2022 21:33, Alan Stern wrote: >>> On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: >>>> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: >>>>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: >>>>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it >>>>> fixes the issue by introducing another one. It doesn't look very >>>>> probable, but it would be nice to fix it to make the lock dependency >>>>> checker happy. >>>> Indeed. >>>> I suspect the problem is that udc_lock is held for too long. Probably it >>>> should be released during the calls to udc->driver->bind and >>>> udc->driver->unbind. >>>> >>>> Getting this right will require some careful study. Marek, if I send you >>>> a patch later, will you be able to test it? >>> Here's a patch for you to try, when you have the chance. It reduces the >>> scope of udc_lock to cover only the fields it's supposed to protect and >>> changes the locking in a few other places. >>> >>> There's still the possibility of a locking cycle, because udc_lock is >>> held in the ->disconnect pathway. It's very hard to know whether that >>> might cause any trouble; it depends on how the function drivers handle >>> disconnections. >> It looks this fixed the issue I've reported. I've checked it on all my >> test systems and none reported any issue related to the udc. >> >> Feel free to add: >> >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Thanks for the quick testing. I'll submit the patch when the current > merge window ends. Gentle ping for the final patch... Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-26 6:30 ` Marek Szyprowski @ 2022-08-26 14:50 ` Alan Stern 0 siblings, 0 replies; 16+ messages in thread From: Alan Stern @ 2022-08-26 14:50 UTC (permalink / raw) To: Marek Szyprowski Cc: Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs On Fri, Aug 26, 2022 at 08:30:50AM +0200, Marek Szyprowski wrote: > On 11.08.2022 18:06, Alan Stern wrote: > > On Thu, Aug 11, 2022 at 09:31:34AM +0200, Marek Szyprowski wrote: > >> On 10.08.2022 21:33, Alan Stern wrote: > >>> On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > >>>> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > >>>>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > >>>>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > >>>>> fixes the issue by introducing another one. It doesn't look very > >>>>> probable, but it would be nice to fix it to make the lock dependency > >>>>> checker happy. > >>>> Indeed. > >>>> I suspect the problem is that udc_lock is held for too long. Probably it > >>>> should be released during the calls to udc->driver->bind and > >>>> udc->driver->unbind. > >>>> > >>>> Getting this right will require some careful study. Marek, if I send you > >>>> a patch later, will you be able to test it? > >>> Here's a patch for you to try, when you have the chance. It reduces the > >>> scope of udc_lock to cover only the fields it's supposed to protect and > >>> changes the locking in a few other places. > >>> > >>> There's still the possibility of a locking cycle, because udc_lock is > >>> held in the ->disconnect pathway. It's very hard to know whether that > >>> might cause any trouble; it depends on how the function drivers handle > >>> disconnections. > >> It looks this fixed the issue I've reported. I've checked it on all my > >> test systems and none reported any issue related to the udc. > >> > >> Feel free to add: > >> > >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> > >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Thanks for the quick testing. I'll submit the patch when the current > > merge window ends. > > Gentle ping for the final patch... On its way. It turns out that the hardest part of this patch is writing up the descriptions and justifications in the Changelog. :-( (Part of the reason for the delay is that I usually wait until the -rc2 or -rc3 release before starting to work on a new kernel version...) I think in the end there is more work still to do. In particular, the gadget core doesn't seem to be careful about not connecting (i.e., not turning on the D+ pullup) when no driver is bound to the gadget. Those actions need to be synchronized somehow. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-08-10 19:33 ` Alan Stern 2022-08-11 7:31 ` Marek Szyprowski @ 2022-09-01 19:22 ` Francesco Dolcini 2022-09-01 19:29 ` Alan Stern 1 sibling, 1 reply; 16+ messages in thread From: Francesco Dolcini @ 2022-09-01 19:22 UTC (permalink / raw) To: Alan Stern Cc: Marek Szyprowski, Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs On Wed, Aug 10, 2022 at 03:33:00PM -0400, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > > > Hi Alan, > > > > Hi. > > > > > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > > > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > > > fixes the issue by introducing another one. It doesn't look very > > > probable, but it would be nice to fix it to make the lock dependency > > > checker happy. Hi just bisected a different warning [1], triggered by the same patch and I was about to send a revert while I noticed you already have a fix more or less ready, it would be great if you could send it. Please find my tested-by afterward. [1] https://lore.kernel.org/all/20220901122129.GA493609@francesco-nb.int.toradex.com/ [ 25.942837] ====================================================== [ 25.942845] WARNING: possible circular locking dependency detected [ 25.942854] 6.0.0-rc3-0.0.0-devel+git.b90cb1053190 #1 Not tainted [ 25.942866] ------------------------------------------------------ [ 25.942873] connmand/667 is trying to acquire lock: [ 25.942887] c2b03348 (kn->active#9){++++}-{0:0}, at: kernfs_remove_by_name_ns+0x50/0xa0 [ 25.942948] but task is already holding lock: [ 25.942956] c17af6a0 (rtnl_mutex){+.+.}-{3:3}, at: devinet_ioctl+0xb8/0x8e0 [ 25.942996] which lock already depends on the new lock. [ 25.943003] the existing dependency chain (in reverse order) is: [ 25.943010] -> #2 (rtnl_mutex){+.+.}-{3:3}: [ 25.943039] mutex_lock_killable_nested+0x1c/0x28 [ 25.943058] register_netdev+0xc/0x34 [ 25.943072] gether_register_netdev+0x38/0xb0 [ 25.943087] rndis_bind+0x230/0x3a0 [ 25.943102] usb_add_function+0x7c/0x1d4 [ 25.943119] configfs_composite_bind+0x1c0/0x360 [ 25.943133] gadget_bind_driver+0x9c/0x208 [ 25.943148] really_probe+0xd8/0x410 [ 25.943165] __driver_probe_device+0x94/0x204 [ 25.943180] driver_probe_device+0x2c/0xc4 [ 25.943195] __driver_attach+0xe0/0x1f0 [ 25.943209] bus_for_each_dev+0x70/0xb0 [ 25.943224] bus_add_driver+0x164/0x218 [ 25.943238] driver_register+0x88/0x11c [ 25.943253] usb_gadget_register_driver_owner+0x40/0xd4 [ 25.943267] gadget_dev_desc_UDC_store+0xd4/0x110 [ 25.943281] configfs_write_iter+0xac/0x118 [ 25.943295] vfs_write+0x2c4/0x46c [ 25.943312] ksys_write+0x5c/0xd4 [ 25.943326] ret_fast_syscall+0x0/0x1c [ 25.943340] 0xbe9deb88 [ 25.943352] -> #1 (udc_lock){+.+.}-{3:3}: [ 25.943381] mutex_lock_nested+0x1c/0x24 [ 25.943396] usb_udc_uevent+0x34/0xb0 [ 25.943409] dev_uevent+0xfc/0x2cc [ 25.943423] uevent_show+0x90/0x10c [ 25.943436] dev_attr_show+0x18/0x48 [ 25.943453] sysfs_kf_seq_show+0x88/0x118 [ 25.943465] seq_read_iter+0x1a4/0x4d4 [ 25.943479] vfs_read+0x1bc/0x288 [ 25.943493] ksys_read+0x5c/0xd4 [ 25.943508] ret_fast_syscall+0x0/0x1c [ 25.943520] 0xbeb5d840 [ 25.943530] -> #0 (kn->active#9){++++}-{0:0}: [ 25.943564] lock_acquire+0xf4/0x364 [ 25.943581] __kernfs_remove+0x330/0x410 [ 25.943595] kernfs_remove_by_name_ns+0x50/0xa0 [ 25.943611] device_del+0x158/0x3dc [ 25.943623] device_unregister+0x20/0x64 [ 25.943636] wakeup_source_unregister.part.0+0x20/0x54 [ 25.943655] device_set_wakeup_enable+0x54/0x64 [ 25.943668] fec_enet_open+0x2ec/0x394 [ 25.943684] __dev_open+0xe0/0x1a0 [ 25.943696] __dev_change_flags+0x180/0x214 [ 25.943708] dev_change_flags+0x14/0x44 [ 25.943720] devinet_ioctl+0x878/0x8e0 [ 25.943733] inet_ioctl+0x180/0x238 [ 25.943746] sock_ioctl+0x4a0/0x5b4 [ 25.943760] sys_ioctl+0x530/0xdbc [ 25.943775] ret_fast_syscall+0x0/0x1c [ 25.943787] 0xbe91d960 [ 25.943797] other info that might help us debug this: [ 25.943804] Chain exists of: kn->active#9 --> udc_lock --> rtnl_mutex [ 25.943844] Possible unsafe locking scenario: [ 25.943851] CPU0 CPU1 [ 25.943857] ---- ---- [ 25.943863] lock(rtnl_mutex); [ 25.943879] lock(udc_lock); [ 25.943894] lock(rtnl_mutex); [ 25.943910] lock(kn->active#9); [ 25.943930] *** DEADLOCK *** [ 25.943937] 1 lock held by connmand/667: [ 25.943947] #0: c17af6a0 (rtnl_mutex){+.+.}-{3:3}, at: devinet_ioctl+0xb8/0x8e0 [ 25.943989] stack backtrace: [ 25.943997] CPU: 1 PID: 667 Comm: connmand Not tainted 6.0.0-rc3-0.0.0-devel+git.b90cb1053190 #1 [ 25.944012] Hardware name: Freescale i.MX7 Dual (Device Tree) [ 25.944025] unwind_backtrace from show_stack+0x10/0x14 [ 25.944049] show_stack from dump_stack_lvl+0x58/0x70 [ 25.944071] dump_stack_lvl from check_noncircular+0xe8/0x158 [ 25.944094] check_noncircular from __lock_acquire+0x1510/0x288c [ 25.944115] __lock_acquire from lock_acquire+0xf4/0x364 [ 25.944135] lock_acquire from __kernfs_remove+0x330/0x410 [ 25.944157] __kernfs_remove from kernfs_remove_by_name_ns+0x50/0xa0 [ 25.944179] kernfs_remove_by_name_ns from device_del+0x158/0x3dc [ 25.944199] device_del from device_unregister+0x20/0x64 [ 25.944216] device_unregister from wakeup_source_unregister.part.0+0x20/0x54 [ 25.944238] wakeup_source_unregister.part.0 from device_set_wakeup_enable+0x54/0x64 [ 25.944259] device_set_wakeup_enable from fec_enet_open+0x2ec/0x394 [ 25.944279] fec_enet_open from __dev_open+0xe0/0x1a0 [ 25.944298] __dev_open from __dev_change_flags+0x180/0x214 [ 25.944314] __dev_change_flags from dev_change_flags+0x14/0x44 [ 25.944331] dev_change_flags from devinet_ioctl+0x878/0x8e0 [ 25.944348] devinet_ioctl from inet_ioctl+0x180/0x238 [ 25.944365] inet_ioctl from sock_ioctl+0x4a0/0x5b4 [ 25.944383] sock_ioctl from sys_ioctl+0x530/0xdbc [ 25.944401] sys_ioctl from ret_fast_syscall+0x0/0x1c [ 25.944419] Exception stack(0xe14e9fa8 to 0xe14e9ff0) [ 25.944435] 9fa0: 00000000 be91d984 00000010 00008914 be91d984 be91d978 [ 25.944450] 9fc0: 00000000 be91d984 00000010 00000036 00000003 00001002 00000b20 be91db3c [ 25.944462] 9fe0: 00000036 be91d960 b6ba8189 b6b21ae6 > > > I suspect the problem is that udc_lock is held for too long. Probably it > > should be released during the calls to udc->driver->bind and > > udc->driver->unbind. > > > > Getting this right will require some careful study. Marek, if I send you > > a patch later, will you be able to test it? > > Here's a patch for you to try, when you have the chance. It reduces the > scope of udc_lock to cover only the fields it's supposed to protect and > changes the locking in a few other places. > > Index: usb-devel/drivers/usb/gadget/udc/core.c > =================================================================== > --- usb-devel.orig/drivers/usb/gadget/udc/core.c > +++ usb-devel/drivers/usb/gadget/udc/core.c > @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad > ret = gadget->ops->pullup(gadget, 0); > if (!ret) { > gadget->connected = 0; > - gadget->udc->driver->disconnect(gadget); > + mutex_lock(&udc_lock); > + if (gadget->udc->driver) > + gadget->udc->driver->disconnect(gadget); > + mutex_unlock(&udc_lock); > } > > out: > @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev > > usb_gadget_udc_set_speed(udc, driver->max_speed); > > - mutex_lock(&udc_lock); > ret = driver->bind(udc->gadget, driver); > if (ret) > goto err_bind; > @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev > goto err_start; > usb_gadget_enable_async_callbacks(udc); > usb_udc_connect_control(udc); > - mutex_unlock(&udc_lock); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev > dev_err(&udc->dev, "failed to start %s: %d\n", > driver->function, ret); > > + mutex_lock(&udc_lock); > udc->driver = NULL; > driver->is_bound = false; > mutex_unlock(&udc_lock); > @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > - mutex_lock(&udc_lock); > usb_gadget_disconnect(gadget); > usb_gadget_disable_async_callbacks(udc); > if (gadget->irq) > @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct > udc->driver->unbind(gadget); > usb_gadget_udc_stop(udc); > > + mutex_lock(&udc_lock); > driver->is_bound = false; > udc->driver = NULL; > mutex_unlock(&udc_lock); > @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > ssize_t ret; > > - mutex_lock(&udc_lock); > + device_lock(&udc->gadget->dev); > if (!udc->driver) { > dev_err(dev, "soft-connect without a gadget driver\n"); > ret = -EOPNOTSUPP; > @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct > > ret = n; > out: > - mutex_unlock(&udc_lock); > + device_unlock(&udc->gadget->dev); > return ret; > } > static DEVICE_ATTR_WO(soft_connect); > @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi > char *buf) > { > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > - struct usb_gadget_driver *drv = udc->driver; > + struct usb_gadget_driver *drv; > + int rc = 0; > > - if (!drv || !drv->function) > - return 0; > - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_lock(&udc_lock); > + drv = udc->driver; > + if (drv && drv->function) > + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_unlock(&udc_lock); > + return rc; > } > static DEVICE_ATTR_RO(function); > Tested-by: Francesco Dolcini <francesco.dolcini@toradex.com> Francesco ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() 2022-09-01 19:22 ` Francesco Dolcini @ 2022-09-01 19:29 ` Alan Stern 0 siblings, 0 replies; 16+ messages in thread From: Alan Stern @ 2022-09-01 19:29 UTC (permalink / raw) To: Francesco Dolcini Cc: Marek Szyprowski, Greg KH, Felipe Balbi, Peter Zijlstra, USB mailing list, syzkaller-bugs On Thu, Sep 01, 2022 at 09:22:04PM +0200, Francesco Dolcini wrote: > On Wed, Aug 10, 2022 at 03:33:00PM -0400, Alan Stern wrote: > > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > > > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > > > > Hi Alan, > > > > > > Hi. > > > > > > > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > > > > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > > > > fixes the issue by introducing another one. It doesn't look very > > > > probable, but it would be nice to fix it to make the lock dependency > > > > checker happy. > > Hi just bisected a different warning [1], triggered by the same patch and I > was about to send a revert while I noticed you already have a fix more > or less ready, it would be great if you could send it. Please find my > tested-by afterward. It has already been submitted: https://lore.kernel.org/linux-usb/YwkfhdxA%2FI2nOcK7@rowland.harvard.edu/ Greg hasn't merged it yet, though. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-09-01 19:29 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-02 13:21 KASAN: use-after-free Read in usb_udc_uevent syzbot
2022-07-20 18:03 ` [syzbot] " syzbot
2022-07-21 13:58 ` Alan Stern
2022-07-21 14:26 ` syzbot
2022-07-21 15:07 ` [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent() Alan Stern
2022-07-27 12:17 ` Greg KH
[not found] ` <CGME20220808145736eucas1p234e56422bd7973d7f0676e74f03ba405@eucas1p2.samsung.com>
2022-08-08 14:57 ` Marek Szyprowski
2022-08-08 20:26 ` Alan Stern
2022-08-09 6:29 ` Marek Szyprowski
2022-08-10 19:33 ` Alan Stern
2022-08-11 7:31 ` Marek Szyprowski
2022-08-11 16:06 ` Alan Stern
2022-08-26 6:30 ` Marek Szyprowski
2022-08-26 14:50 ` Alan Stern
2022-09-01 19:22 ` Francesco Dolcini
2022-09-01 19:29 ` Alan Stern
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.