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

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 <redacted>
---
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

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?

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

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

Le 11/08/2022 à 17:13, Segher Boessenkool a écrit :

Hi!

On Mon, Jul 25, 2022 at 04:31:11PM +1000, Rohan McLure wrote:

quoted

+	/*
+	 * 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

On Thu, Aug 11, 2022 at 03:39:58PM +0000, Christophe Leroy wrote:

Le 11/08/2022 à 17:13, Segher Boessenkool a écrit :

quoted

Hi!

On Mon, Jul 25, 2022 at 04:31:11PM +1000, Rohan McLure wrote:

quoted

+	/*
+	 * 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

quoted

quoted

"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

On 11 Aug 2022, at 8:11 pm, Andrew Donnellan [off-list ref] wrote:

On Mon, 2022-07-25 at 16:31 +1000, Rohan McLure wrote:

quoted

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.

Le 15/08/2022 à 01:59, Rohan McLure a écrit :

quoted

quoted

quoted

"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