From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Rohan McLure" <rmclure@linux.ibm.com>, <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v4 5/7] powerpc/64s: Zeroise gprs on interrupt routine entry on Book3S
Date: Tue, 29 Nov 2022 20:20:29 +1000 [thread overview]
Message-ID: <COOPCXXM1F8D.14MC5S84PB97E@bobo> (raw)
In-Reply-To: <20221129044354.1836018-5-rmclure@linux.ibm.com>
On Tue Nov 29, 2022 at 2:43 PM AEST, Rohan McLure wrote:
> Zeroise 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.
>
> Zeroise 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. If an interrupt does not
> select the IMSR_R12 IOption, zeroise r12.
>
> 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.
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> v2: REST_NVGPRS should be conditional on mitigation in scv handler. Fix
> improper multi-line preprocessor macro in interrupt_64.S
> v4: Split off IMSR_R12 definition into its own patch. Move macro
> definitions for register sanitisation into asm/ppc_asm.h
> ---
> arch/powerpc/kernel/exceptions-64s.S | 27 ++++++++++++++++++---------
> arch/powerpc/kernel/interrupt_64.S | 16 ++++++++++++++--
> 2 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 58d72db1d484..8ee3db3b6595 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -506,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
> @@ -548,8 +549,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
These unconditionally zero. Should be SANITIZE_ZEROIZE_GPRS()? Same
for a few more cases.
Hmm. r12 actually contains come-from-MSR here, which isn't really user
controlled. r9-r11 just got loaded with some user GPRs, but they're
the usual scratch registers and get used for other things later.
The whole interrupt entry file could probably use a spring clean, some
re-scheduling, data layout improvements, and more consistent use of
scratch registers... so I'm overly concerned about removing every
possible redundant instruction here if it makes things a bit harder
to follow. But we might be able to get rid of the above zeroize, with
a comment.
>
> SAVE_NVGPRS(r1)
> + SANITIZE_ZEROIZE_NVGPRS()
>
> .if IDAR
> .if IISIDE
> @@ -581,8 +588,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)
r2 gets zeroed then used again in LOAD_PACA_TOC too.
Otherwise looks pretty good... Although CTR doesn't get zeroed and I
suppose it could be speculatively used as a source (e.g., bctr). CRs
other than 0 and sometimes 1 too, they're probably a bit less important
than CTR though. We don't use TAR in the kernel so that one should be
okay to leave (maybe with a comment).
That can be done another time, I'd like to see the GPR sanitising
patches go in... It *might* just be a matter of one mtctr in the
case of !RELOCATABLE kernel though to get the ctr...
Still-Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thanks,
Nick
next prev parent reply other threads:[~2022-11-29 10:21 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-29 4:43 [PATCH v4 1/7] powerpc/64: Add INTERRUPT_SANITIZE_REGISTERS Kconfig Rohan McLure
2022-11-29 4:43 ` [PATCH v4 2/7] powerpc/64: Add interrupt register sanitisation macros Rohan McLure
2022-11-29 9:40 ` Nicholas Piggin
2022-11-29 4:43 ` [PATCH v4 3/7] powerpc/64: Sanitise common exit code for interrupts Rohan McLure
2022-11-29 9:46 ` Nicholas Piggin
2022-11-29 4:43 ` [PATCH v4 4/7] powerpc/64s: IOption for MSR stored in r12 Rohan McLure
2022-11-29 9:48 ` Nicholas Piggin
2022-11-29 4:43 ` [PATCH v4 5/7] powerpc/64s: Zeroise gprs on interrupt routine entry on Book3S Rohan McLure
2022-11-29 10:20 ` Nicholas Piggin [this message]
2022-11-29 4:43 ` [PATCH v4 6/7] powerpc/64e: Clear gprs on interrupt routine entry on Book3E Rohan McLure
2022-11-29 4:43 ` [PATCH v4 7/7] powerpc/64: Sanitise user registers on interrupt in pseries, POWERNV Rohan McLure
2022-11-29 10:21 ` Nicholas Piggin
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=COOPCXXM1F8D.14MC5S84PB97E@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;
as well as URLs for NNTP newsgroup(s).