On Fri, 19 Aug 2016, a...@linux-foundation.org wrote: > From: Vegard Nossum <vegard.nos...@oracle.com> > Subject: stackdepot: fix mempolicy use-after-free > > This patch fixes the following: > > BUG: KASAN: use-after-free in alloc_pages_current+0x363/0x370 at addr > ffff88010b48102c > Read of size 2 by task trinity-c2/15425 > CPU: 0 PID: 15425 Comm: trinity-c2 Not tainted 4.8.0-rc2+ #140 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.9.3-0-ge2fc41e-prebuilt.qemu-proje > ct.org 04/01/2014 > ffff88010b481040 ffff88010b557650 ffffffff81f08d11 ffff88011a40d380 > ffff88010b481028 ffff88010b557678 ffffffff815dac7c ffff88010b557708 > ffff88010b481028 ffff88011a40d380 ffff88010b5576f8 ffffffff815daf15 > Call Trace: > [<ffffffff81f08d11>] dump_stack+0x65/0x84 > [<ffffffff815dac7c>] kasan_object_err+0x1c/0x70 > [<ffffffff815daf15>] kasan_report_error+0x1f5/0x4c0 > [<ffffffff815db2fe>] __asan_report_load2_noabort+0x3e/0x40 > [<ffffffff815cb903>] alloc_pages_current+0x363/0x370 <---- > use-after-free > [<ffffffff81fa9954>] depot_save_stack+0x3f4/0x490 > [<ffffffff815d9bb5>] save_stack+0xb5/0xd0 > [<ffffffff815da211>] kasan_slab_free+0x71/0xb0 > [<ffffffff815d6643>] kmem_cache_free+0xa3/0x290 > [<ffffffff815c8149>] __mpol_put+0x19/0x20 <---- free > [<ffffffff81260635>] do_exit+0x1515/0x2b70 > [<ffffffff81261dc4>] do_group_exit+0xf4/0x2f0 > [<ffffffff81281c5d>] get_signal+0x53d/0x1120 > [<ffffffff8119e993>] do_signal+0x83/0x1e20 > [<ffffffff810027af>] exit_to_usermode_loop+0xaf/0x140 > [<ffffffff810051e4>] syscall_return_slowpath+0x144/0x170 > [<ffffffff83ae406f>] ret_from_fork+0x2f/0x40 > Read of size 2 by task trinity-c2/15425 > > The problem is that we may be calling alloc_pages() in a code path where > current->mempolicy has already been freed. > > By passing __GFP_THISNODE we will always use default_mempolicy (which > cannot be freed). > > Link: https://lkml.org/lkml/2016/7/29/277 > Link: https://github.com/google/kasan/issues/35 > Link: > http://lkml.kernel.org/r/1471603265-31804-1-git-send-email-vegard.nos...@oracle.com > Signed-off-by: Vegard Nossum <vegard.nos...@oracle.com> > Acked-by: Dmitry Vyukov <dvyu...@google.com> > Cc: Alexander Potapenko <gli...@google.com> > Cc: Andrey Ryabinin <aryabi...@virtuozzo.com> > Signed-off-by: Andrew Morton <a...@linux-foundation.org> > --- > > lib/stackdepot.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff -puN lib/stackdepot.c~stackdepot-fix-mempolicy-use-after-free > lib/stackdepot.c > --- a/lib/stackdepot.c~stackdepot-fix-mempolicy-use-after-free > +++ a/lib/stackdepot.c > @@ -243,6 +243,12 @@ depot_stack_handle_t depot_save_stack(st > alloc_flags &= ~GFP_ZONEMASK; > alloc_flags &= (GFP_ATOMIC | GFP_KERNEL); > alloc_flags |= __GFP_NOWARN; > + /* > + * Avoid using current->mempolicy which may already have > + * been freed -- we may be in the process of saving the > + * stack for exactly that __mpol_put() call. > + */ > + alloc_flags |= __GFP_THISNODE; > page = alloc_pages(alloc_flags, STACK_ALLOC_ORDER); > if (page) > prealloc = page_address(page);
This is wrong, it unnecessarily restricts the allocation to a local node and has a greater chance to fail. Passing __GFP_THISNODE here is only an implementation detail of mempolicies to avoid reference to freed policy. It is easy to fix by using alloc_pages_node(NUMA_NO_NODE, alloc_flags, STACK_ALLOC_ORDER) instead of alloc_pages() directly.
