[PATCH 20/23] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Rohan McLure]

> On 20 Sep 2022, at 2:54 pm, Rohan McLure <rmclure at linux.ibm.com> wrote: 
> 
>> On 20 Sep 2022, at 12:03 pm, Nicholas Piggin <npiggin at gmail.com> wrote:
>> 
>> On Fri Sep 16, 2022 at 3:32 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 at linux.ibm.com>
>>> ---
>>> V1 -> V2: Update summary
>>> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
>>> V3 -> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
>>> V4 -> V5: Move to end of patch series.
>>> ---
>> 
>> I think it looks okay. I'll have to take a better look with the series
>> applied.
> 
> 
> Your comments alerted me to the fact that general interrupt and syscalls
> share their exit code in interrupt_return and its derivatives. Meaning
> that disabling INTERRUPT_SANITIZE_REGISTERS also reverts restores of NVGPRS
> to being optional, which makes it possible to clobber NVGPRS and then not
> restore them. The cleanest way forward I belive is going to be to cause
> INTERRUPT_SANITIZE_REGISTERS to perform sanitisation on all interrupt sources
> rather than continuing with syscalls as their own special case. I’ll put
> this out in a v6 soon.

I think I managed to confuse myself here. Syscall handlers directly RFID, while
other interrupt sources share a common exit path.

>> 
>>> arch/powerpc/kernel/interrupt_64.S | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
>>> index 16a1b44088e7..40147558e1a6 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 */
>> 
>> These two comment changes could go in your system_call_exception API
>> change patch though.
>> 
>> Thanks,
>> Nick
>> 
>>> 	SAVE_GPRS(3, 8, r1)
>>> 	/* Zero r9-r12, this should only be required when restoring all GPRs */
>>> 	std	r11,GPR9(r1)
>>> @@ -110,6 +110,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>> 	 * 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
>>> @@ -140,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
>>> 
>>> @@ -243,7 +245,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 */
>>> 	SAVE_GPRS(3, 8, r1)
>>> 	/* Zero r9-r12, this should only be required when restoring all GPRs */
>>> 	std	r11,GPR9(r1)
>>> @@ -295,6 +297,7 @@ END_BTB_FLUSH_SECTION
>>> 	 * 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
>>> @@ -337,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 */
>>> @@ -364,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