Re: KASAN: use-after-free Read in handle_userfault (2)

[Andrea Arcangeli]

Hello,

On Sun, Dec 30, 2018 at 08:48:05AM +0100, Dmitry Vyukov wrote:
> On Wed, Dec 12, 2018 at 10:58 AM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
> >
> > On Wed, Dec 12, 2018 at 10:45 AM syzbot
> > <syzbot+cbc64b24b2b2d54c07a9@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > Hello,
> > >
> > > syzbot found the following crash on:
> > >
> > > HEAD commit:    14cf8c1d5b90 Add linux-next specific files for 20181210
> > > git tree:       linux-next
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=133296db400000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=c9133d0a4284c012
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=cbc64b24b2b2d54c07a9
> > > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> >
> > This is a corrupted/intermixed kernel output, the bug actually
> > happened somewhere in net/core/neighbour.c.
> > syzkaller has a bunch of code to try to deal to corrupted kernel
> > output, but it's not always possible as a parallel thread printing
> > something can inject an unrelated frame just in the right place, and
> > then it's indistinguishable from a crash happened there.
> >
> > The question is: what's the significance of that
> > "FAULT_FLAG_ALLOW_RETRY missing 70"?
> > Is it something to fix in kernel? Can the check be removed? Should it

This is a rate limited warning not compiled in production kernels.

#ifdef CONFIG_DEBUG_VM
		if (printk_ratelimit()) {
			printk(KERN_WARNING
			       "FAULT_FLAG_ALLOW_RETRY missing %x\n",
			       vmf->flags);
			dump_stack();
		}
#endif

It helps to tell if it's an userland bug or a kernel missing feature
like some get_user_pages that can't drop mmap_sem in a path where we
may need userfaultfd to be functional.

Those get_user_pages locations are deterministic, so for example when
qemu started explicitly calling get_mempolicy(MPOL_F_NODE|MPOL_F_ADDR)
we had to change the get_user_pages there to make uffd functional for
such syscall. An example of where this debug aid paid off relatively
recently is commit 3b9aadf7278d16d7bed4d5d808501065f70898d8.

> > be moved somewhere earlier when flags are passed to kernel and cause
> > EINVAL? Can it be at least converted to a single-line pr_warn?

I don't see the problem with the VM_FAULT_SIGBUS retval.

The reason of the full dump_stack is because pr_warn would tell us
nothing about the location of the get_user_pages that needs
conversion. It's pretty much like when you get an -EINVAL in userland
and there's a zillon places that returns -EINVAL and you've no clue
which one you hit. The debug_stack is about pinpointing the exact
location after the fact, to evaluate what to do about it.

> > +Sasha go as far as suggesting that any "Call Trace:" in kernel output
> > means a kernel bugs. This is one of few places in kernel that dumps
> > stacks left and right without a kernel bug (?).

Yes this is a case where the Call Trace in the logs doesn't guarantee
there was a kernel bug, this is why it's rate limited and only
compiled in non production builds.

I haven't looked at the reproducer source, but looking only at the
trace I can tell this happened because a region has been registered in
uffd while a page fault was already undergoing a VM_FAULT_RETRY
cycle.

We could remove the dump_stack if FAULT_FLAG_TRIED is set, but
removing it for all cases would make it harder to solve other kernel
issues like described above.

However with Peter Xu recent work FAULT_FLAG_TRIED will go away as
part of the userfaultfd WP support (FAULT_FLAG_TRIED is the reason of
this specific false positive) so you won't be able to trigger this
path through undefined state userland race conditions anymore after
that.

So I'd propose Peter's FAULT_FLAG_TRIED removal as the best path
forward because it's will solve 3 problems: 1) this rate limited false
positive FAULT_FLAG_TRIED dump_stack with malicious app, 2) signal
case not returning VM_FAULT_RETRY despite we dropped mmap_sem to sleep
until we got waken up by a signal, 3) it'll allow the uffd write
protection to work.

https://lkml.kernel.org/r/20181121062103.18835-1-peterx@xxxxxxxxxx

Until FAULT_FLAG_TRIED is dropped, it's up to the manager in the non
cooperative case and up to the app itself in the normal more common
cooperative case, to ensure there aren't page faults already happening
in the region while the region is registered in uffd. It's totally
trivial for the app to do so until we relax this requirement in a
fully backwards compatible way.

The reason FAULT_FLAG_TRIED must be dropped for the write-protection
feature is that the uffd per-page granular write protection ioctl must
be allowed to run at any time on any region, potentially while there
are already VM_FAULT_RETRY faults being retried. So the kernel common
code must be allowed to return VM_FAULT_RETRY twice or more in a row
from the point of view of the arch page fault code.

To remove the dump_stack completely (not only for the case of a
malicious userland app) we could add a debug facility in debugfs where
we could dump stack traces in a circular buffer, so we could ask the
root user to cat the file and send us the extra call trace debug data
associated to the single pr_warn in the log. It'd actually be nice if
all kind of -EINVAL could dump their stack there too in fact
(supposedly they're not fast paths).

> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+cbc64b24b2b2d54c07a9@xxxxxxxxxxxxxxxxxxxxxxxxx
> > >
> > > RDX: 00000000000003ff RSI: 0000000020012fe0 RDI: 00007f5dbe489850
> > > RBP: 000000000072bf00 R08: 00000000000003ff R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007f5dbe48a6d4
> > > R13: 00000000004c578a R14: 00000000004d9d90 R15: 00000000ffffffff
> > > ==================================================================
> > > BUG: KASAN: use-after-free in __list_del_entry_valid+0xf1/0x100
> > > lib/list_debug.c:51
> > > CPU: 1 PID: 20306 Comm: syz-executor2 Not tainted 4.20.0-rc6-next-20181210+
> > > #164
> > > Read of size 8 at addr ffff8881c5e72bb0 by task kworker/0:1/12
> > > 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+0x244/0x39d lib/dump_stack.c:113
> > >   handle_userfault.cold.30+0x47/0x62 fs/userfaultfd.c:431
> > >   do_anonymous_page mm/memory.c:2938 [inline]
> > >   handle_pte_fault mm/memory.c:3780 [inline]
> > >   __handle_mm_fault+0x4d26/0x5b70 mm/memory.c:3906
> > >   handle_mm_fault+0x54f/0xc70 mm/memory.c:3943
> > >   do_user_addr_fault arch/x86/mm/fault.c:1475 [inline]
> > >   __do_page_fault+0x5f6/0xd70 arch/x86/mm/fault.c:1541
> > >   do_page_fault+0xf2/0x7e0 arch/x86/mm/fault.c:1572
> > >   page_fault+0x1e/0x30 arch/x86/entry/entry_64.S:1143
> > > RIP: 0033:0x4510a0
> > > Code: 0f 84 c4 0f 00 00 48 89 f1 48 89 f8 48 83 e1 3f 48 83 f9 20 0f 86 7b
> > > 02 00 00 48 83 e6 f0 48 83 e1 0f 66 0f ef c0 66 0f ef c9 <66> 0f 74 0e 66
> > > 0f d7 d1 48 d3 ea 49 c7 c2 11 00 00 00 49 29 ca 4d
> > > RSP: 002b:00007fab1fbba7a8 EFLAGS: 00010202
> > > RAX: 00007fab1fbba850 RBX: 0000000000000003 RCX: 000000000000000e
> > > RDX: 00000000000003ff RSI: 0000000020012fe0 RDI: 00007fab1fbba850
> > > RBP: 000000000072bf00 R08: 00000000000003ff R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fab1fbbb6d4
> > > R13: 00000000004c578a R14: 00000000004d9d90 R15: 00000000ffffffff
> > > CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 4.20.0-rc6-next-20181210+ #164
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > Google 01/01/2011
> > > Workqueue: events_power_efficient neigh_periodic_work
> > > Call Trace:
> > >   __dump_stack lib/dump_stack.c:77 [inline]
> > >   dump_stack+0x244/0x39d lib/dump_stack.c:113
> > >   print_address_description.cold.4+0x9/0x1ff mm/kasan/report.c:187
> > >   kasan_report.cold.5+0x1b/0x39 mm/kasan/report.c:317
> > >   __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135
> > >   __list_del_entry_valid+0xf1/0x100 lib/list_debug.c:51
> > >   __list_del_entry include/linux/list.h:117 [inline]
> > >   list_del_init include/linux/list.h:159 [inline]
> > >   neigh_mark_dead+0x13b/0x410 net/core/neighbour.c:125

The real crash seems to be completely unrelated to userfaultfd, the
list_del_init is in neigh_mark_dead.

	if (!list_empty(&n->gc_list)) {
		list_del_init(&n->gc_list);

Thanks,
Andrea