> On 20 Sep 2022, at 12:03 pm, Nicholas Piggin <npig...@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 <rmcl...@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
