Re: [PATCH v4 5/7] powerpc/64s: Zeroise gprs on interrupt routine entry on Book3S

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Nov 29, 2022 at 2:43 PM AEST, Rohan McLure wrote:
> Zeroise 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.
>
> Zeroise GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
> other interrupt sources. The remaining gprs are overwritten by
> entry macros to interrupt handlers, irrespective of whether or not a
> given handler consumes these register values. If an interrupt does not
> select the IMSR_R12 IOption, zeroise r12.
>
> Prior to this commit, r14-r31 are restored on a per-interrupt basis at
> exit, but now they are always restored on 64bit Book3S. Remove explicit
> REST_NVGPRS invocations on 64-bit Book3S. 32-bit systems do not clear
> user registers on interrupt, and continue to depend on the return value
> of interrupt_exit_user_prepare to determine whether or not to restore
> non-volatiles.
>
> The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
> See ~0.8% performance regression with this mitigation, but this
> indicates the worst-case performance due to heavier-weight interrupt
> handlers. This mitigation is able to be enabled/disabled through
> CONFIG_INTERRUPT_SANITIZE_REGISTERS.
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> v2: REST_NVGPRS should be conditional on mitigation in scv handler. Fix
> improper multi-line preprocessor macro in interrupt_64.S
> v4: Split off IMSR_R12 definition into its own patch. Move macro
> definitions for register sanitisation into asm/ppc_asm.h
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 27 ++++++++++++++++++---------
>  arch/powerpc/kernel/interrupt_64.S   | 16 ++++++++++++++--
>  2 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 58d72db1d484..8ee3db3b6595 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -506,6 +506,7 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real, text)
>  	std	r10,0(r1)		/* make stack chain pointer	*/
>  	std	r0,GPR0(r1)		/* save r0 in stackframe	*/
>  	std	r10,GPR1(r1)		/* save r1 in stackframe	*/
> +	ZEROIZE_GPR(0)
>  
>  	/* Mark our [H]SRRs valid for return */
>  	li	r10,1
> @@ -548,8 +549,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	std	r9,GPR11(r1)
>  	std	r10,GPR12(r1)
>  	std	r11,GPR13(r1)
> +	.if !IMSR_R12
> +	ZEROIZE_GPRS(9, 12)
> +	.else
> +	ZEROIZE_GPRS(9, 11)
> +	.endif

These unconditionally zero. Should be SANITIZE_ZEROIZE_GPRS()? Same
for a few more cases.

Hmm. r12 actually contains come-from-MSR here, which isn't really user
controlled. r9-r11 just got loaded with some user GPRs, but they're
the usual scratch registers and get used for other things later.

The whole interrupt entry file could probably use a spring clean, some
re-scheduling, data layout improvements, and more consistent use of
scratch registers... so I'm overly concerned about removing every
possible redundant instruction here if it makes things a bit harder
to follow. But we might be able to get rid of the above zeroize, with
a comment.

>  
>  	SAVE_NVGPRS(r1)
> +	SANITIZE_ZEROIZE_NVGPRS()
>  
>  	.if IDAR
>  	.if IISIDE
> @@ -581,8 +588,8 @@ BEGIN_FTR_SECTION
>  END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
>  	ld	r10,IAREA+EX_CTR(r13)
>  	std	r10,_CTR(r1)
> -	std	r2,GPR2(r1)		/* save r2 in stackframe	*/
> -	SAVE_GPRS(3, 8, r1)		/* save r3 - r8 in stackframe   */
> +	SAVE_GPRS(2, 8, r1)		/* save r2 - r8 in stackframe   */
> +	ZEROIZE_GPRS(2, 8)
>  	mflr	r9			/* Get LR, later save to stack	*/
>  	LOAD_PACA_TOC()			/* get kernel TOC into r2	*/
>  	std	r9,_LINK(r1)

r2 gets zeroed then used again in LOAD_PACA_TOC too.

Otherwise looks pretty good... Although CTR doesn't get zeroed and I
suppose it could be speculatively used as a source (e.g., bctr). CRs
other than 0 and sometimes 1 too, they're probably a bit less important
than CTR though. We don't use TAR in the kernel so that one should be
okay to leave (maybe with a comment).

That can be done another time, I'd like to see the GPR sanitising
patches go in... It *might* just be a matter of one mtctr in the
case of !RELOCATABLE kernel though to get the ctr...

Still-Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick