Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 13 Jul 2017 11:35:23 +0100
From: Dave Martin <Dave.Martin@....com>
To: Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc: linux-arm-kernel@...ts.infradead.org,
	kernel-hardening@...ts.openwall.com, mark.rutland@....com,
	catalin.marinas@....com, will.deacon@....com,
	labbott@...oraproject.org
Subject: Re: [RFC PATCH 10/10] arm64: kernel: add support for virtually
 mapped stacks

On Wed, Jul 12, 2017 at 03:44:23PM +0100, Ard Biesheuvel wrote:
> Add code that checks whether an exception taken from EL1 was caused
> by a faulting stack access before proceeding to save the interrupted
> context to the stack.
> 
> This involves checking whether the faulting address coincides with the
> guard page below the task stack of 'current'. This uses tpidrro_el0 and
> sp_el0 as scratch registers, so we can free up a couple of general
> purpose registers for use in the code that performs this check. If it
> turns out we are dealing with a stack overflow, switch to a special
> per-CPU overflow stack so we can at least call panic().
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> ---
>  arch/arm64/Kconfig                   |  1 +
>  arch/arm64/include/asm/thread_info.h |  2 +
>  arch/arm64/kernel/entry.S            | 49 ++++++++++++++++++++
>  arch/arm64/mm/fault.c                |  9 ++++
>  4 files changed, 61 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b52db8bb1270..50caf63099c8 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -73,6 +73,7 @@ config ARM64
>  	select HAVE_ARCH_SECCOMP_FILTER
>  	select HAVE_ARCH_TRACEHOOK
>  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> +	select HAVE_ARCH_VMAP_STACK
>  	select HAVE_ARM_SMCCC
>  	select HAVE_EBPF_JIT
>  	select HAVE_C_RECORDMCOUNT
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index 46c3b93cf865..1c3e0a3bf87a 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -32,6 +32,8 @@
>  #define THREAD_SIZE		16384
>  #define THREAD_START_SP		(THREAD_SIZE - 16)
>  
> +#define OVERFLOW_STACK_SIZE	1024
> +

How was this number chosen?

Kernel stack overflow is not going to get routinely tested -- after all
it should only get exercised when things go horribly wrong.

So, if the panic code (or compiler-generated stack frames) become
bloated enough, we will silently overrun this limit.

Maybe it doesn't matter if we trash a load of unrelated percpu data when
we panic.


>  #ifndef __ASSEMBLY__
>  
>  struct task_struct;
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 2ba3185b1c78..4c3e82d6e2f2 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -392,6 +392,20 @@ ENDPROC(el1_error_invalid)
>   */
>  	.align	6
>  el1_sync:
> +#ifdef CONFIG_VMAP_STACK
> +	/*
> +	 * When taking an exception from EL1, we need to check whether it is
> +	 * caused by a faulting out-of-bounds access to the virtually mapped
> +	 * stack before we can attempt to preserve the interrupted context.
> +	 */
> +	msr	tpidrro_el0, x0			// stash x0
> +	mrs	x0, far_el1			// get faulting address
> +	tbnz	x0, #63, .Lcheck_stack_ovf	// check if not user address
> +
> +.Lcheck_stack_resume:
> +	mrs	x0, tpidrro_el0			// restore x0
> +#endif
> +

Unless I'm missing something, this leaves tpidrr0_el0 trashed (and
containing kernel data that userspace can subsequently read).

This could be solved by moving the tpidrro_el0 load out of context
switch and into ret_to_user.

[...]

Cheers
---Dave

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.