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

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

[Jann Horn]

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
+
+	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

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

[Dmitry Vyukov]

On Fri, Aug 24, 2018 at 4:58 PM, Jann Horn <jannh@google.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@google.com>

Acked-by: Dmitry Vyukov <dvyukov@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
> +
> +       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
>

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

[Andrey Ryabinin]

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()?

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

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@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)
> >

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

[Andrey Ryabinin]

On 08/28/2018 01:38 PM, Jann Horn wrote:

>>
>>
>> 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. 

It doesn't really matter. This would work with instrumented oops_end() as well.
kasan_unpoison_task_stack() will unpoison everything including oops_end's stack.
It would be also ok if kasan_unpoison_task_stack() instrumented, or calling any number of instrumented functions
in between kasan_unpoison_task_stack() and rewind_stack_do_exit(). As long as we return from these functions before
the rewind_stack_do_exit(), the stack will be unpoisoned on return.

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

[Jann Horn]

On Tue, Aug 28, 2018 at 1:33 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> On 08/28/2018 01:38 PM, Jann Horn wrote:
> >> 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.
>
> It doesn't really matter. This would work with instrumented oops_end() as well.
> kasan_unpoison_task_stack() will unpoison everything including oops_end's stack.
> It would be also ok if kasan_unpoison_task_stack() instrumented,

But then that would rely on ASAN implementation details, so I'd be
hesitant to do that.

> or calling any number of instrumented functions
> in between kasan_unpoison_task_stack() and rewind_stack_do_exit(). As long as we return from these functions before
> the rewind_stack_do_exit(), the stack will be unpoisoned on return.

Yeah, I understand that. :)

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

[Andy Lutomirski]

> On Aug 28, 2018, at 4:33 AM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> 
> 
> 
> On 08/28/2018 01:38 PM, Jann Horn wrote:
> 
>>> 
>>> 
>>> 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. 
> 
> It doesn't really matter. This would work with instrumented oops_end() as well.
> kasan_unpoison_task_stack() will unpoison everything including oops_end's stack.
> It would be also ok if kasan_unpoison_task_stack() instrumented, or calling any number of instrumented functions
> in between kasan_unpoison_task_stack() and rewind_stack_do_exit(). As long as we return from these functions before
> the rewind_stack_do_exit(), the stack will be unpoisoned on return.

I think that, if all this is in C, we need rewind_stack_do_exit()’s caller to be uninstrumented. Which it is.