[PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Rohan McLure]

Clear 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.

One function of syscall_exit_prepare is to determine when non-volatile
regs must be restored, and it still serves that purpose on 32-bit. Use
it now for determining where to find XER, CTR, CR.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Update summary
---
 arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index 3e8a811e09c4..34167cfa5d60 100644
--- a/arch/powerpc/kernel/interrupt_64.S
+++ b/arch/powerpc/kernel/interrupt_64.S
@@ -70,7 +70,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
 	ld	r2,PACATOC(r13)
 	mfcr	r12
 	li	r11,0
-	/* Can we avoid saving r3-r8 in common case? */
+	/* Save syscall parameters in r3-r8 */
 	std	r3,GPR3(r1)
 	std	r4,GPR4(r1)
 	std	r5,GPR5(r1)
@@ -108,6 +108,13 @@ 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.
+	 */
+	NULLIFY_GPRS(5, 12)
+	NULLIFY_NVGPRS()
+
 	/* Calling convention has r3 = orig r0, r4 = regs */
 	mr	r3,r0
 	bl	system_call_exception
@@ -138,6 +145,7 @@ BEGIN_FTR_SECTION
 	HMT_MEDIUM_LOW
 END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 
+	REST_NVGPRS(r1)
 	cmpdi	r3,0
 	bne	.Lsyscall_vectored_\name\()_restore_regs
 
@@ -180,7 +188,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	ld	r4,_LINK(r1)
 	ld	r5,_XER(r1)
 
-	REST_NVGPRS(r1)
 	ld	r0,GPR0(r1)
 	mtcr	r2
 	mtctr	r3
@@ -248,7 +255,7 @@ END_BTB_FLUSH_SECTION
 	ld	r2,PACATOC(r13)
 	mfcr	r12
 	li	r11,0
-	/* Can we avoid saving r3-r8 in common case? */
+	/* Save syscall parameters in r3-r8 */
 	std	r3,GPR3(r1)
 	std	r4,GPR4(r1)
 	std	r5,GPR5(r1)
@@ -298,6 +305,13 @@ END_BTB_FLUSH_SECTION
 	wrteei	1
 #endif
 
+	/*
+	 * Zero user registers to prevent influencing speculative execution
+	 * state of kernel code.
+	 */
+	NULLIFY_GPRS(5, 12)
+	NULLIFY_NVGPRS()
+
 	/* Calling convention has r3 = orig r0, r4 = regs */
 	mr	r3,r0
 	bl	system_call_exception
@@ -340,6 +354,7 @@ BEGIN_FTR_SECTION
 	stdcx.	r0,0,r1			/* to clear the reservation */
 END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
 
+	REST_NVGPRS(r1)
 	cmpdi	r3,0
 	bne	.Lsyscall_restore_regs
 	/* Zero volatile regs that may contain sensitive kernel data */
@@ -367,7 +382,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 .Lsyscall_restore_regs:
 	ld	r3,_CTR(r1)
 	ld	r4,_XER(r1)
-	REST_NVGPRS(r1)
 	mtctr	r3
 	mtspr	SPRN_XER,r4
 	REST_GPR(0, r1)
-- 
2.34.1

Re: [PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Andrew Donnellan]

On Mon, 2022-07-25 at 16:31 +1000, Rohan McLure wrote:
> Clear 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.
> 
> One function of syscall_exit_prepare is to determine when non-
> volatile
> regs must be restored, and it still serves that purpose on 32-bit.
> Use
> it now for determining where to find XER, CTR, CR.

I'm not sure exactly how syscall_exit_prepare comes into this?

> 
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Update summary
> ---
>  arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt_64.S
> b/arch/powerpc/kernel/interrupt_64.S
> index 3e8a811e09c4..34167cfa5d60 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -70,7 +70,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
>         ld      r2,PACATOC(r13)
>         mfcr    r12
>         li      r11,0
> -       /* Can we avoid saving r3-r8 in common case? */
> +       /* Save syscall parameters in r3-r8 */
>         std     r3,GPR3(r1)
>         std     r4,GPR4(r1)
>         std     r5,GPR5(r1)
> @@ -108,6 +108,13 @@ 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.
> +        */
> +       NULLIFY_GPRS(5, 12)
> +       NULLIFY_NVGPRS()
> +
>         /* Calling convention has r3 = orig r0, r4 = regs */
>         mr      r3,r0
>         bl      system_call_exception
> @@ -138,6 +145,7 @@ BEGIN_FTR_SECTION
>         HMT_MEDIUM_LOW
>  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  
> +       REST_NVGPRS(r1)
>         cmpdi   r3,0
>         bne     .Lsyscall_vectored_\name\()_restore_regs
>  
> @@ -180,7 +188,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>         ld      r4,_LINK(r1)
>         ld      r5,_XER(r1)
>  
> -       REST_NVGPRS(r1)
>         ld      r0,GPR0(r1)
>         mtcr    r2
>         mtctr   r3
> @@ -248,7 +255,7 @@ END_BTB_FLUSH_SECTION
>         ld      r2,PACATOC(r13)
>         mfcr    r12
>         li      r11,0
> -       /* Can we avoid saving r3-r8 in common case? */
> +       /* Save syscall parameters in r3-r8 */
>         std     r3,GPR3(r1)
>         std     r4,GPR4(r1)
>         std     r5,GPR5(r1)
> @@ -298,6 +305,13 @@ END_BTB_FLUSH_SECTION
>         wrteei  1
>  #endif
>  
> +       /*
> +        * Zero user registers to prevent influencing speculative
> execution
> +        * state of kernel code.
> +        */
> +       NULLIFY_GPRS(5, 12)
> +       NULLIFY_NVGPRS()
> +
>         /* Calling convention has r3 = orig r0, r4 = regs */
>         mr      r3,r0
>         bl      system_call_exception
> @@ -340,6 +354,7 @@ BEGIN_FTR_SECTION
>         stdcx.  r0,0,r1                 /* to clear the reservation
> */
>  END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>  
> +       REST_NVGPRS(r1)
>         cmpdi   r3,0
>         bne     .Lsyscall_restore_regs
>         /* Zero volatile regs that may contain sensitive kernel data
> */
> @@ -367,7 +382,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  .Lsyscall_restore_regs:
>         ld      r3,_CTR(r1)
>         ld      r4,_XER(r1)
> -       REST_NVGPRS(r1)
>         mtctr   r3
>         mtspr   SPRN_XER,r4
>         REST_GPR(0, r1)

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Rohan McLure]

> On 11 Aug 2022, at 8:11 pm, Andrew Donnellan <ajd@linux.ibm.com> wrote:
> 
> On Mon, 2022-07-25 at 16:31 +1000, Rohan McLure wrote:
>> Clear 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.
>> 
>> One function of syscall_exit_prepare is to determine when non-
>> volatile
>> regs must be restored, and it still serves that purpose on 32-bit.
>> Use
>> it now for determining where to find XER, CTR, CR.
> 
> I'm not sure exactly how syscall_exit_prepare comes into this?

Apologies, this comment belongs in patch 14 and concerns interrupt_exit_user_prepare.

Re: [PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Segher Boessenkool]

Hi!

On Mon, Jul 25, 2022 at 04:31:11PM +1000, Rohan McLure wrote:
> +	/*
> +	 * Zero user registers to prevent influencing speculative execution
> +	 * state of kernel code.
> +	 */
> +	NULLIFY_GPRS(5, 12)
> +	NULLIFY_NVGPRS()

"Nullify" means "invalidate", which is not what this does or is for :-(


Segher

Re: [PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Christophe Leroy]

Le 11/08/2022 à 17:13, Segher Boessenkool a écrit :
> Hi!
> 
> On Mon, Jul 25, 2022 at 04:31:11PM +1000, Rohan McLure wrote:
>> +	/*
>> +	 * Zero user registers to prevent influencing speculative execution
>> +	 * state of kernel code.
>> +	 */
>> +	NULLIFY_GPRS(5, 12)
>> +	NULLIFY_NVGPRS()
> 
> "Nullify" means "invalidate", which is not what this does or is for :-(
> 

Would "Zeroise" be more appropriate ?

Christophe

Re: [PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Segher Boessenkool]

On Thu, Aug 11, 2022 at 03:39:58PM +0000, Christophe Leroy wrote:
> 
> 
> Le 11/08/2022 à 17:13, Segher Boessenkool a écrit :
> > Hi!
> > 
> > On Mon, Jul 25, 2022 at 04:31:11PM +1000, Rohan McLure wrote:
> >> +	/*
> >> +	 * Zero user registers to prevent influencing speculative execution
> >> +	 * state of kernel code.
> >> +	 */
> >> +	NULLIFY_GPRS(5, 12)
> >> +	NULLIFY_NVGPRS()
> > 
> > "Nullify" means "invalidate", which is not what this does or is for :-(
> 
> Would "Zeroise" be more appropriate ?

That is probably a good compromise, yes.  It obviously is a verb, its
meaning is clear and unamiguous, and there is precedent for it even :-)

Thanks,


Segher

Re: [PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Rohan McLure]

>>> "Nullify" means "invalidate", which is not what this does or is for :-(
>> 
>> Would "Zeroise" be more appropriate ?
> 
> That is probably a good compromise, yes.  It obviously is a verb, its
> meaning is clear and unamiguous, and there is precedent for it even :-)

Zeroise it is. The ‘zeroize’ spelling exists already in the kernel so I’ll use that.

Rohan

Re: [PATCH v2 11/14] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

[Christophe Leroy]

Le 15/08/2022 à 01:59, Rohan McLure a écrit :
>>>> "Nullify" means "invalidate", which is not what this does or is for :-(
>>>
>>> Would "Zeroise" be more appropriate ?
>>
>> That is probably a good compromise, yes.  It obviously is a verb, its
>> meaning is clear and unamiguous, and there is precedent for it even :-)
> 
> Zeroise it is. The ‘zeroize’ spelling exists already in the kernel so I’ll use that.

I have some preference for British spelling over American spelling, but 
never mind.

Christophe