* [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig
@ 2022-11-07 3:31 Rohan McLure
2022-11-07 3:32 ` [PATCH v2 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Rohan McLure @ 2022-11-07 3:31 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, Nicholas Piggin
Add Kconfig option for enabling clearing of registers on arrival in an
interrupt handler. This reduces the speculation influence of registers
on kernel internals. The option will be consumed by 64-bit systems that
feature speculation and wish to implement this mitigation.
This patch only introduces the Kconfig option, no actual mitigations.
The primary overhead of this mitigation lies in an increased number of
registers that must be saved and restored by interrupt handlers on
Book3S systems. Enable by default on Book3E systems, which prior to
this patch eagerly save and restore register state, meaning that the
mitigation when implemented will have minimal overhead.
Acked-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
Resubmitting patches as their own series after v6 partially merged:
Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/
---
arch/powerpc/Kconfig | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2ca5418457ed..9d3d20c6f365 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -529,6 +529,15 @@ config HOTPLUG_CPU
Say N if you are unsure.
+config INTERRUPT_SANITIZE_REGISTERS
+ bool "Clear gprs on interrupt arrival"
+ depends on PPC64 && ARCH_HAS_SYSCALL_WRAPPER
+ default PPC_BOOK3E_64
+ help
+ Reduce the influence of user register state on interrupt handlers and
+ syscalls through clearing user state from registers before handling
+ the exception.
+
config PPC_QUEUED_SPINLOCKS
bool "Queued spinlocks" if EXPERT
depends on SMP
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S 2022-11-07 3:31 [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure @ 2022-11-07 3:32 ` Rohan McLure 2022-11-28 1:52 ` Nicholas Piggin 2022-11-07 3:32 ` [PATCH v2 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure ` (3 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Rohan McLure @ 2022-11-07 3:32 UTC (permalink / raw) To: linuxppc-dev; +Cc: Rohan McLure Zero 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. Zero 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. 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. Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> --- Resubmitting patches as their own series after v6 partially merged: Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/ v2: REST_NVGPRS should be conditional on mitigation in scv handler. Fix improper multi-line preprocessor macro in interrupt_64.S --- arch/powerpc/kernel/exceptions-64s.S | 47 +++++++++++++++++++++----- arch/powerpc/kernel/interrupt_64.S | 36 ++++++++++++++++++++ 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S index 651c36b056bd..0605018762d1 100644 --- a/arch/powerpc/kernel/exceptions-64s.S +++ b/arch/powerpc/kernel/exceptions-64s.S @@ -21,6 +21,19 @@ #include <asm/feature-fixups.h> #include <asm/kup.h> +/* + * macros for handling user register sanitisation + */ +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS +#define SANITIZE_ZEROIZE_NVGPRS() ZEROIZE_NVGPRS() +#define SANITIZE_RESTORE_NVGPRS() REST_NVGPRS(r1) +#define HANDLER_RESTORE_NVGPRS() +#else +#define SANITIZE_ZEROIZE_NVGPRS() +#define SANITIZE_RESTORE_NVGPRS() +#define HANDLER_RESTORE_NVGPRS() REST_NVGPRS(r1) +#endif /* CONFIG_INTERRUPT_SANITIZE_REGISTERS */ + /* * Following are fixed section helper macros. * @@ -111,6 +124,7 @@ name: #define ISTACK .L_ISTACK_\name\() /* Set regular kernel stack */ #define __ISTACK(name) .L_ISTACK_ ## name #define IKUAP .L_IKUAP_\name\() /* Do KUAP lock */ +#define IMSR_R12 .L_IMSR_R12_\name\() /* Assumes MSR saved to r12 */ #define INT_DEFINE_BEGIN(n) \ .macro int_define_ ## n name @@ -176,6 +190,9 @@ do_define_int n .ifndef IKUAP IKUAP=1 .endif + .ifndef IMSR_R12 + IMSR_R12=0 + .endif .endm /* @@ -502,6 +519,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 @@ -544,8 +562,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 SAVE_NVGPRS(r1) + SANITIZE_ZEROIZE_NVGPRS() .if IDAR .if IISIDE @@ -577,8 +601,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) @@ -696,6 +720,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR) mtlr r9 ld r9,_CCR(r1) mtcr r9 + SANITIZE_RESTORE_NVGPRS() REST_GPRS(2, 13, r1) REST_GPR(0, r1) /* restore original r1. */ @@ -1441,7 +1466,7 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX) * 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) + HANDLER_RESTORE_NVGPRS() b interrupt_return_srr @@ -1667,7 +1692,7 @@ 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 */ + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ b interrupt_return_srr @@ -1733,7 +1758,7 @@ 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 */ + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ b interrupt_return_srr @@ -1751,6 +1776,7 @@ INT_DEFINE_BEGIN(fp_unavailable) #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE IKVM_REAL=1 #endif + IMSR_R12=1 INT_DEFINE_END(fp_unavailable) EXC_REAL_BEGIN(fp_unavailable, 0x800, 0x100) @@ -2164,7 +2190,7 @@ 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 */ + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ b interrupt_return_hsrr @@ -2384,6 +2410,7 @@ INT_DEFINE_BEGIN(altivec_unavailable) #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE IKVM_REAL=1 #endif + IMSR_R12=1 INT_DEFINE_END(altivec_unavailable) EXC_REAL_BEGIN(altivec_unavailable, 0xf20, 0x20) @@ -2433,6 +2460,7 @@ INT_DEFINE_BEGIN(vsx_unavailable) #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE IKVM_REAL=1 #endif + IMSR_R12=1 INT_DEFINE_END(vsx_unavailable) EXC_REAL_BEGIN(vsx_unavailable, 0xf40, 0x20) @@ -2494,7 +2522,7 @@ 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 */ + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ b interrupt_return_srr @@ -2522,7 +2550,8 @@ 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 */ + /* XXX Shouldn't be necessary in practice */ + HANDLER_RESTORE_NVGPRS() b interrupt_return_hsrr @@ -2748,7 +2777,7 @@ 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 */ + HANDLER_RESTORE_NVGPRS() /* 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 a019ed6fc839..a262fe6964f3 100644 --- a/arch/powerpc/kernel/interrupt_64.S +++ b/arch/powerpc/kernel/interrupt_64.S @@ -13,6 +13,20 @@ #include <asm/ppc_asm.h> #include <asm/ptrace.h> +/* + * macros for handling user register sanitisation + */ +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS +#define SANITIZE_ZEROIZE_SYSCALL_GPRS() \ + ZEROIZE_GPR(0); \ + ZEROIZE_GPRS(5, 12); \ + ZEROIZE_NVGPRS() +#define SANITIZE_RESTORE_NVGPRS() REST_NVGPRS(r1) +#else +#define SANITIZE_ZEROIZE_SYSCALL_GPRS() +#define SANITIZE_RESTORE_NVGPRS() +#endif /* CONFIG_INTERRUPT_SANITIZE_REGISTERS */ + .align 7 .macro DEBUG_SRR_VALID srr @@ -96,6 +110,11 @@ 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. + */ + SANITIZE_ZEROIZE_SYSCALL_GPRS() bl system_call_exception .Lsyscall_vectored_\name\()_exit: @@ -124,6 +143,7 @@ BEGIN_FTR_SECTION HMT_MEDIUM_LOW END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) + SANITIZE_RESTORE_NVGPRS() cmpdi r3,0 bne .Lsyscall_vectored_\name\()_restore_regs @@ -159,7 +179,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) ld r4,_LINK(r1) ld r5,_XER(r1) +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS REST_NVGPRS(r1) +#endif REST_GPR(0, r1) mtcr r2 mtctr r3 @@ -275,6 +297,11 @@ END_BTB_FLUSH_SECTION wrteei 1 #endif + /* + * Zero user registers to prevent influencing speculative execution + * state of kernel code. + */ + SANITIZE_ZEROIZE_SYSCALL_GPRS() bl system_call_exception .Lsyscall_exit: @@ -315,6 +342,7 @@ BEGIN_FTR_SECTION stdcx. r0,0,r1 /* to clear the reservation */ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) + SANITIZE_RESTORE_NVGPRS() cmpdi r3,0 bne .Lsyscall_restore_regs /* Zero volatile regs that may contain sensitive kernel data */ @@ -342,7 +370,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) .Lsyscall_restore_regs: ld r3,_CTR(r1) ld r4,_XER(r1) +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS REST_NVGPRS(r1) +#endif mtctr r3 mtspr SPRN_XER,r4 REST_GPR(0, r1) @@ -408,9 +438,11 @@ 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 +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS cmpdi r3,0 bne- .Lrestore_nvgprs_\srr .Lrestore_nvgprs_\srr\()_cont: +#endif std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */ #ifdef CONFIG_PPC_BOOK3S .Linterrupt_return_\srr\()_user_rst_start: @@ -424,6 +456,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS .Lfast_user_interrupt_return_\srr\(): + SANITIZE_RESTORE_NVGPRS() #ifdef CONFIG_PPC_BOOK3S .ifc \srr,srr lbz r4,PACASRR_VALID(r13) @@ -493,9 +526,11 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) b . /* prevent speculative execution */ .Linterrupt_return_\srr\()_user_rst_end: +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS .Lrestore_nvgprs_\srr\(): REST_NVGPRS(r1) b .Lrestore_nvgprs_\srr\()_cont +#endif #ifdef CONFIG_PPC_BOOK3S interrupt_return_\srr\()_user_restart: @@ -585,6 +620,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel) stb r11,PACAIRQHAPPENED(r13) // clear the possible HARD_DIS .Lfast_kernel_interrupt_return_\srr\(): + SANITIZE_RESTORE_NVGPRS() cmpdi cr1,r3,0 #ifdef CONFIG_PPC_BOOK3S .ifc \srr,srr -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S 2022-11-07 3:32 ` [PATCH v2 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure @ 2022-11-28 1:52 ` Nicholas Piggin 0 siblings, 0 replies; 11+ messages in thread From: Nicholas Piggin @ 2022-11-28 1:52 UTC (permalink / raw) To: Rohan McLure, linuxppc-dev On Mon Nov 7, 2022 at 1:32 PM AEST, Rohan McLure wrote: > Zero 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. > > Zero 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. > > 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. I think it looks good. You could put those macros into a .h file shared by exceptions-64s.S and interrupt_64.S. Also interrupt_64.S could use the HANDLER_RESTORE_NVGPRS macro to kill a few ifdefs I think? The IMSR_R12 change *could* be done in a separate patch, if you're doing another spin... sorry for the late feedback. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > Resubmitting patches as their own series after v6 partially merged: > Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/ > > v2: REST_NVGPRS should be conditional on mitigation in scv handler. Fix > improper multi-line preprocessor macro in interrupt_64.S > --- > arch/powerpc/kernel/exceptions-64s.S | 47 +++++++++++++++++++++----- > arch/powerpc/kernel/interrupt_64.S | 36 ++++++++++++++++++++ > 2 files changed, 74 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > index 651c36b056bd..0605018762d1 100644 > --- a/arch/powerpc/kernel/exceptions-64s.S > +++ b/arch/powerpc/kernel/exceptions-64s.S > @@ -21,6 +21,19 @@ > #include <asm/feature-fixups.h> > #include <asm/kup.h> > > +/* > + * macros for handling user register sanitisation > + */ > +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS > +#define SANITIZE_ZEROIZE_NVGPRS() ZEROIZE_NVGPRS() > +#define SANITIZE_RESTORE_NVGPRS() REST_NVGPRS(r1) > +#define HANDLER_RESTORE_NVGPRS() > +#else > +#define SANITIZE_ZEROIZE_NVGPRS() > +#define SANITIZE_RESTORE_NVGPRS() > +#define HANDLER_RESTORE_NVGPRS() REST_NVGPRS(r1) > +#endif /* CONFIG_INTERRUPT_SANITIZE_REGISTERS */ > + > /* > * Following are fixed section helper macros. > * > @@ -111,6 +124,7 @@ name: > #define ISTACK .L_ISTACK_\name\() /* Set regular kernel stack */ > #define __ISTACK(name) .L_ISTACK_ ## name > #define IKUAP .L_IKUAP_\name\() /* Do KUAP lock */ > +#define IMSR_R12 .L_IMSR_R12_\name\() /* Assumes MSR saved to r12 */ > > #define INT_DEFINE_BEGIN(n) \ > .macro int_define_ ## n name > @@ -176,6 +190,9 @@ do_define_int n > .ifndef IKUAP > IKUAP=1 > .endif > + .ifndef IMSR_R12 > + IMSR_R12=0 > + .endif > .endm > > /* > @@ -502,6 +519,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 > @@ -544,8 +562,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 > > SAVE_NVGPRS(r1) > + SANITIZE_ZEROIZE_NVGPRS() > > .if IDAR > .if IISIDE > @@ -577,8 +601,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) > @@ -696,6 +720,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR) > mtlr r9 > ld r9,_CCR(r1) > mtcr r9 > + SANITIZE_RESTORE_NVGPRS() > REST_GPRS(2, 13, r1) > REST_GPR(0, r1) > /* restore original r1. */ > @@ -1441,7 +1466,7 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX) > * 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) > + HANDLER_RESTORE_NVGPRS() > b interrupt_return_srr > > > @@ -1667,7 +1692,7 @@ 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 */ > + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ > b interrupt_return_srr > > > @@ -1733,7 +1758,7 @@ 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 */ > + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ > b interrupt_return_srr > > > @@ -1751,6 +1776,7 @@ INT_DEFINE_BEGIN(fp_unavailable) > #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE > IKVM_REAL=1 > #endif > + IMSR_R12=1 > INT_DEFINE_END(fp_unavailable) > > EXC_REAL_BEGIN(fp_unavailable, 0x800, 0x100) > @@ -2164,7 +2190,7 @@ 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 */ > + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ > b interrupt_return_hsrr > > > @@ -2384,6 +2410,7 @@ INT_DEFINE_BEGIN(altivec_unavailable) > #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE > IKVM_REAL=1 > #endif > + IMSR_R12=1 > INT_DEFINE_END(altivec_unavailable) > > EXC_REAL_BEGIN(altivec_unavailable, 0xf20, 0x20) > @@ -2433,6 +2460,7 @@ INT_DEFINE_BEGIN(vsx_unavailable) > #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE > IKVM_REAL=1 > #endif > + IMSR_R12=1 > INT_DEFINE_END(vsx_unavailable) > > EXC_REAL_BEGIN(vsx_unavailable, 0xf40, 0x20) > @@ -2494,7 +2522,7 @@ 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 */ > + HANDLER_RESTORE_NVGPRS() /* instruction emulation may change GPRs */ > b interrupt_return_srr > > > @@ -2522,7 +2550,8 @@ 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 */ > + /* XXX Shouldn't be necessary in practice */ > + HANDLER_RESTORE_NVGPRS() > b interrupt_return_hsrr > > > @@ -2748,7 +2777,7 @@ 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 */ > + HANDLER_RESTORE_NVGPRS() /* 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 a019ed6fc839..a262fe6964f3 100644 > --- a/arch/powerpc/kernel/interrupt_64.S > +++ b/arch/powerpc/kernel/interrupt_64.S > @@ -13,6 +13,20 @@ > #include <asm/ppc_asm.h> > #include <asm/ptrace.h> > > +/* > + * macros for handling user register sanitisation > + */ > +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS > +#define SANITIZE_ZEROIZE_SYSCALL_GPRS() \ > + ZEROIZE_GPR(0); \ > + ZEROIZE_GPRS(5, 12); \ > + ZEROIZE_NVGPRS() > +#define SANITIZE_RESTORE_NVGPRS() REST_NVGPRS(r1) > +#else > +#define SANITIZE_ZEROIZE_SYSCALL_GPRS() > +#define SANITIZE_RESTORE_NVGPRS() > +#endif /* CONFIG_INTERRUPT_SANITIZE_REGISTERS */ > + > .align 7 > > .macro DEBUG_SRR_VALID srr > @@ -96,6 +110,11 @@ 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. > + */ > + SANITIZE_ZEROIZE_SYSCALL_GPRS() > bl system_call_exception > > .Lsyscall_vectored_\name\()_exit: > @@ -124,6 +143,7 @@ BEGIN_FTR_SECTION > HMT_MEDIUM_LOW > END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > > + SANITIZE_RESTORE_NVGPRS() > cmpdi r3,0 > bne .Lsyscall_vectored_\name\()_restore_regs > > @@ -159,7 +179,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > ld r4,_LINK(r1) > ld r5,_XER(r1) > > +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS > REST_NVGPRS(r1) > +#endif > REST_GPR(0, r1) > mtcr r2 > mtctr r3 > @@ -275,6 +297,11 @@ END_BTB_FLUSH_SECTION > wrteei 1 > #endif > > + /* > + * Zero user registers to prevent influencing speculative execution > + * state of kernel code. > + */ > + SANITIZE_ZEROIZE_SYSCALL_GPRS() > bl system_call_exception > > .Lsyscall_exit: > @@ -315,6 +342,7 @@ BEGIN_FTR_SECTION > stdcx. r0,0,r1 /* to clear the reservation */ > END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) > > + SANITIZE_RESTORE_NVGPRS() > cmpdi r3,0 > bne .Lsyscall_restore_regs > /* Zero volatile regs that may contain sensitive kernel data */ > @@ -342,7 +370,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > .Lsyscall_restore_regs: > ld r3,_CTR(r1) > ld r4,_XER(r1) > +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS > REST_NVGPRS(r1) > +#endif > mtctr r3 > mtspr SPRN_XER,r4 > REST_GPR(0, r1) > @@ -408,9 +438,11 @@ 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 > +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS > cmpdi r3,0 > bne- .Lrestore_nvgprs_\srr > .Lrestore_nvgprs_\srr\()_cont: > +#endif > std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */ > #ifdef CONFIG_PPC_BOOK3S > .Linterrupt_return_\srr\()_user_rst_start: > @@ -424,6 +456,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user) > stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS > > .Lfast_user_interrupt_return_\srr\(): > + SANITIZE_RESTORE_NVGPRS() > #ifdef CONFIG_PPC_BOOK3S > .ifc \srr,srr > lbz r4,PACASRR_VALID(r13) > @@ -493,9 +526,11 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS) > b . /* prevent speculative execution */ > .Linterrupt_return_\srr\()_user_rst_end: > > +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS > .Lrestore_nvgprs_\srr\(): > REST_NVGPRS(r1) > b .Lrestore_nvgprs_\srr\()_cont > +#endif > > #ifdef CONFIG_PPC_BOOK3S > interrupt_return_\srr\()_user_restart: > @@ -585,6 +620,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel) > stb r11,PACAIRQHAPPENED(r13) // clear the possible HARD_DIS > > .Lfast_kernel_interrupt_return_\srr\(): > + SANITIZE_RESTORE_NVGPRS() > cmpdi cr1,r3,0 > #ifdef CONFIG_PPC_BOOK3S > .ifc \srr,srr > -- > 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E 2022-11-07 3:31 [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure 2022-11-07 3:32 ` [PATCH v2 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure @ 2022-11-07 3:32 ` Rohan McLure 2022-11-28 2:02 ` Nicholas Piggin 2022-11-07 3:32 ` [PATCH v2 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries Rohan McLure ` (2 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Rohan McLure @ 2022-11-07 3:32 UTC (permalink / raw) To: linuxppc-dev; +Cc: Rohan McLure Zero GPRS r14-r31 on entry into the kernel for interrupt sources to limit influence of user-space values in potential speculation gadgets. Prior to this commit, all other GPRS are reassigned during the common prologue to interrupt handlers and so need not be zeroised explicitly. This may be done safely, without loss of register state prior to the interrupt, as the common prologue saves the initial values of non-volatiles, which are unconditionally restored in interrupt_64.S. Mitigation defaults to enabled by INTERRUPT_SANITIZE_REGISTERS. Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> --- Resubmitting patches as their own series after v6 partially merged: Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/ --- arch/powerpc/kernel/exceptions-64e.S | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S index 2f68fb2ee4fc..91d8019123c2 100644 --- a/arch/powerpc/kernel/exceptions-64e.S +++ b/arch/powerpc/kernel/exceptions-64e.S @@ -358,6 +358,11 @@ ret_from_mc_except: std r14,PACA_EXMC+EX_R14(r13); \ std r15,PACA_EXMC+EX_R15(r13) +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS +#define SANITIZE_ZEROIZE_NVGPRS() ZEROIZE_NVGPRS() +#else +#define SANITIZE_ZEROIZE_NVGPRS() +#endif /* Core exception code for all exceptions except TLB misses. */ #define EXCEPTION_COMMON_LVL(n, scratch, excf) \ @@ -394,7 +399,8 @@ exc_##n##_common: \ std r12,STACK_FRAME_OVERHEAD-16(r1); /* mark the frame */ \ std r3,_TRAP(r1); /* set trap number */ \ std r0,RESULT(r1); /* clear regs->result */ \ - SAVE_NVGPRS(r1); + SAVE_NVGPRS(r1); \ + SANITIZE_ZEROIZE_NVGPRS(); /* minimise speculation influence */ #define EXCEPTION_COMMON(n) \ EXCEPTION_COMMON_LVL(n, SPRN_SPRG_GEN_SCRATCH, PACA_EXGEN) -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E 2022-11-07 3:32 ` [PATCH v2 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure @ 2022-11-28 2:02 ` Nicholas Piggin 0 siblings, 0 replies; 11+ messages in thread From: Nicholas Piggin @ 2022-11-28 2:02 UTC (permalink / raw) To: Rohan McLure, linuxppc-dev On Mon Nov 7, 2022 at 1:32 PM AEST, Rohan McLure wrote: > Zero GPRS r14-r31 on entry into the kernel for interrupt sources to > limit influence of user-space values in potential speculation gadgets. > Prior to this commit, all other GPRS are reassigned during the common > prologue to interrupt handlers and so need not be zeroised explicitly. > > This may be done safely, without loss of register state prior to the > interrupt, as the common prologue saves the initial values of > non-volatiles, which are unconditionally restored in interrupt_64.S. In the case of ret_from_crit_except and ret_from_mc_except, it looks like those are restored by ret_from_level_except, so that's fine. And fast_interrupt_return you added NVGPRS restore in the previous patch too. Maybe actually you could move that interrupt_64.h code that applies to both 64s and 64e in patch 1. So then the 64s/e enablement patches are independent and apply to exactly that subarch. But code-wise I think this looks good. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > Mitigation defaults to enabled by INTERRUPT_SANITIZE_REGISTERS. > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > Resubmitting patches as their own series after v6 partially merged: > Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/ > --- > arch/powerpc/kernel/exceptions-64e.S | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S > index 2f68fb2ee4fc..91d8019123c2 100644 > --- a/arch/powerpc/kernel/exceptions-64e.S > +++ b/arch/powerpc/kernel/exceptions-64e.S > @@ -358,6 +358,11 @@ ret_from_mc_except: > std r14,PACA_EXMC+EX_R14(r13); \ > std r15,PACA_EXMC+EX_R15(r13) > > +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS > +#define SANITIZE_ZEROIZE_NVGPRS() ZEROIZE_NVGPRS() > +#else > +#define SANITIZE_ZEROIZE_NVGPRS() > +#endif Could possibly share these macros. > > /* Core exception code for all exceptions except TLB misses. */ > #define EXCEPTION_COMMON_LVL(n, scratch, excf) \ > @@ -394,7 +399,8 @@ exc_##n##_common: \ > std r12,STACK_FRAME_OVERHEAD-16(r1); /* mark the frame */ \ > std r3,_TRAP(r1); /* set trap number */ \ > std r0,RESULT(r1); /* clear regs->result */ \ > - SAVE_NVGPRS(r1); > + SAVE_NVGPRS(r1); \ > + SANITIZE_ZEROIZE_NVGPRS(); /* minimise speculation influence */ > > #define EXCEPTION_COMMON(n) \ > EXCEPTION_COMMON_LVL(n, SPRN_SPRG_GEN_SCRATCH, PACA_EXGEN) > -- > 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries 2022-11-07 3:31 [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure 2022-11-07 3:32 ` [PATCH v2 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure 2022-11-07 3:32 ` [PATCH v2 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure @ 2022-11-07 3:32 ` Rohan McLure 2022-11-28 2:12 ` Nicholas Piggin 2022-11-07 14:28 ` [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Christophe Leroy 2022-11-07 16:39 ` Segher Boessenkool 4 siblings, 1 reply; 11+ messages in thread From: Rohan McLure @ 2022-11-07 3:32 UTC (permalink / raw) To: linuxppc-dev; +Cc: Rohan McLure Cause pseries platforms to default to zeroising all potentially user-defined registers when entering the kernel by means of any interrupt source, reducing user-influence of the kernel and the likelihood or producing speculation gadgets. Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> --- Resubmitting patches as their own series after v6 partially merged: Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/ --- arch/powerpc/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 9d3d20c6f365..2eb328b25e49 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -532,7 +532,7 @@ config HOTPLUG_CPU config INTERRUPT_SANITIZE_REGISTERS bool "Clear gprs on interrupt arrival" depends on PPC64 && ARCH_HAS_SYSCALL_WRAPPER - default PPC_BOOK3E_64 + default PPC_BOOK3E_64 || PPC_PSERIES help Reduce the influence of user register state on interrupt handlers and syscalls through clearing user state from registers before handling -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries 2022-11-07 3:32 ` [PATCH v2 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries Rohan McLure @ 2022-11-28 2:12 ` Nicholas Piggin 0 siblings, 0 replies; 11+ messages in thread From: Nicholas Piggin @ 2022-11-28 2:12 UTC (permalink / raw) To: Rohan McLure, linuxppc-dev On Mon Nov 7, 2022 at 1:32 PM AEST, Rohan McLure wrote: > Cause pseries platforms to default to zeroising all potentially user-defined > registers when entering the kernel by means of any interrupt source, > reducing user-influence of the kernel and the likelihood or producing > speculation gadgets. For POWERNV as well? Thanks, Nick > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > Resubmitting patches as their own series after v6 partially merged: > Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/ > --- > arch/powerpc/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 9d3d20c6f365..2eb328b25e49 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -532,7 +532,7 @@ config HOTPLUG_CPU > config INTERRUPT_SANITIZE_REGISTERS > bool "Clear gprs on interrupt arrival" > depends on PPC64 && ARCH_HAS_SYSCALL_WRAPPER > - default PPC_BOOK3E_64 > + default PPC_BOOK3E_64 || PPC_PSERIES > help > Reduce the influence of user register state on interrupt handlers and > syscalls through clearing user state from registers before handling > -- > 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig 2022-11-07 3:31 [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure ` (2 preceding siblings ...) 2022-11-07 3:32 ` [PATCH v2 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries Rohan McLure @ 2022-11-07 14:28 ` Christophe Leroy 2022-11-28 1:42 ` Nicholas Piggin 2022-11-07 16:39 ` Segher Boessenkool 4 siblings, 1 reply; 11+ messages in thread From: Christophe Leroy @ 2022-11-07 14:28 UTC (permalink / raw) To: Rohan McLure, linuxppc-dev; +Cc: Nicholas Piggin Le 07/11/2022 à 04:31, Rohan McLure a écrit : > Add Kconfig option for enabling clearing of registers on arrival in an > interrupt handler. This reduces the speculation influence of registers > on kernel internals. The option will be consumed by 64-bit systems that > feature speculation and wish to implement this mitigation. > > This patch only introduces the Kconfig option, no actual mitigations. If that has to do with speculation, do we need a new Kconfig option ? Can't we use CONFIG_PPC_BARRIER_NOSPEC for that ? > > The primary overhead of this mitigation lies in an increased number of > registers that must be saved and restored by interrupt handlers on > Book3S systems. Enable by default on Book3E systems, which prior to > this patch eagerly save and restore register state, meaning that the > mitigation when implemented will have minimal overhead. > > Acked-by: Nicholas Piggin <npiggin@gmail.com> > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > Resubmitting patches as their own series after v6 partially merged: > Link: https://lore.kernel.org/all/166488988686.779920.13794870102696416283.b4-ty@ellerman.id.au/t/ > --- > arch/powerpc/Kconfig | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 2ca5418457ed..9d3d20c6f365 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -529,6 +529,15 @@ config HOTPLUG_CPU > > Say N if you are unsure. > > +config INTERRUPT_SANITIZE_REGISTERS > + bool "Clear gprs on interrupt arrival" > + depends on PPC64 && ARCH_HAS_SYSCALL_WRAPPER > + default PPC_BOOK3E_64 > + help > + Reduce the influence of user register state on interrupt handlers and > + syscalls through clearing user state from registers before handling > + the exception. > + > config PPC_QUEUED_SPINLOCKS > bool "Queued spinlocks" if EXPERT > depends on SMP ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig 2022-11-07 14:28 ` [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Christophe Leroy @ 2022-11-28 1:42 ` Nicholas Piggin 0 siblings, 0 replies; 11+ messages in thread From: Nicholas Piggin @ 2022-11-28 1:42 UTC (permalink / raw) To: Christophe Leroy, Rohan McLure, linuxppc-dev On Tue Nov 8, 2022 at 12:28 AM AEST, Christophe Leroy wrote: > > > Le 07/11/2022 à 04:31, Rohan McLure a écrit : > > Add Kconfig option for enabling clearing of registers on arrival in an > > interrupt handler. This reduces the speculation influence of registers > > on kernel internals. The option will be consumed by 64-bit systems that > > feature speculation and wish to implement this mitigation. > > > > This patch only introduces the Kconfig option, no actual mitigations. > > If that has to do with speculation, do we need a new Kconfig option ? > Can't we use CONFIG_PPC_BARRIER_NOSPEC for that ? NOSPEC barrier adds runtime-patchable hardware barrier and that config is a build implementation detail. Also that spec barrier is for bounds checks speculation that is easy to get the kernel to do something like speculatively branch to arbitrary address. Interrupt/syscall register sanitization is more handwavy. It could be a bandaid for cases where the above speculation barrier was missed for exampel. But at some point, at least for syscalls, registers have to contain some values influenced by userspace so if we were paranoid we would have to put barriers before every branch while any registers contained a value from userspace. A security option menu might be a good idea though. There's some other build time options like rop protection that we might want to add. Thanks, Nick ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig 2022-11-07 3:31 [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure ` (3 preceding siblings ...) 2022-11-07 14:28 ` [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Christophe Leroy @ 2022-11-07 16:39 ` Segher Boessenkool 2022-11-08 10:09 ` Nicholas Piggin 4 siblings, 1 reply; 11+ messages in thread From: Segher Boessenkool @ 2022-11-07 16:39 UTC (permalink / raw) To: Rohan McLure; +Cc: linuxppc-dev, Nicholas Piggin Hi! On Mon, Nov 07, 2022 at 02:31:59PM +1100, Rohan McLure wrote: > Add Kconfig option for enabling clearing of registers on arrival in an > interrupt handler. This reduces the speculation influence of registers > on kernel internals. Assuming you are talking about existing PowerPC CPUs from the last 30 years: There is no data speculation. At all. Ever. There is branch prediction, but that is not influenced by register contents, either (for any current CPUs at least). (Except when you get a flush because of a mispredict, but if this zeroing changes anything, we will have used wild (but user controlled) values in the old non-zeroing situation, and that is a much bigger problem itself already, also for security! This can be an unlikely kernel bug, or a very unlikely compiler bug.) All GPRs are renamed, always. If you zero all GPRs on interrupt entry (which is context synchronising, importantly), this will guarantee there can be no timing influence from the GPRs, because all of the physical registers depend on nothing that happened before. So that is good, at least it can give some peace of mind. Except that this makes 30 new registers in just a few cycles, which *itself* can cause stalls, if the renaming things are still busy. Context synchronising does not necessarily help there, the renaming machinery can do stuff *after* an insn completes. I don't see how this helps anything. If it does, "reduces speculation influence" is not a good description of what it does, afaics? Segher ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig 2022-11-07 16:39 ` Segher Boessenkool @ 2022-11-08 10:09 ` Nicholas Piggin 0 siblings, 0 replies; 11+ messages in thread From: Nicholas Piggin @ 2022-11-08 10:09 UTC (permalink / raw) To: Segher Boessenkool, Rohan McLure; +Cc: linuxppc-dev On Tue Nov 8, 2022 at 2:39 AM AEST, Segher Boessenkool wrote: > Hi! > > On Mon, Nov 07, 2022 at 02:31:59PM +1100, Rohan McLure wrote: > > Add Kconfig option for enabling clearing of registers on arrival in an > > interrupt handler. This reduces the speculation influence of registers > > on kernel internals. > > Assuming you are talking about existing PowerPC CPUs from the last 30 > years: > > There is no data speculation. At all. Ever. > > There is branch prediction, but that is not influenced by register > contents, either (for any current CPUs at least). (Except when you get > a flush because of a mispredict, but if this zeroing changes anything, > we will have used wild (but user controlled) values in the old > non-zeroing situation, and that is a much bigger problem itself already, > also for security! This can be an unlikely kernel bug, or a very > unlikely compiler bug.) This is not about data speculation, it is about speculative execution in kernel mode that uses architected value that were set by user, and that value being used to influence a speculative gadget that can expose data to user via a side channel. > All GPRs are renamed, always. If you zero all GPRs on interrupt entry > (which is context synchronising, importantly), this will guarantee there > can be no timing influence from the GPRs, because all of the physical > registers depend on nothing that happened before. So that is good, at > least it can give some peace of mind. Except that this makes 30 new > registers in just a few cycles, which *itself* can cause stalls, if the > renaming things are still busy. It will have some pipeline effect like any instruction I suppose. At least in latest processors, zeroing idiom should release registers AFAIK. > Context synchronising does not > necessarily help there, the renaming machinery can do stuff *after* an > insn completes. Possibly context synchronization does not push everything prior to completion, certainly it does not drain prior stores from store queues even if they had previously completed, so there can be things going on there. Software does not really have the ability to do anything about that though, so that's more of a hardware problem if that exposes a security issue IMO. Or at least a separate issue if there is some architecture that could deal with it. Thanks, Nick ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-11-28 2:13 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-07 3:31 [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure 2022-11-07 3:32 ` [PATCH v2 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure 2022-11-28 1:52 ` Nicholas Piggin 2022-11-07 3:32 ` [PATCH v2 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure 2022-11-28 2:02 ` Nicholas Piggin 2022-11-07 3:32 ` [PATCH v2 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries Rohan McLure 2022-11-28 2:12 ` Nicholas Piggin 2022-11-07 14:28 ` [PATCH v2 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Christophe Leroy 2022-11-28 1:42 ` Nicholas Piggin 2022-11-07 16:39 ` Segher Boessenkool 2022-11-08 10:09 ` Nicholas Piggin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.