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

[Rohan McLure]

> 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.

> 
>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20220920/d3fabc57/attachment-0001.htm>