* [PATCH 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig
@ 2022-11-02 22:45 Rohan McLure
2022-11-02 22:45 ` [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Rohan McLure @ 2022-11-02 22:45 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, npiggin
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.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Acked-by: Nicholas Piggin <npiggin@gmail.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/
V5: New patch
---
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] 5+ messages in thread
* [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S
2022-11-02 22:45 [PATCH 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure
@ 2022-11-02 22:45 ` Rohan McLure
2022-11-07 1:08 ` Russell Currey
2022-11-02 22:45 ` [PATCH 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure
2022-11-02 22:45 ` [PATCH 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries Rohan McLure
2 siblings, 1 reply; 5+ messages in thread
From: Rohan McLure @ 2022-11-02 22:45 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, npiggin
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/
Standalone series: Now syscall register clearing is included
under the same configuration option. This now matches the
description given for CONFIG_INTERRUPT_SANITIZE_REGISTERS.
---
arch/powerpc/kernel/exceptions-64s.S | 47 +++++++++++++++++++++-----
arch/powerpc/kernel/interrupt_64.S | 34 +++++++++++++++++++
2 files changed, 72 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..26c04b83da1b 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)
+ REST_NVGPRS(r1)
cmpdi r3,0
bne .Lsyscall_vectored_\name\()_restore_regs
@@ -275,6 +295,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 +340,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 +368,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 +436,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 +454,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 +524,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 +618,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] 5+ messages in thread
* [PATCH 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E
2022-11-02 22:45 [PATCH 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure
2022-11-02 22:45 ` [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure
@ 2022-11-02 22:45 ` Rohan McLure
2022-11-02 22:45 ` [PATCH 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries Rohan McLure
2 siblings, 0 replies; 5+ messages in thread
From: Rohan McLure @ 2022-11-02 22:45 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, npiggin
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/
V4: New patch.
V5: Depend on Kconfig option. Remove ZEROIZE_NVGPRS on bad kernel
stack handler.
V6: Change name of zeroize macro to be consistent with Book3S.
---
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] 5+ messages in thread
* [PATCH 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries
2022-11-02 22:45 [PATCH 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure
2022-11-02 22:45 ` [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure
2022-11-02 22:45 ` [PATCH 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure
@ 2022-11-02 22:45 ` Rohan McLure
2 siblings, 0 replies; 5+ messages in thread
From: Rohan McLure @ 2022-11-02 22:45 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, npiggin
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. Interrupt sources include syscalls.
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/
Standalone series: new patch
---
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] 5+ messages in thread
* Re: [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S
2022-11-02 22:45 ` [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure
@ 2022-11-07 1:08 ` Russell Currey
0 siblings, 0 replies; 5+ messages in thread
From: Russell Currey @ 2022-11-07 1:08 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev; +Cc: npiggin
On Thu, 2022-11-03 at 09:45 +1100, 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.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
Hi Rohan, I haven't figured out what it's coming from, but I'm seeing
assembler errors from this patch:
/linux/arch/powerpc/kernel/interrupt_64.S: Assembler messages:
/linux/arch/powerpc/kernel/interrupt_64.S:212: Error: too many
positional arguments
/linux/arch/powerpc/kernel/interrupt_64.S:219: Error: too many
positional arguments
/linux/arch/powerpc/kernel/interrupt_64.S:302: Error: too many
positional arguments
Log here:
https://github.com/ruscur/linux-ci/actions/runs/3381711748/jobs/5615903876#step:4:98
> 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/
>
> Standalone series: Now syscall register clearing is included
> under the same configuration option. This now matches the
> description given for CONFIG_INTERRUPT_SANITIZE_REGISTERS.
> ---
> arch/powerpc/kernel/exceptions-64s.S | 47 +++++++++++++++++++++-----
> arch/powerpc/kernel/interrupt_64.S | 34 +++++++++++++++++++
> 2 files changed, 72 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..26c04b83da1b 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) \
Getting a warning from some trailing whitespace on this line.
> + 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)
>
> + REST_NVGPRS(r1)
Should this be SANITIZE_RESTORE_NVGPRS()?
> cmpdi r3,0
> bne .Lsyscall_vectored_\name\()_restore_regs
>
> @@ -275,6 +295,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 +340,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 +368,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 +436,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 +454,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 +524,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 +618,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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-07 1:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-02 22:45 [PATCH 1/4] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure
2022-11-02 22:45 ` [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S Rohan McLure
2022-11-07 1:08 ` Russell Currey
2022-11-02 22:45 ` [PATCH 3/4] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure
2022-11-02 22:45 ` [PATCH 4/4] powerpc/64s: Sanitise user registers on interrupt in pseries Rohan McLure
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).