Re: [PATCH v3 14/18] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 19/08/2022 à 05:38, Rohan McLure a écrit :
> Clear user state in gprs (assign to zero) to reduce the influence of user
> registers on speculation within kernel syscall handlers. Clears occur
> at the very beginning of the sc and scv 0 interrupt handlers, with
> restores occurring following the execution of the syscall handler.
> 
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Update summary
> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
> ---
>   arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index 0178aeba3820..d9625113c7a5 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -70,7 +70,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
>   	ld	r2,PACATOC(r13)
>   	mfcr	r12
>   	li	r11,0
> -	/* Can we avoid saving r3-r8 in common case? */
> +	/* Save syscall parameters in r3-r8 */
>   	std	r3,GPR3(r1)
>   	std	r4,GPR4(r1)
>   	std	r5,GPR5(r1)
> @@ -109,6 +109,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>   	 * but this is the best we can do.
>   	 */
>   
> +	/*
> +	 * Zero user registers to prevent influencing speculative execution
> +	 * state of kernel code.
> +	 */
> +	NULLIFY_GPRS(5, 12)

Macro name has changed.

> +	NULLIFY_NVGPRS()

Why clearing non volatile GPRs ? They are supposed to be callee saved so 
I can't see how they could be used for speculation. Do you have any 
exemple ?

> +
>   	/* Calling convention has r3 = orig r0, r4 = regs */
>   	mr	r3,r0
>   	bl	system_call_exception
> @@ -139,6 +146,7 @@ BEGIN_FTR_SECTION
>   	HMT_MEDIUM_LOW
>   END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>   
> +	REST_NVGPRS(r1)

What is the link between this change and the description in the commit 
message ?

>   	cmpdi	r3,0
>   	bne	.Lsyscall_vectored_\name\()_restore_regs
>   
> @@ -181,7 +189,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>   	ld	r4,_LINK(r1)
>   	ld	r5,_XER(r1)
>   
> -	REST_NVGPRS(r1)
>   	ld	r0,GPR0(r1)
>   	mtcr	r2
>   	mtctr	r3
> @@ -249,7 +256,7 @@ END_BTB_FLUSH_SECTION
>   	ld	r2,PACATOC(r13)
>   	mfcr	r12
>   	li	r11,0
> -	/* Can we avoid saving r3-r8 in common case? */
> +	/* Save syscall parameters in r3-r8 */
>   	std	r3,GPR3(r1)
>   	std	r4,GPR4(r1)
>   	std	r5,GPR5(r1)
> @@ -300,6 +307,13 @@ END_BTB_FLUSH_SECTION
>   	wrteei	1
>   #endif
>   
> +	/*
> +	 * Zero user registers to prevent influencing speculative execution
> +	 * state of kernel code.
> +	 */
> +	NULLIFY_GPRS(5, 12)
> +	NULLIFY_NVGPRS()
> +

Name has changed.

>   	/* Calling convention has r3 = orig r0, r4 = regs */
>   	mr	r3,r0
>   	bl	system_call_exception
> @@ -342,6 +356,7 @@ BEGIN_FTR_SECTION
>   	stdcx.	r0,0,r1			/* to clear the reservation */
>   END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>   
> +	REST_NVGPRS(r1)

Same question.

>   	cmpdi	r3,0
>   	bne	.Lsyscall_restore_regs
>   	/* Zero volatile regs that may contain sensitive kernel data */
> @@ -377,7 +392,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>   .Lsyscall_restore_regs:
>   	ld	r3,_CTR(r1)
>   	ld	r4,_XER(r1)
> -	REST_NVGPRS(r1)
>   	mtctr	r3
>   	mtspr	SPRN_XER,r4
>   	ld	r0,GPR0(r1)