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 <npig...@gmail.com> > Signed-off-by: Rohan McLure <rmcl...@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 <npig...@gmail.com> Thanks, Nick
