From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Rohan McLure" <rmclure@linux.ibm.com>, <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH 22/23] powerpc/64s: Clear gprs on interrupt routine entry in Book3S
Date: Tue, 20 Sep 2022 12:27:15 +1000 [thread overview]
Message-ID: <CN0VGH6HLTMX.3KK2Z9RMT9N6B@bobo> (raw)
In-Reply-To: <20220916053300.786330-23-rmclure@linux.ibm.com>
On Fri Sep 16, 2022 at 3:32 PM AEST, Rohan McLure wrote:
> Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
> other interrupt sources to limit influence of user-space values
> in potential speculation gadgets. The remaining gprs are overwritten by
> entry macros to interrupt handlers, irrespective of whether or not a
> given handler consumes these register values.
>
> Prior to this commit, r14-r31 are restored on a per-interrupt basis at
> exit, but now they are always restored. Remove explicit REST_NVGPRS
> invocations as non-volatiles must now always be restored. 32-bit systems
> do not clear user registers on interrupt, and continue to depend on the
> return value of interrupt_exit_user_prepare to determine whether or not
> to restore non-volatiles.
>
> The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
> See ~0.8% performance regression with this mitigation, but this
> indicates the worst-case performance due to heavier-weight interrupt
> handlers. This mitigation is disabled by default, but enabled with
> CONFIG_INTERRUPT_SANITIZE_REGISTERS.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Add benchmark data
> V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify
> interrupt_exit_user_prepare changes in summary.
> V4 -> V5: Configurable now with INTERRUPT_SANITIZE_REGISTERS. Zero r12
> (containing MSR) from common macro on per-interrupt basis with IOPTION.
Thanks, this is looking pretty good. I'll have a closer look at the asm
though.
Possibly here and in some of the previous patches as well there could be
some macros that hide some of the ifdefs.
e.g., SANITIZE_ZEROIZE_NVGPRS.
And HANDLER_RESTORE_NVGPRS() could do the restore if !SANITIZE, etc.
Also I just realised you're Americanising arch/powerpc. Lizzy would not
have been amused.
Thanks,
Nick
> ---
> arch/powerpc/kernel/exceptions-64s.S | 37 ++++++++++++++++++++++++--
> arch/powerpc/kernel/interrupt_64.S | 10 +++++++
> 2 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index a3b51441b039..be5e72caada1 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -111,6 +111,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 +177,9 @@ do_define_int n
> .ifndef IKUAP
> IKUAP=1
> .endif
> + .ifndef IMSR_R12
> + IMSR_R12=0
> + .endif
> .endm
>
> /*
> @@ -502,6 +506,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 +549,16 @@ 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)
> +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> + ZEROIZE_NVGPRS()
> +#endif
>
> .if IDAR
> .if IISIDE
> @@ -577,8 +590,8 @@ BEGIN_FTR_SECTION
> END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> ld r10,IAREA+EX_CTR(r13)
> std r10,_CTR(r1)
> - std r2,GPR2(r1) /* save r2 in stackframe */
> - SAVE_GPRS(3, 8, r1) /* save r3 - r8 in stackframe */
> + SAVE_GPRS(2, 8, r1) /* save r2 - r8 in stackframe */
> + ZEROIZE_GPRS(2, 8)
> mflr r9 /* Get LR, later save to stack */
> ld r2,PACATOC(r13) /* get kernel TOC into r2 */
> std r9,_LINK(r1)
> @@ -696,6 +709,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> mtlr r9
> ld r9,_CCR(r1)
> mtcr r9
> +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> + REST_NVGPRS(r1)
> +#endif
> REST_GPRS(2, 13, r1)
> REST_GPR(0, r1)
> /* restore original r1. */
> @@ -1368,11 +1384,13 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
> b interrupt_return_srr
>
> 1: bl do_break
> +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> /*
> * 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)
> +#endif
> b interrupt_return_srr
>
>
> @@ -1598,7 +1616,9 @@ EXC_COMMON_BEGIN(alignment_common)
> GEN_COMMON alignment
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl alignment_exception
> +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> +#endif
> b interrupt_return_srr
>
>
> @@ -1708,7 +1728,9 @@ EXC_COMMON_BEGIN(program_check_common)
> .Ldo_program_check:
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl program_check_exception
> +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> +#endif
> b interrupt_return_srr
>
>
> @@ -1726,6 +1748,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)
> @@ -2139,7 +2162,9 @@ EXC_COMMON_BEGIN(emulation_assist_common)
> GEN_COMMON emulation_assist
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl emulation_assist_interrupt
> +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> +#endif
> b interrupt_return_hsrr
>
>
> @@ -2347,6 +2372,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)
> @@ -2396,6 +2422,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)
> @@ -2457,7 +2484,9 @@ EXC_COMMON_BEGIN(facility_unavailable_common)
> GEN_COMMON facility_unavailable
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl facility_unavailable_exception
> +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> +#endif
> b interrupt_return_srr
>
>
> @@ -2485,7 +2514,9 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common)
> GEN_COMMON h_facility_unavailable
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl facility_unavailable_exception
> +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> REST_NVGPRS(r1) /* XXX Shouldn't be necessary in practice */
> +#endif
> b interrupt_return_hsrr
>
>
> @@ -2711,7 +2742,9 @@ EXC_COMMON_BEGIN(altivec_assist_common)
> addi r3,r1,STACK_FRAME_OVERHEAD
> #ifdef CONFIG_ALTIVEC
> bl altivec_assist_exception
> +#ifndef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> +#endif
> #else
> bl unknown_exception
> #endif
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index 40147558e1a6..edad0c17e47a 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -433,9 +433,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:
> @@ -449,6 +451,9 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user)
> stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS
>
> .Lfast_user_interrupt_return_\srr\():
> +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> + REST_NVGPRS(r1)
> +#endif
> #ifdef CONFIG_PPC_BOOK3S
> .ifc \srr,srr
> lbz r4,PACASRR_VALID(r13)
> @@ -518,9 +523,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:
> @@ -562,6 +569,9 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel)
> 1:
>
> .Lfast_kernel_interrupt_return_\srr\():
> +#ifdef CONFIG_INTERRUPT_SANITIZE_REGISTERS
> + REST_NVGPRS(r1)
> +#endif
> cmpdi cr1,r3,0
> #ifdef CONFIG_PPC_BOOK3S
> .ifc \srr,srr
> --
> 2.34.1
next prev parent reply other threads:[~2022-09-20 2:28 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-16 5:32 [PATCH 00/23] powerpc: Syscall wrapper and register clearing Rohan McLure
2022-09-16 5:32 ` [PATCH 01/23] powerpc: Remove asmlinkage from syscall handler definitions Rohan McLure
2022-09-16 5:32 ` [PATCH 02/23] powerpc: Save caller r3 prior to system_call_exception Rohan McLure
2022-09-16 5:32 ` [PATCH 03/23] powerpc: Add ZEROIZE_GPRS macros for register clears Rohan McLure
2022-09-16 5:32 ` [PATCH 04/23] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers Rohan McLure
2022-09-16 5:32 ` [PATCH 05/23] powerpc/32: Clarify interrupt restores with REST_GPR macro in entry_32.S Rohan McLure
2022-09-20 0:51 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 06/23] powerpc/64e: Clarify register saves and clears with {SAVE,ZEROIZE}_GPRS Rohan McLure
2022-09-20 0:55 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 07/23] powerpc/64s: Fix comment on interrupt handler prologue Rohan McLure
2022-09-20 0:55 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 08/23] powerpc: Fix fallocate and fadvise64_64 compat parameter combination Rohan McLure
2022-09-16 6:54 ` Arnd Bergmann
2022-09-20 1:01 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 09/23] asm-generic: compat: Support BE for long long args in 32-bit ABIs Rohan McLure
2022-09-20 1:06 ` Nicholas Piggin
2022-09-20 7:09 ` Arnd Bergmann
2022-09-16 5:32 ` [PATCH 10/23] powerpc: Use generic fallocate compatibility syscall Rohan McLure
2022-09-16 6:56 ` Arnd Bergmann
2022-09-16 5:32 ` [PATCH 11/23] powerpc/32: Remove powerpc select specialisation Rohan McLure
2022-09-16 5:32 ` [PATCH 12/23] powerpc: Remove direct call to personality syscall handler Rohan McLure
2022-09-16 5:32 ` [PATCH 13/23] powerpc: Remove direct call to mmap2 syscall handlers Rohan McLure
2022-09-16 5:32 ` [PATCH 14/23] powerpc: Provide do_ppc64_personality helper Rohan McLure
2022-09-16 5:32 ` [PATCH 15/23] powerpc: Adopt SYSCALL_DEFINE for arch-specific syscall handlers Rohan McLure
2022-09-16 5:48 ` Rohan McLure
2022-09-20 1:24 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 16/23] powerpc: Include all arch-specific syscall prototypes Rohan McLure
2022-09-20 1:27 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 17/23] powerpc: Enable compile-time check for syscall handlers Rohan McLure
2022-09-20 1:30 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 18/23] powerpc: Use common syscall handler type Rohan McLure
2022-09-20 1:39 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 19/23] powerpc: Provide syscall wrapper Rohan McLure
2022-09-20 1:59 ` Nicholas Piggin
2022-09-21 3:44 ` Rohan McLure
2022-09-16 5:32 ` [PATCH 20/23] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return Rohan McLure
2022-09-20 2:03 ` Nicholas Piggin
2022-09-20 4:54 ` Rohan McLure
2022-09-21 5:33 ` Rohan McLure
2022-09-20 2:07 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 21/23] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure
2022-09-20 2:10 ` Nicholas Piggin
2022-09-16 5:32 ` [PATCH 22/23] powerpc/64s: Clear gprs on interrupt routine entry in Book3S Rohan McLure
2022-09-20 2:27 ` Nicholas Piggin [this message]
2022-09-16 5:33 ` [PATCH 23/23] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure
2022-09-16 5:58 ` [PATCH 00/23] powerpc: Syscall wrapper and register clearing 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=CN0VGH6HLTMX.3KK2Z9RMT9N6B@bobo \
--to=npiggin@gmail.com \
--cc=linuxppc-dev@lists.ozlabs.org \
--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