| Message ID | 20220819033806.162054-15-rmclure@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Syscall wrapper and register clearing | expand |
Le 19/08/2022 à 05:38, Rohan McLure a écrit : > 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 > --- > arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S > index 0178aeba3820..d9625113c7a5 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 */ > std r3,GPR3(r1) > std r4,GPR4(r1) > std r5,GPR5(r1) > @@ -109,6 +109,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > * but this is the best we can do. > */ > > + /* > + * Zero user registers to prevent influencing speculative execution > + * state of kernel code. > + */ > + NULLIFY_GPRS(5, 12) Macro name has changed. > + NULLIFY_NVGPRS() Why clearing non volatile GPRs ? They are supposed to be callee saved so I can't see how they could be used for speculation. Do you have any exemple ? > + > /* Calling convention has r3 = orig r0, r4 = regs */ > mr r3,r0 > bl system_call_exception > @@ -139,6 +146,7 @@ BEGIN_FTR_SECTION > HMT_MEDIUM_LOW > END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > > + REST_NVGPRS(r1) What is the link between this change and the description in the commit message ? > cmpdi r3,0 > bne .Lsyscall_vectored_\name\()_restore_regs > > @@ -181,7 +189,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > ld r4,_LINK(r1) > ld r5,_XER(r1) > > - REST_NVGPRS(r1) > ld r0,GPR0(r1) > mtcr r2 > mtctr r3 > @@ -249,7 +256,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 */ > std r3,GPR3(r1) > std r4,GPR4(r1) > std r5,GPR5(r1) > @@ -300,6 +307,13 @@ END_BTB_FLUSH_SECTION > wrteei 1 > #endif > > + /* > + * Zero user registers to prevent influencing speculative execution > + * state of kernel code. > + */ > + NULLIFY_GPRS(5, 12) > + NULLIFY_NVGPRS() > + Name has changed. > /* Calling convention has r3 = orig r0, r4 = regs */ > mr r3,r0 > bl system_call_exception > @@ -342,6 +356,7 @@ BEGIN_FTR_SECTION > stdcx. r0,0,r1 /* to clear the reservation */ > END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) > > + REST_NVGPRS(r1) Same question. > cmpdi r3,0 > bne .Lsyscall_restore_regs > /* Zero volatile regs that may contain sensitive kernel data */ > @@ -377,7 +392,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 > ld r0,GPR0(r1)
> On 19 Aug 2022, at 4:52 pm, Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 19/08/2022 à 05:38, Rohan McLure a écrit : >> 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 >> --- >> arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S >> index 0178aeba3820..d9625113c7a5 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 */ >> std r3,GPR3(r1) >> std r4,GPR4(r1) >> std r5,GPR5(r1) >> @@ -109,6 +109,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) >> * but this is the best we can do. >> */ >> >> + /* >> + * Zero user registers to prevent influencing speculative execution >> + * state of kernel code. >> + */ >> + NULLIFY_GPRS(5, 12) > > Macro name has changed. > >> + NULLIFY_NVGPRS() > > Why clearing non volatile GPRs ? They are supposed to be callee saved so > I can't see how they could be used for speculation. Do you have any > exemple ? > >> + >> /* Calling convention has r3 = orig r0, r4 = regs */ >> mr r3,r0 >> bl system_call_exception >> @@ -139,6 +146,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 >> >> @@ -181,7 +189,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) >> ld r4,_LINK(r1) >> ld r5,_XER(r1) >> >> - REST_NVGPRS(r1) >> ld r0,GPR0(r1) >> mtcr r2 >> mtctr r3 >> @@ -249,7 +256,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 */ >> std r3,GPR3(r1) >> std r4,GPR4(r1) >> std r5,GPR5(r1) >> @@ -300,6 +307,13 @@ END_BTB_FLUSH_SECTION >> wrteei 1 >> #endif >> >> + /* >> + * Zero user registers to prevent influencing speculative execution >> + * state of kernel code. >> + */ >> + NULLIFY_GPRS(5, 12) >> + NULLIFY_NVGPRS() >> + > > Name has changed. > >> /* Calling convention has r3 = orig r0, r4 = regs */ >> mr r3,r0 >> bl system_call_exception >> @@ -342,6 +356,7 @@ BEGIN_FTR_SECTION >> stdcx. r0,0,r1 /* to clear the reservation */ >> END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) >> >> + REST_NVGPRS(r1) > > Same question. > >> cmpdi r3,0 >> bne .Lsyscall_restore_regs >> /* Zero volatile regs that may contain sensitive kernel data */ >> @@ -377,7 +392,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 >> ld r0,GPR0(r1) > Why clearing non volatile GPRs ? They are supposed to be callee saved so > I can't see how they could be used for speculation. Do you have any > exemple ? Even though non-volatiles will be callee-saved subject to ABI in the syscall handler prologue, it is still conceivable that a syscall handler will leave a register uninitialised on one branch outcome, assigning that register in the other. On speculative processors, there remains the possibility for untaken branches to be executed microarchitecturally (by mistraining the branch predictor or otherwise), whereby the microarchitectural effects of the execution present a side-channel. Such a hardening measure removes the burden for developers to prove that toolchains will never emit code with these properties in code reachable via a syscall. For what it’s worth, precedent already exists in arm64 and x86. Links: https://lore.kernel.org/linux-arm-kernel/20180702110415.10465-13-mark.rutland@arm.com/ <https://lore.kernel.org/linux-arm-kernel/20180702110415.10465-13-mark.rutland@arm.com/> https://lore.kernel.org/lkml/20180405095307.3730-8-linux@dominikbrodowski.net/ <https://lore.kernel.org/lkml/20180405095307.3730-8-linux@dominikbrodowski.net/> > What is the link between this change and the description in the commit > message ? I’ll include a detail on why we must restore nvgprs in both cases in the commit message. Thanks for pointing that out.
diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S index 0178aeba3820..d9625113c7a5 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 */ std r3,GPR3(r1) std r4,GPR4(r1) std r5,GPR5(r1) @@ -109,6 +109,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) * but this is the best we can do. */ + /* + * Zero user registers to prevent influencing speculative execution + * state of kernel code. + */ + NULLIFY_GPRS(5, 12) + NULLIFY_NVGPRS() + /* Calling convention has r3 = orig r0, r4 = regs */ mr r3,r0 bl system_call_exception @@ -139,6 +146,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 @@ -181,7 +189,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) ld r4,_LINK(r1) ld r5,_XER(r1) - REST_NVGPRS(r1) ld r0,GPR0(r1) mtcr r2 mtctr r3 @@ -249,7 +256,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 */ std r3,GPR3(r1) std r4,GPR4(r1) std r5,GPR5(r1) @@ -300,6 +307,13 @@ END_BTB_FLUSH_SECTION wrteei 1 #endif + /* + * Zero user registers to prevent influencing speculative execution + * state of kernel code. + */ + NULLIFY_GPRS(5, 12) + NULLIFY_NVGPRS() + /* Calling convention has r3 = orig r0, r4 = regs */ mr r3,r0 bl system_call_exception @@ -342,6 +356,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 */ @@ -377,7 +392,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 ld r0,GPR0(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 --- arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)