Message ID | 20220916053300.786330-21-rmclure@linux.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | powerpc: Syscall wrapper and register clearing | expand |
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@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. > 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
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@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. > --- Oh I would say change the title more like the interrupt patches to avoid mentioning the restore/return because that's implicit. e.g., "Clear user GPRs in syscall interrupt entry" Could we have the one config option that does both this and the interrupt zeroing? Thanks, Nick > 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 */ > 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
> On 20 Sep 2022, at 12:03 pm, Nicholas Piggin <npiggin@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@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
> On 20 Sep 2022, at 2:54 pm, Rohan McLure <rmclure@linux.ibm.com> wrote: > >> On 20 Sep 2022, at 12:03 pm, Nicholas Piggin <npiggin@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@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
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 */ 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)
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> --- 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. --- arch/powerpc/kernel/interrupt_64.S | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)