linux-kernel - Re: [PATCH 1/1] arm64/entry.S: check for stack overflow in el1 case only

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]

Date:   Thu, 7 Jan 2021 11:29:03 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Maninder Singh <maninder1.s@...sung.com>
Cc:     catalin.marinas@....com, will@...nel.org, broonie@...nel.org,
        vincenzo.frascino@....com, samitolvanen@...gle.com,
        ardb@...nel.org, maz@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        v.narang@...sung.com, a.sahrawat@...sung.com
Subject: Re: [PATCH 1/1] arm64/entry.S: check for stack overflow in el1 case
 only

On Fri, Dec 11, 2020 at 02:45:31PM +0530, Maninder Singh wrote:
> current code checks for sp bit flip in all exceptions,
> but only el1 exceptions requires this. el0 can not enter
> into stack overflow case directly.
> 
> it will improve performance for el0 exceptions and interrupts.
> 
> Signed-off-by: Maninder Singh <maninder1.s@...sung.com>
> Signed-off-by: Vaneet Narang <v.narang@...sung.com>

I did consider doing this at the time Ard and I wrote the overflow
detection, but there was no measureable impact on the workloads that I
tested, and it seemed worthwhile to have this as a sanity check in case
the SP was somehow corrupted (and to avoid any surprizing differences
between the EL0 and EL1 entry paths).

When you say "it will improve performance for el0 exceptions and
interrupts", do you have a workload where this has a measureable impact,
or was this found by inspection? Unless this is causing a real issue,
I'd prefer to leave it as-is for now.

Thanks,
Mark.

> ---
>  arch/arm64/kernel/entry.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 2a93fa5..cad8faf 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -77,6 +77,7 @@ alternative_else_nop_endif
>  
>  	sub	sp, sp, #S_FRAME_SIZE
>  #ifdef CONFIG_VMAP_STACK
> +	.if	\el == 1
>  	/*
>  	 * Test whether the SP has overflowed, without corrupting a GPR.
>  	 * Task and IRQ stacks are aligned so that SP & (1 << THREAD_SHIFT)
> @@ -118,6 +119,7 @@ alternative_else_nop_endif
>  	/* We were already on the overflow stack. Restore sp/x0 and carry on. */
>  	sub	sp, sp, x0
>  	mrs	x0, tpidrro_el0
> +	.endif
>  #endif
>  	b	el\()\el\()_\label
>  	.endm
> -- 
> 1.9.1
>