[PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S
Rohan McLure
rmclure at linux.ibm.com
Thu Sep 15 16:55:01 AEST 2022
> On 12 Sep 2022, at 10:15 pm, Nicholas Piggin <npiggin at 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 at 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
More information about the Linuxppc-dev
mailing list