Re: [syzbot] KASAN: use-after-free Write in put_ucounts

Re: [syzbot] KASAN: use-after-free Write in put_ucounts

[Hillf Danton]

On Sat, 24 Jul 2021 19:57:24 +0200 Alexey Gladkov wrote:
>On Mon, Jul 19, 2021 at 12:24:41PM -0500, Eric W. Biederman wrote:
>> Hillf Danton <hdanton@sina.com> writes:
>> 
>> > On Fri, 16 Jul 2021 23:22:19 -0700
>> >>syzbot found the following issue on:
>> >>
>> >>HEAD commit:    3dbdb38e2869 Merge branch 'for-5.14' of git://git.kernel.o..
>> >>git tree:       upstream
>> >>console output: https://syzkaller.appspot.com/x/log.txt?x=1541de9c300000
>> >>kernel config:  https://syzkaller.appspot.com/x/.config?x=a1fcf15a09815757
>> >>dashboard link: https://syzkaller.appspot.com/bug?extid=b6e65bd125a05f803d6b
>> >>userspace arch: i386
>> >>
>> >>Unfortunately, I don't have any reproducer for this issue yet.
>> >>
>> >>IMPORTANT: if you fix the issue, please add the following tag to the commit:
>> >>Reported-by: syzbot+b6e65bd125a05f803d6b@syzkaller.appspotmail.com
>> >>
>> >>==================================================================
>> >>BUG: KASAN: use-after-free in instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
>> >>BUG: KASAN: use-after-free in atomic_dec_and_test include/asm-generic/atomic-instrumented.h:542 [inline]
>> >>BUG: KASAN: use-after-free in put_ucounts+0x1c/0x150 kernel/ucount.c:196
>> >>Write of size 4 at addr ffff88801c86cc1c by task kworker/u4:3/166
>> >>
>> >>CPU: 0 PID: 166 Comm: kworker/u4:3 Not tainted 5.13.0-syzkaller #0
>> >>Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> >>Workqueue: bat_events batadv_nc_worker
>> >>Call Trace:
>> >> <IRQ>
>> >> __dump_stack lib/dump_stack.c:79 [inline]
>> >> dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:96
>> >> print_address_description.constprop.0.cold+0x6c/0x309 mm/kasan/report.c:233
>> >> __kasan_report mm/kasan/report.c:419 [inline]
>> >> kasan_report.cold+0x83/0xdf mm/kasan/report.c:436
>> >> check_region_inline mm/kasan/generic.c:183 [inline]
>> >> kasan_check_range+0x13d/0x180 mm/kasan/generic.c:189
>> >> instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
>> >> atomic_dec_and_test include/asm-generic/atomic-instrumented.h:542 [inline]
>> >> put_ucounts+0x1c/0x150 kernel/ucount.c:196
>> >> put_cred_rcu+0x27a/0x520 kernel/cred.c:124
>> >> rcu_do_batch kernel/rcu/tree.c:2558 [inline]
>> >> rcu_core+0x7ab/0x1380 kernel/rcu/tree.c:2793
>> >> __do_softirq+0x29b/0x9bd kernel/softirq.c:558
>> >> invoke_softirq kernel/softirq.c:432 [inline]
>> >> __irq_exit_rcu+0x16e/0x1c0 kernel/softirq.c:636
>> >> irq_exit_rcu+0x5/0x20 kernel/softirq.c:648
>> >> sysvec_apic_timer_interrupt+0x93/0xc0 arch/x86/kernel/apic/apic.c:1100
>> >> </IRQ>
>> >> asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:638
>> >>RIP: 0010:lock_acquire+0x1ef/0x510 kernel/locking/lockdep.c:5593
>> >>Code: e1 a6 7e 83 f8 01 0f 85 a6 02 00 00 9c 58 f6 c4 02 0f 85 91 02 00 00 48 83 7c 24 08 00 74 01 fb 48 b8 00 00 00 00 00 fc ff df <48> 01 c3 48 c7 03 00 00 00 00 48 c7 43 08 00 00 00 00 48 8b 84 24
>> >>RSP: 0018:ffffc900010bfba8 EFLAGS: 00000206
>> >>RAX: dffffc0000000000 RBX: 1ffff92000217f77 RCX: 3f6e7590f6a9846c
>> >>RDX: 1ffff1100283613d RSI: 0000000000000000 RDI: 0000000000000000
>> >>RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff9049d8a7
>> >>R10: fffffbfff2093b14 R11: 0000000000000000 R12: 0000000000000002
>> >>R13: 0000000000000000 R14: ffffffff8c17bb80 R15: 0000000000000000
>> >> rcu_lock_acquire include/linux/rcupdate.h:267 [inline]
>> >> rcu_read_lock include/linux/rcupdate.h:671 [inline]
>> >> batadv_nc_purge_orig_hash net/batman-adv/network-coding.c:404 [inline]
>> >> batadv_nc_worker+0x12d/0xe50 net/batman-adv/network-coding.c:715
>> >> process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>> >> worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>> >> kthread+0x3e5/0x4d0 kernel/kthread.c:319
>> >> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>> >>
>> >>Allocated by task 8622:
>> >> kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
>> >> kasan_set_track mm/kasan/common.c:46 [inline]
>> >> set_alloc_info mm/kasan/common.c:434 [inline]
>> >> ____kasan_kmalloc mm/kasan/common.c:513 [inline]
>> >> ____kasan_kmalloc mm/kasan/common.c:472 [inline]
>> >> __kasan_kmalloc+0x9b/0xd0 mm/kasan/common.c:522
>> >> kmalloc include/linux/slab.h:591 [inline]
>> >> kzalloc include/linux/slab.h:721 [inline]
>> >> alloc_ucounts+0x23d/0x5b0 kernel/ucount.c:169
>> >> set_cred_ucounts+0x171/0x3a0 kernel/cred.c:684
>> >> copy_creds+0x853/0xb20 kernel/cred.c:375
>> >> copy_process+0x1413/0x74c0 kernel/fork.c:1992
>> >> kernel_clone+0xe7/0xab0 kernel/fork.c:2509
>> >> __do_compat_sys_ia32_clone+0xac/0xe0 arch/x86/kernel/sys_ia32.c:254
>> >> do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
>> >> do_int80_syscall_32+0x46/0x90 arch/x86/entry/common.c:132
>> >> entry_INT80_compat+0x71/0x76 arch/x86/entry/entry_64_compat.S:413
>> >>
>> >>Last potentially related work creation:
>> >> kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
>> >> kasan_record_aux_stack+0xe5/0x110 mm/kasan/generic.c:348
>> >> insert_work+0x48/0x370 kernel/workqueue.c:1332
>> >> __queue_work+0x5c1/0xed0 kernel/workqueue.c:1498
>> >> queue_work_on+0xee/0x110 kernel/workqueue.c:1525
>> >> queue_work include/linux/workqueue.h:507 [inline]
>> >> call_usermodehelper_exec+0x1f0/0x4c0 kernel/umh.c:435
>> >> kobject_uevent_env+0xf8f/0x1650 lib/kobject_uevent.c:618
>> >> kobject_synth_uevent+0x701/0x850 lib/kobject_uevent.c:208
>> >> uevent_store+0x20/0x50 drivers/base/core.c:2370
>> >> dev_attr_store+0x50/0x80 drivers/base/core.c:2071
>> >> sysfs_kf_write+0x110/0x160 fs/sysfs/file.c:139
>> >> kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
>> >> call_write_iter include/linux/fs.h:2114 [inline]
>> >> new_sync_write+0x426/0x650 fs/read_write.c:518
>> >> vfs_write+0x796/0xa30 fs/read_write.c:605
>> >> ksys_write+0x12d/0x250 fs/read_write.c:658
>> >> 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+0x44/0xae
>> >>
>> >>Second to last potentially related work creation:
>> >> kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
>> >> kasan_record_aux_stack+0xe5/0x110 mm/kasan/generic.c:348
>> >> insert_work+0x48/0x370 kernel/workqueue.c:1332
>> >> __queue_work+0x5c1/0xed0 kernel/workqueue.c:1498
>> >> queue_work_on+0xee/0x110 kernel/workqueue.c:1525
>> >> queue_work include/linux/workqueue.h:507 [inline]
>> >> call_usermodehelper_exec+0x1f0/0x4c0 kernel/umh.c:435
>> >> kobject_uevent_env+0xf8f/0x1650 lib/kobject_uevent.c:618
>> >> kobject_synth_uevent+0x701/0x850 lib/kobject_uevent.c:208
>> >> uevent_store+0x42/0x90 drivers/base/bus.c:585
>> >> drv_attr_store+0x6d/0xa0 drivers/base/bus.c:77
>> >> sysfs_kf_write+0x110/0x160 fs/sysfs/file.c:139
>> >> kernfs_fop_write_iter+0x342/0x500 fs/kernfs/file.c:296
>> >> call_write_iter include/linux/fs.h:2114 [inline]
>> >> new_sync_write+0x426/0x650 fs/read_write.c:518
>> >> vfs_write+0x796/0xa30 fs/read_write.c:605
>> >> ksys_write+0x12d/0x250 fs/read_write.c:658
>> >> 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+0x44/0xae
>> >>
>> >>The buggy address belongs to the object at ffff88801c86cc00
>> >> which belongs to the cache kmalloc-192 of size 192
>> >>The buggy address is located 28 bytes inside of
>> >> 192-byte region [ffff88801c86cc00, ffff88801c86ccc0)
>> >>The buggy address belongs to the page:
>> >>page:ffffea0000721b00 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88801c86cc00 pfn:0x1c86c
>> >>flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
>> >>raw: 00fff00000000200 ffffea00009f0e48 ffffea00009cd188 ffff888011041a00
>> >>raw: ffff88801c86cc00 000000000010000b 00000001ffffffff 0000000000000000
>> >>page dumped because: kasan: bad access detected
>> >>page_owner tracks the page as allocated
>> >>page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY), pid 1, ts 7975119940, free_ts 7474437105
>> >> prep_new_page mm/page_alloc.c:2445 [inline]
>> >> get_page_from_freelist+0xa72/0x2f80 mm/page_alloc.c:4178
>> >> __alloc_pages+0x1b2/0x500 mm/page_alloc.c:5386
>> >> alloc_page_interleave+0x1e/0x200 mm/mempolicy.c:2147
>> >> alloc_pages+0x238/0x2a0 mm/mempolicy.c:2270
>> >> alloc_slab_page mm/slub.c:1702 [inline]
>> >> allocate_slab+0x32b/0x4c0 mm/slub.c:1842
>> >> new_slab mm/slub.c:1905 [inline]
>> >> new_slab_objects mm/slub.c:2651 [inline]
>> >> ___slab_alloc+0x4ba/0x820 mm/slub.c:2814
>> >> __slab_alloc.constprop.0+0xa7/0xf0 mm/slub.c:2854
>> >> slab_alloc_node mm/slub.c:2936 [inline]
>> >> slab_alloc mm/slub.c:2978 [inline]
>> >> kmem_cache_alloc_trace+0x325/0x3c0 mm/slub.c:2995
>> >> kmalloc include/linux/slab.h:591 [inline]
>> >> kzalloc include/linux/slab.h:721 [inline]
>> >> call_usermodehelper_setup+0x97/0x340 kernel/umh.c:365
>> >> kobject_uevent_env+0xf73/0x1650 lib/kobject_uevent.c:614
>> >> device_add+0xb71/0x2100 drivers/base/core.c:3305
>> >> register_virtio_device+0x1e1/0x2c0 drivers/virtio/virtio.c:363
>> >> virtio_pci_probe+0x203/0x390 drivers/virtio/virtio_pci_common.c:552
>> >> local_pci_probe+0xdb/0x190 drivers/pci/pci-driver.c:309
>> >> pci_call_probe drivers/pci/pci-driver.c:366 [inline]
>> >> __pci_device_probe drivers/pci/pci-driver.c:391 [inline]
>> >> pci_device_probe+0x3dd/0x6f0 drivers/pci/pci-driver.c:434
>> >> really_probe+0x291/0xf60 drivers/base/dd.c:576
>> >>page last free stack trace:
>> >> reset_page_owner include/linux/page_owner.h:24 [inline]
>> >> free_pages_prepare mm/page_alloc.c:1355 [inline]
>> >> free_pcp_prepare+0x2c5/0x780 mm/page_alloc.c:1406
>> >> free_unref_page_prepare mm/page_alloc.c:3341 [inline]
>> >> free_unref_page+0x19/0x690 mm/page_alloc.c:3420
>> >> __vunmap+0x783/0xb70 mm/vmalloc.c:2569
>> >> free_work+0x58/0x70 mm/vmalloc.c:80
>> >> process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>> >> worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>> >> kthread+0x3e5/0x4d0 kernel/kthread.c:319
>> >> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>> >>
>> >>Memory state around the buggy address:
>> >> ffff88801c86cb00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> >> ffff88801c86cb80: 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc fc
>> >>>ffff88801c86cc00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> >>                            ^
>> >> ffff88801c86cc80: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>> >> ffff88801c86cd00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> >>==================================================================
>> >
>> > To fix the uaf, add a couple of changes. Now only for thoughts.
>> 
>> >
>> > 1/ s/atomic_add_negative/atomic_inc_not_zero/ to correct the get
>> > method.
>> 
>> I really don't think so.  The use of atomic_add_negative is very
>> deliberate.  Changing that fundamentally changes the algorithm into used
>> to keep track of things.  Definitely not something to lead with.
>> 
>> Before it even makes sense to talk about how to change the code,
>> a plausible explanation for how a use after free happens is needed.
>> 
>> That explanation should account for the fact this code was in linux-next
>> the since last -rc1 without any kind of issue with the test code.
>> 
>> Did something change in the test infrastructure?  Is there another
>> bug that is stomping memory and making it look like this code is buggy?
>> 
>> I haven't had a chance to dig in in detail (everyone in my house is
>> sick).  I am hoping that Alexey Gladkov will have that time soon.
>
>It's better to look at another report. I think it illustrates
>regression better:
>
>https://syzkaller.appspot.com/bug?id=edc996c68de05701cbd709b1ee99344defb94334
>
>Hillf Danton is right about race condition. The situation is rare because
>most often alloc_ucounts() and put_ucounts() are executed under rcu lock.
>I spent a few days but couldn't reproduce it.
>
>I did this regression in the b6c336528926 ("Use atomic_t for ucounts
>reference counting"). Before to this, the reference count check was under
>spin_lock. Now spin_lock in alloc_ucounts() doesn't protect find_ucounts()
>from parallel put_ucounts().
>
>Atomically increasing the reference count solves the problem between
>get_accounts() and put_accounts(), but find_ucounts() is not protected.
>
>We need to either return spin_lock to the beginning of put_ucounts(), or
>recycle alloc_ucounts().
>
>> What I can say is that a solution that leads with your code is
>> fundamentally misdesigned and everything needs to change, and there
>> is no explanation for why it all needs to change is probably not a
>> solution to the problem.
>
>The proposed patch is incorrect because in case of an overflow of the
>reference count in find_get_ucounts(), we will get a counter leak.

Curious how you will handle the leak without change in the type of count
as I failed to work it out within ten minutes without putting the
atomic_dec_and_test() in put_ucounts() under ucounts_lock protection.

Hillf