Re: [PATCH] x86/entry/64: wipe KASAN stack shadow in rewind_stack_do_exit()

[Jann Horn]

On Tue, Aug 28, 2018 at 11:04 AM Andrey Ryabinin
<aryabinin@xxxxxxxxxxxxx> wrote:
>
> On 08/25/2018 02:58 AM, Jann Horn wrote:
> > Reset the KASAN shadow state of the task stack when rewinding RSP.
> > Without this, a kernel oops will leave parts of the stack poisoned, and
> > code running under do_exit() can trip over such poisoned regions and cause
> > nonsensical false-positive KASAN reports about stack-out-of-bounds bugs.
> >
> > This patch is 64-bit only because KASAN doesn't exist on 32-bit.
> >
> > This patch does not wipe exception stacks; if you oops on an exception
> > stack, you might get random KASAN false-positives from other tasks
> > afterwards. This is probably relatively uninteresting, since if you're
> > oopsing on an exception stack, you likely have bigger things to worry
> > about. It'd be more interesting if vmapped stacks and KASAN were
> > compatible, since then handle_stack_overflow() would oops from exception
> > stack context.
> >
> > Fixes: 2deb4be28077 ("x86/dumpstack: When OOPSing, rewind the stack before do_exit()")
> > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > ---
> > I have manually tested that an oops that previously triggered this bug
> > doesn't trigger it anymore.
> >
> > It would be possible to rewrite this assembly to use fewer instructions
> > in non-KASAN builds, but I think it's clearer this way.
> >
> > If anyone thinks that this thing should also be wiping exception stacks:
> > I did write some (entirely untested) code that should take care of that
> > (before realizing that it's rather unlikely to occur in practice because
> > vmapped stacks and KASAN are mutually exclusive), but I'm not sure
> > whether it's worth complicating this code for that.
> > In case anyone's curious how that would look:
> > https://gist.github.com/thejh/c91f9b4e3cc4c58659bb3cd056c4fa40
> >
> >  arch/x86/entry/entry_64.S | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> > index 957dfb693ecc..92d3ad5bd365 100644
> > --- a/arch/x86/entry/entry_64.S
> > +++ b/arch/x86/entry/entry_64.S
> > @@ -1673,9 +1673,25 @@ ENTRY(rewind_stack_do_exit)
> >       /* Prevent any naive code from trying to unwind to our caller. */
> >       xorl    %ebp, %ebp
> >
> > +     movq    %rdi, %r14
> > +
> >       movq    PER_CPU_VAR(cpu_current_top_of_stack), %rax
> > -     leaq    -PTREGS_SIZE(%rax), %rsp
> > +     leaq    -PTREGS_SIZE(%rax), %r15
> > +
> > +#ifdef CONFIG_KASAN
> > +     /*
> > +      * Remove stack poisons left behind by our old stack.
> > +      * Do this before updating RSP to avoid problems in case we get some
> > +      * interrupt that is not handled on an exception stack before we're done
> > +      * with the unpoisoning.
> > +      */
> > +     movq    %r15, %rdi
> > +     call    kasan_unpoison_task_stack_below
> > +#endif
>
>
> Why this has to be done in the rewind_stack_do_exit()?
> Are there any problems with calling the kasan_unpoison_task_stack(current) from oops_end(), before the rewind_stack_do_exit()?

Ooh, good point! I didn't see that KASAN instrumentation is disabled
for dumpstack.c. So I guess I'll send a new patch that does it from
oops_end().

> > +
> > +     movq    %r15, %rsp
> >       UNWIND_HINT_FUNC sp_offset=PTREGS_SIZE
> >
> > +     movq    %r14, %rdi
> >       call    do_exit
> >  END(rewind_stack_do_exit)
> >