From: Michael Neuling <mikey@neuling.org>
To: Nicholas Piggin <npiggin@gmail.com>, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] powerpc/tm: Fix more userspace r13 corruption
Date: Fri, 11 Mar 2022 14:59:42 +1100 [thread overview]
Message-ID: <b7ddaf945ed35c0e072acc41b7bbc6d8c4c5d69f.camel@neuling.org> (raw)
In-Reply-To: <20220311024733.48926-1-npiggin@gmail.com>
On Fri, 2022-03-11 at 12:47 +1000, Nicholas Piggin wrote:
> Commit cf13435b730a ("powerpc/tm: Fix userspace r13 corruption") fixes
> a problem in treclaim where a SLB miss can occur on the
> thread_struct->ckpt_regs while SCRATCH0 is live with the saved user r13
> value, clobbering it with the kernel r13 and ultimately resulting in
> kernel r13 being stored in ckpt_regs.
>
> There is an equivalent problem in trechkpt where the user r13 value is
> loaded into r13 from chkpt_regs to be recheckpointed, but a SLB miss
> could occur on ckpt_regs accesses after that, which will result in r13
> being clobbered with a kernel value and that will get recheckpointed and
> then restored to user registers.
>
> The same memory page is accessed right before this critical window where
> a SLB miss could cause corruption, so hitting the bug requires the SLB
> entry be removed within a small window of instructions, which is possible
> if a SLB related MCE hits there. PAPR also permits the hypervisor to
> discard this SLB entry (because slb_shadow->persistent is only set to
> SLB_NUM_BOLTED) although it's not known whether any implementations would
> do this (KVM does not). So this is an extremely unlikely bug, only found
> by inspection.
>
> Fix this by also storing user r13 in a temporary location on the kernel
> stack and don't chane the r13 register from kernel r13 until the RI=0
> critical section that does not fault.
s/chane/change/
>
> [ The SCRATCH0 change is not strictly part of the fix, it's only used in
> the RI=0 section so it does not have the same problem as the previous
> SCRATCH0 bug. ]
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
This needs to be marked for stable also. Other than that:
Acked-by: Michael Neuling <mikey@neuling.org>
Thanks!
> ---
> arch/powerpc/kernel/tm.S | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
> index 3beecc32940b..5a0f023a26e9 100644
> --- a/arch/powerpc/kernel/tm.S
> +++ b/arch/powerpc/kernel/tm.S
> @@ -443,7 +443,8 @@ restore_gprs:
>
> REST_GPR(0, r7) /* GPR0 */
> REST_GPRS(2, 4, r7) /* GPR2-4 */
> - REST_GPRS(8, 31, r7) /* GPR8-31 */
> + REST_GPRS(8, 12, r7) /* GPR8-12 */
> + REST_GPRS(14, 31, r7) /* GPR14-31 */
>
> /* Load up PPR and DSCR here so we don't run with user values for long
> */
> mtspr SPRN_DSCR, r5
> @@ -479,18 +480,24 @@ restore_gprs:
> REST_GPR(6, r7)
>
> /*
> - * Store r1 and r5 on the stack so that we can access them after we
> - * clear MSR RI.
> + * Store user r1 and r5 and r13 on the stack (in the unused save
> + * areas / compiler reserved areas), so that we can access them after
> + * we clear MSR RI.
> */
>
> REST_GPR(5, r7)
> std r5, -8(r1)
> - ld r5, GPR1(r7)
> + ld r5, GPR13(r7)
> std r5, -16(r1)
> + ld r5, GPR1(r7)
> + std r5, -24(r1)
>
> REST_GPR(7, r7)
>
> - /* Clear MSR RI since we are about to use SCRATCH0. EE is already off
> */
> + /* Stash the stack pointer away for use after recheckpoint */
> + std r1, PACAR1(r13)
> +
> + /* Clear MSR RI since we are about to clobber r13. EE is already off
> */
> li r5, 0
> mtmsrd r5, 1
>
> @@ -501,9 +508,9 @@ restore_gprs:
> * until we turn MSR RI back on.
> */
>
> - SET_SCRATCH0(r1)
> ld r5, -8(r1)
> - ld r1, -16(r1)
> + ld r13, -16(r1)
> + ld r1, -24(r1)
>
> /* Commit register state as checkpointed state: */
> TRECHKPT
> @@ -519,9 +526,9 @@ restore_gprs:
> */
>
> GET_PACA(r13)
> - GET_SCRATCH0(r1)
> + ld r1, PACAR1(r13)
>
> - /* R1 is restored, so we are recoverable again. EE is still off */
> + /* R13, R1 is restored, so we are recoverable again. EE is still off
> */
> li r4, MSR_RI
> mtmsrd r4, 1
>
next prev parent reply other threads:[~2022-03-11 4:00 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-11 2:47 [PATCH] powerpc/tm: Fix more userspace r13 corruption Nicholas Piggin
2022-03-11 3:59 ` Michael Neuling [this message]
2022-03-16 1:00 ` Michael Ellerman
2022-03-21 5:05 ` Michael Ellerman
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=b7ddaf945ed35c0e072acc41b7bbc6d8c4c5d69f.camel@neuling.org \
--to=mikey@neuling.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=npiggin@gmail.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 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.