From: Russell Currey <ruscur@russell.cc>
To: Rohan McLure <rmclure@linux.ibm.com>, linuxppc-dev@lists.ozlabs.org
Cc: npiggin@gmail.com
Subject: Re: [PATCH 2/4] powerpc/64s: Clear gprs on interrupt routine entry on Book3S
Date: Mon, 07 Nov 2022 12:08:33 +1100 [thread overview]
Message-ID: <87aa4cce8ae878dc61bd460dec2c98704f82acdd.camel@russell.cc> (raw)
In-Reply-To: <20221102224528.639894-2-rmclure@linux.ibm.com>
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
next prev parent reply other threads:[~2022-11-07 1:09 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87aa4cce8ae878dc61bd460dec2c98704f82acdd.camel@russell.cc \
--to=ruscur@russell.cc \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=npiggin@gmail.com \
--cc=rmclure@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).