[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACT4Y+YPh3QqononWCyXBByuzjFHrDz_PZrbh9XK7toiJMA82g@mail.gmail.com>
Date: Fri, 24 Aug 2018 17:13:43 -0700
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Jann Horn <jannh@...gle.com>
Cc: Andy Lutomirski <luto@...nel.org>,
"the arch/x86 maintainers" <x86@...nel.org>,
kernel list <linux-kernel@...r.kernel.org>,
kasan-dev <kasan-dev@...glegroups.com>,
Andrey Ryabinin <aryabinin@...tuozzo.com>,
Alexander Potapenko <glider@...gle.com>,
Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] x86/entry/64: wipe KASAN stack shadow in rewind_stack_do_exit()
On Fri, Aug 24, 2018 at 4:58 PM, Jann Horn <jannh@...gle.com> 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@...gle.com>
Acked-by: Dmitry Vyukov <dvyukov@...gle.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
> +
> + movq %r15, %rsp
> UNWIND_HINT_FUNC sp_offset=PTREGS_SIZE
>
> + movq %r14, %rdi
> call do_exit
> END(rewind_stack_do_exit)
> --
> 2.19.0.rc0.228.g281dcd1b4d0-goog
>
Powered by blists - more mailing lists