Re: [PATCH v2 10/10] x86/entry/32: Add and check a stack canary for the SYSENTER stack

[Borislav Petkov]

On Sat, Mar 05, 2016 at 09:52:23PM -0800, Andy Lutomirski wrote:

<--- ?

So all patches before are nice and verbose in explaining what they do.
Where did the commit message of this one disappear?

:-)

Or is it that obvious that we should have a stack canary? How about some
text *why* we need it?

> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/processor.h | 3 ++-
>  arch/x86/kernel/process.c        | 3 +++
>  arch/x86/kernel/traps.c          | 8 ++++++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 7cd01b71b5bd..50a6dc871cc0 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -299,8 +299,9 @@ struct tss_struct {
>  
>  #ifdef CONFIG_X86_32
>  	/*
> -	 * Space for the temporary SYSENTER stack:
> +	 * Space for the temporary SYSENTER stack.
>  	 */
> +	unsigned long		SYSENTER_stack_canary;
>  	unsigned long		SYSENTER_stack[64];
>  #endif
>  
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 9f7c21c22477..ee9a9792caeb 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -57,6 +57,9 @@ __visible DEFINE_PER_CPU_SHARED_ALIGNED(struct tss_struct, cpu_tss) = {
>  	  */
>  	.io_bitmap		= { [0 ... IO_BITMAP_LONGS] = ~0 },
>  #endif
> +#ifdef CONFIG_X86_32
> +	.SYSENTER_stack_canary	= STACK_END_MAGIC,
> +#endif
>  };
>  EXPORT_PER_CPU_SYMBOL(cpu_tss);
>  
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 80928ea78373..590110119e6a 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -713,6 +713,14 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
>  	debug_stack_usage_dec();
>  
>  exit:
> +#if defined(CONFIG_X86_32)
> +	/*
> +	 * This is the most likely code path that involves non-trivial use
> +	 * of the SYSENTER stack.  Check that we haven't overrun it.
> +	 */
> +	WARN(this_cpu_read(cpu_tss.SYSENTER_stack_canary) != STACK_END_MAGIC,
> +	     "Overran or corrupted SYSENTER stack\n");
> +#endif
>  	ist_exit(regs);
>  }
>  NOKPROBE_SYMBOL(do_debug);
> -- 
> 2.5.0
> 
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.