[v6,22/25] powerpc/64s: Clear user GPRs in syscall interrupt entry

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>
---
V2: Update summary
V3: Remove erroneous summary paragraph on syscall_exit_prepare
V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
V5: Move to end of patch series.
V6: Include clears which were previously in the syscall wrapper patch.
Move comment on r3-r8 register save to when we alter the calling
convention for system_call_exception.
---
 arch/powerpc/kernel/interrupt_64.S | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

On Wed Sep 21, 2022 at 4:56 PM AEST, Rohan McLure wrote:
> 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>
> ---
> V2: Update summary
> V3: Remove erroneous summary paragraph on syscall_exit_prepare
> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
> V5: Move to end of patch series.
> V6: Include clears which were previously in the syscall wrapper patch.
> Move comment on r3-r8 register save to when we alter the calling
> convention for system_call_exception.

The series looks good to here, I just need to find a bit more time to
look at the code and do some tests with the next few patches. I don't
see much problem with them, looks a lot better now with fewer ifdefs
so that's good. Possibly you could share some of those new sanitize
macros in a header file but that's a minor nit.

Coud we have this zeroize also under the same config option as the
next? I figure if we care about speculative security we want both,
and if we don't we need neither.

Thanks,
Nick

> ---
>  arch/powerpc/kernel/interrupt_64.S | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index a5dd78bdbe6d..40147558e1a6 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -106,6 +106,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.
> +	 */
> +	ZEROIZE_GPR(0)
> +	ZEROIZE_GPRS(5, 12)
> +	ZEROIZE_NVGPRS()
>  	bl	system_call_exception
>  
>  .Lsyscall_vectored_\name\()_exit:
> @@ -134,6 +141,7 @@ BEGIN_FTR_SECTION
>  	HMT_MEDIUM_LOW
>  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  
> +	REST_NVGPRS(r1)
>  	cmpdi	r3,0
>  	bne	.Lsyscall_vectored_\name\()_restore_regs
>  
> @@ -285,6 +293,13 @@ END_BTB_FLUSH_SECTION
>  	wrteei	1
>  #endif
>  
> +	/*
> +	 * Zero user registers to prevent influencing speculative execution
> +	 * state of kernel code.
> +	 */
> +	ZEROIZE_GPR(0)
> +	ZEROIZE_GPRS(5, 12)
> +	ZEROIZE_NVGPRS()
>  	bl	system_call_exception
>  
>  .Lsyscall_exit:
> @@ -325,6 +340,7 @@ BEGIN_FTR_SECTION
>  	stdcx.	r0,0,r1			/* to clear the reservation */
>  END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>  
> +	REST_NVGPRS(r1)
>  	cmpdi	r3,0
>  	bne	.Lsyscall_restore_regs
>  	/* Zero volatile regs that may contain sensitive kernel data */
> @@ -352,7 +368,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
>  	REST_GPR(0, r1)
> -- 
> 2.34.1

> On 23 Sep 2022, at 6:02 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> On Wed Sep 21, 2022 at 4:56 PM AEST, Rohan McLure wrote:
>> 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>
>> ---
>> V2: Update summary
>> V3: Remove erroneous summary paragraph on syscall_exit_prepare
>> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
>> V5: Move to end of patch series.
>> V6: Include clears which were previously in the syscall wrapper patch.
>> Move comment on r3-r8 register save to when we alter the calling
>> convention for system_call_exception.
> 
> The series looks good to here, I just need to find a bit more time to
> look at the code and do some tests with the next few patches. I don't
> see much problem with them, looks a lot better now with fewer ifdefs
> so that's good. Possibly you could share some of those new sanitize
> macros in a header file but that's a minor nit.
> 
> Coud we have this zeroize also under the same config option as the
> next? I figure if we care about speculative security we want both,
> and if we don't we need neither.

Thanks for this. I’ll resubmit the last four patches with a common
configuration option for register clearing. Just pinging to hear your
thoughts on whether we are willing to wear the small performance
regression in general interrupts, and whether this mitigation should
be a default on pseries.

Rohan

> 
> Thanks,
> Nick
> 
>> ---
>> arch/powerpc/kernel/interrupt_64.S | 17 ++++++++++++++++-
>> 1 file changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
>> index a5dd78bdbe6d..40147558e1a6 100644
>> --- a/arch/powerpc/kernel/interrupt_64.S
>> +++ b/arch/powerpc/kernel/interrupt_64.S
>> @@ -106,6 +106,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.
>> +  */
>> + ZEROIZE_GPR(0)
>> + ZEROIZE_GPRS(5, 12)
>> + ZEROIZE_NVGPRS()
>> bl system_call_exception
>> 
>> .Lsyscall_vectored_\name\()_exit:
>> @@ -134,6 +141,7 @@ BEGIN_FTR_SECTION
>> HMT_MEDIUM_LOW
>> END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> 
>> + REST_NVGPRS(r1)
>> cmpdi r3,0
>> bne .Lsyscall_vectored_\name\()_restore_regs
>> 
>> @@ -285,6 +293,13 @@ END_BTB_FLUSH_SECTION
>> wrteei 1
>> #endif
>> 
>> + /*
>> +  * Zero user registers to prevent influencing speculative execution
>> +  * state of kernel code.
>> +  */
>> + ZEROIZE_GPR(0)
>> + ZEROIZE_GPRS(5, 12)
>> + ZEROIZE_NVGPRS()
>> bl system_call_exception
>> 
>> .Lsyscall_exit:
>> @@ -325,6 +340,7 @@ BEGIN_FTR_SECTION
>> stdcx. r0,0,r1 /* to clear the reservation */
>> END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>> 
>> + REST_NVGPRS(r1)
>> cmpdi r3,0
>> bne .Lsyscall_restore_regs
>> /* Zero volatile regs that may contain sensitive kernel data */
>> @@ -352,7 +368,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
>> REST_GPR(0, r1)
>> -- 
>> 2.34.1