Re: [PATCH] powerpc/tm: Fix more userspace r13 corruption

[Michael Neuling <mikey@neuling.org>]

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
>