| Message ID | 20220824020548.62625-20-rmclure@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc: Syscall wrapper and register clearing | expand |
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote: > Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all > other interrupt sources to limit influence of user-space values > in potential speculation gadgets. The remaining gprs are overwritten by > entry macros to interrupt handlers, irrespective of whether or not a > given handler consumes these register values. > > Prior to this commit, r14-r31 are restored on a per-interrupt basis at > exit, but now they are always restored. Remove explicit REST_NVGPRS > invocations as non-volatiles must now always be restored. 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. Ow, my heart :( Are we not keeping a CONFIG option to rid ourselves of this vile performance robbing thing? Are we getting rid of the whole _TIF_RESTOREALL thing too, or does PPC32 want to keep it? > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > V1 -> V2: Add benchmark data > V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify > interrupt_exit_user_prepare changes in summary. > --- > arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++------------- > arch/powerpc/kernel/interrupt_64.S | 9 ++------- > 2 files changed, 10 insertions(+), 20 deletions(-) > > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > index a3b51441b039..038e42fb2182 100644 > --- a/arch/powerpc/kernel/exceptions-64s.S > +++ b/arch/powerpc/kernel/exceptions-64s.S > @@ -502,6 +502,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 > @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > ld r10,IAREA+EX_R10(r13) > std r9,GPR9(r1) > std r10,GPR10(r1) > + ZEROIZE_GPRS(9, 10) You use 9/10 right afterwards, this'd have to move down to where you zero r11 at least. > ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */ > ld r10,IAREA+EX_R12(r13) > ld r11,IAREA+EX_R13(r13) > std r9,GPR11(r1) > std r10,GPR12(r1) > std r11,GPR13(r1) > + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */ > + ZEROIZE_GPR(11) Kernel always has to keep r13 so no need to comment that. Keeping r11, is that for those annoying fp_unavailable etc handlers? There's probably not much a user can do with this, given they're set from the MSR. Use can influence some bits of its MSR though. So long as we're being paranoid, you could add an IOPTION to retain r11 only for the handlers that need it, or have them load it from MSR and zero it here. Thanks, Nick > > SAVE_NVGPRS(r1) > + ZEROIZE_NVGPRS() > > .if IDAR > .if IISIDE > @@ -577,8 +582,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 */ > ld r2,PACATOC(r13) /* get kernel TOC into r2 */ > std r9,_LINK(r1) > @@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR) > mtlr r9 > ld r9,_CCR(r1) > mtcr r9 > + REST_NVGPRS(r1) > REST_GPRS(2, 13, r1) > REST_GPR(0, r1) > /* restore original r1. */ > @@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX) > b interrupt_return_srr > > 1: bl do_break > - /* > - * do_break() may have changed the NV GPRS while handling a breakpoint. > - * If so, we need to restore them with their updated values. > - */ > - REST_NVGPRS(r1) > b interrupt_return_srr > > > @@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common) > GEN_COMMON alignment > addi r3,r1,STACK_FRAME_OVERHEAD > bl alignment_exception > - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ > b interrupt_return_srr > > > @@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common) > .Ldo_program_check: > addi r3,r1,STACK_FRAME_OVERHEAD > bl program_check_exception > - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ > b interrupt_return_srr > > > @@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common) > GEN_COMMON emulation_assist > addi r3,r1,STACK_FRAME_OVERHEAD > bl emulation_assist_interrupt > - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ > b interrupt_return_hsrr > > > @@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common) > GEN_COMMON facility_unavailable > addi r3,r1,STACK_FRAME_OVERHEAD > bl facility_unavailable_exception > - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ > b interrupt_return_srr > > > @@ -2485,7 +2482,6 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common) > GEN_COMMON h_facility_unavailable > addi r3,r1,STACK_FRAME_OVERHEAD > bl facility_unavailable_exception > - REST_NVGPRS(r1) /* XXX Shouldn't be necessary in practice */ > b interrupt_return_hsrr > > > @@ -2711,7 +2707,6 @@ EXC_COMMON_BEGIN(altivec_assist_common) > addi r3,r1,STACK_FRAME_OVERHEAD > #ifdef CONFIG_ALTIVEC > bl altivec_assist_exception > - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ > #else > bl unknown_exception > #endif > diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S > index ad302ad93433..f9ee93e3a0d3 100644 > --- a/arch/powerpc/kernel/interrupt_64.S > +++ b/arch/powerpc/kernel/interrupt_64.S > @@ -432,9 +432,6 @@ interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */ > _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) > addi r3,r1,STACK_FRAME_OVERHEAD > bl interrupt_exit_user_prepare > - cmpdi r3,0 > - bne- .Lrestore_nvgprs_\srr > -.Lrestore_nvgprs_\srr\()_cont: > std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */ > #ifdef CONFIG_PPC_BOOK3S > .Linterrupt_return_\srr\()_user_rst_start: > @@ -448,6 +445,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) > stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS > > .Lfast_user_interrupt_return_\srr\(): > + REST_NVGPRS(r1) > #ifdef CONFIG_PPC_BOOK3S > .ifc \srr,srr > lbz r4,PACASRR_VALID(r13) > @@ -517,10 +515,6 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) > b . /* prevent speculative execution */ > .Linterrupt_return_\srr\()_user_rst_end: > > -.Lrestore_nvgprs_\srr\(): > - REST_NVGPRS(r1) > - b .Lrestore_nvgprs_\srr\()_cont > - > #ifdef CONFIG_PPC_BOOK3S > interrupt_return_\srr\()_user_restart: > _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user_restart) > @@ -561,6 +555,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel) > 1: > > .Lfast_kernel_interrupt_return_\srr\(): > + REST_NVGPRS(r1) > cmpdi cr1,r3,0 > #ifdef CONFIG_PPC_BOOK3S > .ifc \srr,srr > -- > 2.34.1
> On 12 Sep 2022, at 10:15 pm, Nicholas Piggin <npiggin@gmail.com> wrote: > > On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote: >> Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all >> other interrupt sources to limit influence of user-space values >> in potential speculation gadgets. The remaining gprs are overwritten by >> entry macros to interrupt handlers, irrespective of whether or not a >> given handler consumes these register values. >> >> Prior to this commit, r14-r31 are restored on a per-interrupt basis at >> exit, but now they are always restored. Remove explicit REST_NVGPRS >> invocations as non-volatiles must now always be restored. 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. > > Ow, my heart :( > > Are we not keeping a CONFIG option to rid ourselves of this vile > performance robbing thing? Are we getting rid of the whole > _TIF_RESTOREALL thing too, or does PPC32 want to keep it? I see no reason not to include a CONFIG option for this mitigation here other than simplicity. Any suggestions for a name? I’m thinking PPC64_SANITIZE_INTERRUPTS. Defaults on Book3E_64, optional on Book3S_64. >> >> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> >> --- >> V1 -> V2: Add benchmark data >> V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify >> interrupt_exit_user_prepare changes in summary. >> --- >> arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++------------- >> arch/powerpc/kernel/interrupt_64.S | 9 ++------- >> 2 files changed, 10 insertions(+), 20 deletions(-) >> >> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S >> index a3b51441b039..038e42fb2182 100644 >> --- a/arch/powerpc/kernel/exceptions-64s.S >> +++ b/arch/powerpc/kernel/exceptions-64s.S >> @@ -502,6 +502,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 >> @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) >> ld r10,IAREA+EX_R10(r13) >> std r9,GPR9(r1) >> std r10,GPR10(r1) >> + ZEROIZE_GPRS(9, 10) > > You use 9/10 right afterwards, this'd have to move down to where > you zero r11 at least. > >> ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */ >> ld r10,IAREA+EX_R12(r13) >> ld r11,IAREA+EX_R13(r13) >> std r9,GPR11(r1) >> std r10,GPR12(r1) >> std r11,GPR13(r1) >> + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */ >> + ZEROIZE_GPR(11) > > Kernel always has to keep r13 so no need to comment that. Keeping r11, > is that for those annoying fp_unavailable etc handlers? > > There's probably not much a user can do with this, given they're set > from the MSR. Use can influence some bits of its MSR though. So long > as we're being paranoid, you could add an IOPTION to retain r11 only for > the handlers that need it, or have them load it from MSR and zero it > here. Good suggestion. Presume you’re referring to r12 here. I might go the IOPTION route. > > Thanks, > Nick > >> >> SAVE_NVGPRS(r1) >> + ZEROIZE_NVGPRS() >> >> .if IDAR >> .if IISIDE >> @@ -577,8 +582,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 */ >> ld r2,PACATOC(r13) /* get kernel TOC into r2 */ >> std r9,_LINK(r1) >> @@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR) >> mtlr r9 >> ld r9,_CCR(r1) >> mtcr r9 >> + REST_NVGPRS(r1) >> REST_GPRS(2, 13, r1) >> REST_GPR(0, r1) >> /* restore original r1. */ >> @@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX) >> b interrupt_return_srr >> >> 1: bl do_break >> - /* >> - * do_break() may have changed the NV GPRS while handling a breakpoint. >> - * If so, we need to restore them with their updated values. >> - */ >> - REST_NVGPRS(r1) >> b interrupt_return_srr >> >> >> @@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common) >> GEN_COMMON alignment >> addi r3,r1,STACK_FRAME_OVERHEAD >> bl alignment_exception >> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ >> b interrupt_return_srr >> >> >> @@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common) >> .Ldo_program_check: >> addi r3,r1,STACK_FRAME_OVERHEAD >> bl program_check_exception >> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ >> b interrupt_return_srr >> >> >> @@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common) >> GEN_COMMON emulation_assist >> addi r3,r1,STACK_FRAME_OVERHEAD >> bl emulation_assist_interrupt >> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ >> b interrupt_return_hsrr >> >> >> @@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common) >> GEN_COMMON facility_unavailable >> addi r3,r1,STACK_FRAME_OVERHEAD >> bl facility_unavailable_exception >> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ >> b interrupt_return_srr >> >> >> @@ -2485,7 +2482,6 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common) >> GEN_COMMON h_facility_unavailable >> addi r3,r1,STACK_FRAME_OVERHEAD >> bl facility_unavailable_exception >> - REST_NVGPRS(r1) /* XXX Shouldn't be necessary in practice */ >> b interrupt_return_hsrr >> >> >> @@ -2711,7 +2707,6 @@ EXC_COMMON_BEGIN(altivec_assist_common) >> addi r3,r1,STACK_FRAME_OVERHEAD >> #ifdef CONFIG_ALTIVEC >> bl altivec_assist_exception >> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ >> #else >> bl unknown_exception >> #endif >> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S >> index ad302ad93433..f9ee93e3a0d3 100644 >> --- a/arch/powerpc/kernel/interrupt_64.S >> +++ b/arch/powerpc/kernel/interrupt_64.S >> @@ -432,9 +432,6 @@ interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */ >> _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) >> addi r3,r1,STACK_FRAME_OVERHEAD >> bl interrupt_exit_user_prepare >> - cmpdi r3,0 >> - bne- .Lrestore_nvgprs_\srr >> -.Lrestore_nvgprs_\srr\()_cont: >> std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */ >> #ifdef CONFIG_PPC_BOOK3S >> .Linterrupt_return_\srr\()_user_rst_start: >> @@ -448,6 +445,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) >> stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS >> >> .Lfast_user_interrupt_return_\srr\(): >> + REST_NVGPRS(r1) >> #ifdef CONFIG_PPC_BOOK3S >> .ifc \srr,srr >> lbz r4,PACASRR_VALID(r13) >> @@ -517,10 +515,6 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) >> b . /* prevent speculative execution */ >> .Linterrupt_return_\srr\()_user_rst_end: >> >> -.Lrestore_nvgprs_\srr\(): >> - REST_NVGPRS(r1) >> - b .Lrestore_nvgprs_\srr\()_cont >> - >> #ifdef CONFIG_PPC_BOOK3S >> interrupt_return_\srr\()_user_restart: >> _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user_restart) >> @@ -561,6 +555,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel) >> 1: >> >> .Lfast_kernel_interrupt_return_\srr\(): >> + REST_NVGPRS(r1) >> cmpdi cr1,r3,0 >> #ifdef CONFIG_PPC_BOOK3S >> .ifc \srr,srr >> -- >> 2.34.1
On Thu Sep 15, 2022 at 4:55 PM AEST, Rohan McLure wrote: > > > > On 12 Sep 2022, at 10:15 pm, Nicholas Piggin <npiggin@gmail.com> wrote: > > > > On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote: > >> Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all > >> other interrupt sources to limit influence of user-space values > >> in potential speculation gadgets. The remaining gprs are overwritten by > >> entry macros to interrupt handlers, irrespective of whether or not a > >> given handler consumes these register values. > >> > >> Prior to this commit, r14-r31 are restored on a per-interrupt basis at > >> exit, but now they are always restored. Remove explicit REST_NVGPRS > >> invocations as non-volatiles must now always be restored. 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. > > > > Ow, my heart :( > > > > Are we not keeping a CONFIG option to rid ourselves of this vile > > performance robbing thing? Are we getting rid of the whole > > _TIF_RESTOREALL thing too, or does PPC32 want to keep it? > > I see no reason not to include a CONFIG option for this > mitigation here other than simplicity. Any suggestions for a name? > I’m thinking PPC64_SANITIZE_INTERRUPTS. Defaults on Book3E_64, optional > on Book3S_64. INTERRUPT_SANITIZE_REGISTERS perhaps? > > >> > >> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > >> --- > >> V1 -> V2: Add benchmark data > >> V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify > >> interrupt_exit_user_prepare changes in summary. > >> --- > >> arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++------------- > >> arch/powerpc/kernel/interrupt_64.S | 9 ++------- > >> 2 files changed, 10 insertions(+), 20 deletions(-) > >> > >> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > >> index a3b51441b039..038e42fb2182 100644 > >> --- a/arch/powerpc/kernel/exceptions-64s.S > >> +++ b/arch/powerpc/kernel/exceptions-64s.S > >> @@ -502,6 +502,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 > >> @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > >> ld r10,IAREA+EX_R10(r13) > >> std r9,GPR9(r1) > >> std r10,GPR10(r1) > >> + ZEROIZE_GPRS(9, 10) > > > > You use 9/10 right afterwards, this'd have to move down to where > > you zero r11 at least. > > > >> ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */ > >> ld r10,IAREA+EX_R12(r13) > >> ld r11,IAREA+EX_R13(r13) > >> std r9,GPR11(r1) > >> std r10,GPR12(r1) > >> std r11,GPR13(r1) > >> + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */ > >> + ZEROIZE_GPR(11) > > > > Kernel always has to keep r13 so no need to comment that. Keeping r11, > > is that for those annoying fp_unavailable etc handlers? > > > > There's probably not much a user can do with this, given they're set > > from the MSR. Use can influence some bits of its MSR though. So long > > as we're being paranoid, you could add an IOPTION to retain r11 only for > > the handlers that need it, or have them load it from MSR and zero it > > here. > > Good suggestion. Presume you’re referring to r12 here. I might go the > IOPTION route. Yeah r12, I think you need it because some of those assembly handlers expect it there to check SRR1 bits. Having an IOPTION is probably good and it documents that quirk of those handlers. That's bitten me before. Thanks, Nick
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S index a3b51441b039..038e42fb2182 100644 --- a/arch/powerpc/kernel/exceptions-64s.S +++ b/arch/powerpc/kernel/exceptions-64s.S @@ -502,6 +502,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 @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) ld r10,IAREA+EX_R10(r13) std r9,GPR9(r1) std r10,GPR10(r1) + ZEROIZE_GPRS(9, 10) ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */ ld r10,IAREA+EX_R12(r13) ld r11,IAREA+EX_R13(r13) std r9,GPR11(r1) std r10,GPR12(r1) std r11,GPR13(r1) + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */ + ZEROIZE_GPR(11) SAVE_NVGPRS(r1) + ZEROIZE_NVGPRS() .if IDAR .if IISIDE @@ -577,8 +582,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 */ ld r2,PACATOC(r13) /* get kernel TOC into r2 */ std r9,_LINK(r1) @@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR) mtlr r9 ld r9,_CCR(r1) mtcr r9 + REST_NVGPRS(r1) REST_GPRS(2, 13, r1) REST_GPR(0, r1) /* restore original r1. */ @@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX) b interrupt_return_srr 1: bl do_break - /* - * do_break() may have changed the NV GPRS while handling a breakpoint. - * If so, we need to restore them with their updated values. - */ - REST_NVGPRS(r1) b interrupt_return_srr @@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common) GEN_COMMON alignment addi r3,r1,STACK_FRAME_OVERHEAD bl alignment_exception - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ b interrupt_return_srr @@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common) .Ldo_program_check: addi r3,r1,STACK_FRAME_OVERHEAD bl program_check_exception - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ b interrupt_return_srr @@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common) GEN_COMMON emulation_assist addi r3,r1,STACK_FRAME_OVERHEAD bl emulation_assist_interrupt - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ b interrupt_return_hsrr @@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common) GEN_COMMON facility_unavailable addi r3,r1,STACK_FRAME_OVERHEAD bl facility_unavailable_exception - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ b interrupt_return_srr @@ -2485,7 +2482,6 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common) GEN_COMMON h_facility_unavailable addi r3,r1,STACK_FRAME_OVERHEAD bl facility_unavailable_exception - REST_NVGPRS(r1) /* XXX Shouldn't be necessary in practice */ b interrupt_return_hsrr @@ -2711,7 +2707,6 @@ EXC_COMMON_BEGIN(altivec_assist_common) addi r3,r1,STACK_FRAME_OVERHEAD #ifdef CONFIG_ALTIVEC bl altivec_assist_exception - REST_NVGPRS(r1) /* instruction emulation may change GPRs */ #else bl unknown_exception #endif diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S index ad302ad93433..f9ee93e3a0d3 100644 --- a/arch/powerpc/kernel/interrupt_64.S +++ b/arch/powerpc/kernel/interrupt_64.S @@ -432,9 +432,6 @@ interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) addi r3,r1,STACK_FRAME_OVERHEAD bl interrupt_exit_user_prepare - cmpdi r3,0 - bne- .Lrestore_nvgprs_\srr -.Lrestore_nvgprs_\srr\()_cont: std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */ #ifdef CONFIG_PPC_BOOK3S .Linterrupt_return_\srr\()_user_rst_start: @@ -448,6 +445,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS .Lfast_user_interrupt_return_\srr\(): + REST_NVGPRS(r1) #ifdef CONFIG_PPC_BOOK3S .ifc \srr,srr lbz r4,PACASRR_VALID(r13) @@ -517,10 +515,6 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) b . /* prevent speculative execution */ .Linterrupt_return_\srr\()_user_rst_end: -.Lrestore_nvgprs_\srr\(): - REST_NVGPRS(r1) - b .Lrestore_nvgprs_\srr\()_cont - #ifdef CONFIG_PPC_BOOK3S interrupt_return_\srr\()_user_restart: _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user_restart) @@ -561,6 +555,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel) 1: .Lfast_kernel_interrupt_return_\srr\(): + REST_NVGPRS(r1) cmpdi cr1,r3,0 #ifdef CONFIG_PPC_BOOK3S .ifc \srr,srr
Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all other interrupt sources to limit influence of user-space values in potential speculation gadgets. The remaining gprs are overwritten by entry macros to interrupt handlers, irrespective of whether or not a given handler consumes these register values. Prior to this commit, r14-r31 are restored on a per-interrupt basis at exit, but now they are always restored. Remove explicit REST_NVGPRS invocations as non-volatiles must now always be restored. 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. Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> --- V1 -> V2: Add benchmark data V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify interrupt_exit_user_prepare changes in summary. --- arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++------------- arch/powerpc/kernel/interrupt_64.S | 9 ++------- 2 files changed, 10 insertions(+), 20 deletions(-)