Messages in this thread | | | From | Jann Horn <> | Date | Tue, 28 Aug 2018 12:38:53 +0200 | Subject | Re: [PATCH] x86/entry/64: wipe KASAN stack shadow in rewind_stack_do_exit() |
| |
On Tue, Aug 28, 2018 at 11:04 AM Andrey Ryabinin <aryabinin@virtuozzo.com> 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@google.com> > > --- > > 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) > >
| |