linux-kernel - Re: [PATCH] [RFC] aarch64: scs: reload shadow call stack in user exception entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YdQkM8IlonhEhU/R@FVFF77S0Q05N>
Date:   Tue, 4 Jan 2022 10:40:51 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Dan Li <ashimida@...ux.alibaba.com>
Cc:     catalin.marinas@....com, will@...nel.org, samitolvanen@...gle.com,
        maz@...nel.org, joey.gouly@....com, pcc@...gle.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH] [RFC] aarch64: scs: reload shadow call stack in user
 exception entry

Hi,

On Mon, Dec 27, 2021 at 05:06:04PM -0800, Dan Li wrote:
> When el0 exception occurs, kernel_entry/exit will load/save tsk->scs_sp
> to ensure scs working properly. AFAIK, the SCS offset should always be
> 0 at this time.
> 
> Is it reasonable to reload x18 to scs_base directly in kernel_entry
> here, or am I missing something?
> 
> Signed-off-by: Dan Li <ashimida@...ux.alibaba.com>

AFAICT the saving here is just that we avoid storing the SCS SP upon return to
EL0, and I suspect that in practice that's not measureable.

So without numbers to show otherwise, I'd prefer to leave this as-is, though I
agree that this patch should work.  If we do want to change this, I think we
need a comment in the `kernel_exit` path to pair with the `scs_load` in
`kernel_entry`.

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/scs.h | 7 +++++++
>  arch/arm64/kernel/entry.S    | 3 +--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
> index 8297bccf0784..2bc0d0575e75 100644
> --- a/arch/arm64/include/asm/scs.h
> +++ b/arch/arm64/include/asm/scs.h
> @@ -9,6 +9,10 @@
>  #ifdef CONFIG_SHADOW_CALL_STACK
>  	scs_sp	.req	x18
>  
> +	.macro scs_reload tsk
> +	ldr	scs_sp, [\tsk, #TSK_TI_SCS_BASE]
> +	.endm
> +
>  	.macro scs_load tsk
>  	ldr	scs_sp, [\tsk, #TSK_TI_SCS_SP]
>  	.endm
> @@ -17,6 +21,9 @@
>  	str	scs_sp, [\tsk, #TSK_TI_SCS_SP]
>  	.endm
>  #else
> +	.macro scs_reload tsk
> +	.endm
> +
>  	.macro scs_load tsk
>  	.endm
>  
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index bc6d5a970a13..57547a3e4f7c 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -265,7 +265,7 @@ alternative_if ARM64_HAS_ADDRESS_AUTH
>  alternative_else_nop_endif
>  1:
>  
> -	scs_load tsk
> +	scs_reload tsk
>  	.else
>  	add	x21, sp, #PT_REGS_SIZE
>  	get_current_task tsk
> @@ -365,7 +365,6 @@ alternative_if ARM64_WORKAROUND_845719
>  alternative_else_nop_endif
>  #endif
>  3:
> -	scs_save tsk
>  
>  	/* Ignore asynchronous tag check faults in the uaccess routines */
>  	ldr	x0, [tsk, THREAD_SCTLR_USER]
> -- 
> 2.17.1
>