* 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ messages in thread
* 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ messages in thread
* Re: [syzbot] KASAN: use-after-free Read in usb_udc_uevent
[not found] <20220721103453.1845-1-hdanton@sina.com>
@ 2022-07-21 10:52 ` syzbot
0 siblings, 0 replies; 17+ messages in thread
From: syzbot @ 2022-07-21 10:52 UTC (permalink / raw)
To: hdanton, linux-kernel, syzkaller-bugs
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=172e6f06080000
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=11860454080000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-09-01 19:29 UTC | newest]
Thread overview: 17+ 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
[not found] <20220721103453.1845-1-hdanton@sina.com>
2022-07-21 10:52 ` [syzbot] KASAN: use-after-free Read in usb_udc_uevent syzbot
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.