Re: [PATCH] scs: Release kasan vmalloc poison in scs_free process

[Kuan-Ying Lee]

On Fri, 2021-09-24 at 07:35 -0700, Sami Tolvanen wrote:
> On Thu, Sep 23, 2021 at 4:19 AM Kuan-Ying Lee
> <Kuan-Ying.Lee@xxxxxxxxxxxx> wrote:
> > 
> > On Thu, 2021-09-23 at 17:53 +0800, yee.lee@xxxxxxxxxxxx wrote:
> > > From: Yee Lee <yee.lee@xxxxxxxxxxxx>
> > > 
> > > Since scs allocation has been moved to vmalloc region, the
> > > shadow stack is protected by kasan_posion_vmalloc.
> > > However, the vfree_atomic operation needs to access
> > > its context for scs_free process and causes kasan error
> > > as the dump info below.
> > > 
> > > This patch Adds kasan_unpoison_vmalloc() before vfree_atomic,
> > > which aligns to the prior flow as using kmem_cache.
> > > The vmalloc region will go back posioned in the following
> > > vumap() operations.
> > > 
> > >  ================================================================
> > > ==
> > >  BUG: KASAN: vmalloc-out-of-bounds in llist_add_batch+0x60/0xd4
> > >  Write of size 8 at addr ffff8000100b9000 by task kthreadd/2
> > > 
> > >  CPU: 0 PID: 2 Comm: kthreadd Not tainted 5.15.0-rc2-11681-(skip)
> > >  Hardware name: linux,dummy-virt (DT)
> > >  Call trace:
> > >   dump_backtrace+0x0/0x43c
> > >   show_stack+0x1c/0x2c
> > >   dump_stack_lvl+0x68/0x84
> > >   print_address_description+0x80/0x394
> > >   kasan_report+0x180/0x1dc
> > >   __asan_report_store8_noabort+0x48/0x58
> > >   llist_add_batch+0x60/0xd4
> > >   vfree_atomic+0x60/0xe0
> > >   scs_free+0x1dc/0x1fc
> > >   scs_release+0xa4/0xd4
> > >   free_task+0x30/0xe4
> > >   __put_task_struct+0x1ec/0x2e0
> > >   delayed_put_task_struct+0x5c/0xa0
> > >   rcu_do_batch+0x62c/0x8a0
> > >   rcu_core+0x60c/0xc14
> > >   rcu_core_si+0x14/0x24
> > >   __do_softirq+0x19c/0x68c
> > >   irq_exit+0x118/0x2dc
> > >   handle_domain_irq+0xcc/0x134
> > >   gic_handle_irq+0x7c/0x1bc
> > >   call_on_irq_stack+0x40/0x70
> > >   do_interrupt_handler+0x78/0x9c
> > >   el1_interrupt+0x34/0x60
> > >   el1h_64_irq_handler+0x1c/0x2c
> > >   el1h_64_irq+0x78/0x7c
> > >   _raw_spin_unlock_irqrestore+0x40/0xcc
> > >   sched_fork+0x4f0/0xb00
> > >   copy_process+0xacc/0x3648
> > >   kernel_clone+0x168/0x534
> > >   kernel_thread+0x13c/0x1b0
> > >   kthreadd+0x2bc/0x400
> > >   ret_from_fork+0x10/0x20
> > > 
> > >  Memory state around the buggy address:
> > >   ffff8000100b8f00: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
> > > f8
> > >   ffff8000100b8f80: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
> > > f8
> > >  >ffff8000100b9000: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
> > > f8
> > >                     ^
> > >   ffff8000100b9080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
> > > f8
> > >   ffff8000100b9100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
> > > f8
> > >  ================================================================
> > > ==
> > > 
> > > CC: Matthias Brugger <matthias.bgg@xxxxxxxxx>
> > > CC: Will Deacon <will@xxxxxxxxxx>
> > > CC: Sami Tolvanen <samitolvanen@xxxxxxxxxx>
> > > Signed-off-by: Yee Lee <yee.lee@xxxxxxxxxxxx>
> > > ---
> > >  kernel/scs.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/kernel/scs.c b/kernel/scs.c
> > > index e2a71fc82fa0..25c0d8e416e6 100644
> > > --- a/kernel/scs.c
> > > +++ b/kernel/scs.c
> > > @@ -68,6 +68,7 @@ void scs_free(void *s)
> > > 
> > >       __scs_account(s, -1);
> > > 
> > > +     kasan_unpoison_vmalloc(s, SCS_SIZE);
> > >       /*
> > >        * We cannot sleep as this can be called in interrupt
> > > context,
> > >        * so use this_cpu_cmpxchg to update the cache, and
> > > vfree_atomic
> > 
> > I'm not sure if we need to add kasan_unpoison_vmalloc() and
> > kasan_poison_vmalloc() in this file.
> > 
> > As far as I know, vmalloc() and vfree() will do these two
> > functions.
> 
> The idea here is to poison the shadow stack after it's set up to
> catch
> unintentional accesses. Outside of compiler instrumentation, nothing
> should read or write from this buffer while the task is running.

Got it.
Thanks.

Reviewed-by: Kuan-Ying Lee <Kuan-Ying.Lee@xxxxxxxxxxxx>

> 
> Sami