diff mbox series

[20/23] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return

Message ID 20220916053300.786330-21-rmclure@linux.ibm.com (mailing list archive)
State Superseded
Headers show
Series powerpc: Syscall wrapper and register clearing | expand

Commit Message

Rohan McLure Sept. 16, 2022, 5:32 a.m. UTC 
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.

Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Update summary
V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
V3 -> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
V4 -> V5: Move to end of patch series.
---
 arch/powerpc/kernel/interrupt_64.S | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Nicholas Piggin Sept. 20, 2022, 2:03 a.m. UTC | #1 
On Fri Sep 16, 2022 at 3:32 PM AEST, 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.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Update summary
> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
> V3 -> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
> V4 -> V5: Move to end of patch series.
> ---

I think it looks okay. I'll have to take a better look with the series
applied.

>  arch/powerpc/kernel/interrupt_64.S | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index 16a1b44088e7..40147558e1a6 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 */

These two comment changes could go in your system_call_exception API
change patch though.

Thanks,
Nick

>  	SAVE_GPRS(3, 8, r1)
>  	/* Zero r9-r12, this should only be required when restoring all GPRs */
>  	std	r11,GPR9(r1)
> @@ -110,6 +110,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	 * Zero user registers to prevent influencing speculative execution
>  	 * state of kernel code.
>  	 */
> +	ZEROIZE_GPR(0)
>  	ZEROIZE_GPRS(5, 12)
>  	ZEROIZE_NVGPRS()
>  	bl	system_call_exception
> @@ -140,6 +141,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
>  
> @@ -243,7 +245,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 */
>  	SAVE_GPRS(3, 8, r1)
>  	/* Zero r9-r12, this should only be required when restoring all GPRs */
>  	std	r11,GPR9(r1)
> @@ -295,6 +297,7 @@ END_BTB_FLUSH_SECTION
>  	 * Zero user registers to prevent influencing speculative execution
>  	 * state of kernel code.
>  	 */
> +	ZEROIZE_GPR(0)
>  	ZEROIZE_GPRS(5, 12)
>  	ZEROIZE_NVGPRS()
>  	bl	system_call_exception
> @@ -337,6 +340,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 */
> @@ -364,7 +368,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
Nicholas Piggin Sept. 20, 2022, 2:07 a.m. UTC | #2 
On Fri Sep 16, 2022 at 3:32 PM AEST, 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.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Update summary
> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
> V3 -> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
> V4 -> V5: Move to end of patch series.
> ---

Oh I would say change the title more like the interrupt patches to avoid
mentioning the restore/return because that's implicit.

e.g., "Clear user GPRs in syscall interrupt entry"

Could we have the one config option that does both this and the interrupt
zeroing?

Thanks,
Nick

>  arch/powerpc/kernel/interrupt_64.S | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index 16a1b44088e7..40147558e1a6 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 */
>  	SAVE_GPRS(3, 8, r1)
>  	/* Zero r9-r12, this should only be required when restoring all GPRs */
>  	std	r11,GPR9(r1)
> @@ -110,6 +110,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	 * Zero user registers to prevent influencing speculative execution
>  	 * state of kernel code.
>  	 */
> +	ZEROIZE_GPR(0)
>  	ZEROIZE_GPRS(5, 12)
>  	ZEROIZE_NVGPRS()
>  	bl	system_call_exception
> @@ -140,6 +141,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
>  
> @@ -243,7 +245,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 */
>  	SAVE_GPRS(3, 8, r1)
>  	/* Zero r9-r12, this should only be required when restoring all GPRs */
>  	std	r11,GPR9(r1)
> @@ -295,6 +297,7 @@ END_BTB_FLUSH_SECTION
>  	 * Zero user registers to prevent influencing speculative execution
>  	 * state of kernel code.
>  	 */
> +	ZEROIZE_GPR(0)
>  	ZEROIZE_GPRS(5, 12)
>  	ZEROIZE_NVGPRS()
>  	bl	system_call_exception
> @@ -337,6 +340,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 */
> @@ -364,7 +368,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
Rohan McLure Sept. 20, 2022, 4:54 a.m. UTC | #3 
> On 20 Sep 2022, at 12:03 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> On Fri Sep 16, 2022 at 3:32 PM AEST, 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.
>> 
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> V1 -> V2: Update summary
>> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
>> V3 -> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
>> V4 -> V5: Move to end of patch series.
>> ---
> 
> I think it looks okay. I'll have to take a better look with the series
> applied.


Your comments alerted me to the fact that general interrupt and syscalls
share their exit code in interrupt_return and its derivatives. Meaning
that disabling INTERRUPT_SANITIZE_REGISTERS also reverts restores of NVGPRS
to being optional, which makes it possible to clobber NVGPRS and then not
restore them. The cleanest way forward I belive is going to be to cause
INTERRUPT_SANITIZE_REGISTERS to perform sanitisation on all interrupt sources
rather than continuing with syscalls as their own special case. I’ll put
this out in a v6 soon.

> 
>> arch/powerpc/kernel/interrupt_64.S | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
>> index 16a1b44088e7..40147558e1a6 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 */
> 
> These two comment changes could go in your system_call_exception API
> change patch though.
> 
> Thanks,
> Nick
> 
>> 	SAVE_GPRS(3, 8, r1)
>> 	/* Zero r9-r12, this should only be required when restoring all GPRs */
>> 	std	r11,GPR9(r1)
>> @@ -110,6 +110,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> 	 * Zero user registers to prevent influencing speculative execution
>> 	 * state of kernel code.
>> 	 */
>> +	ZEROIZE_GPR(0)
>> 	ZEROIZE_GPRS(5, 12)
>> 	ZEROIZE_NVGPRS()
>> 	bl	system_call_exception
>> @@ -140,6 +141,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
>> 
>> @@ -243,7 +245,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 */
>> 	SAVE_GPRS(3, 8, r1)
>> 	/* Zero r9-r12, this should only be required when restoring all GPRs */
>> 	std	r11,GPR9(r1)
>> @@ -295,6 +297,7 @@ END_BTB_FLUSH_SECTION
>> 	 * Zero user registers to prevent influencing speculative execution
>> 	 * state of kernel code.
>> 	 */
>> +	ZEROIZE_GPR(0)
>> 	ZEROIZE_GPRS(5, 12)
>> 	ZEROIZE_NVGPRS()
>> 	bl	system_call_exception
>> @@ -337,6 +340,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 */
>> @@ -364,7 +368,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
Rohan McLure Sept. 21, 2022, 5:33 a.m. UTC | #4 
> On 20 Sep 2022, at 2:54 pm, Rohan McLure <rmclure@linux.ibm.com> wrote: 
> 
>> On 20 Sep 2022, at 12:03 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
>> 
>> On Fri Sep 16, 2022 at 3:32 PM AEST, 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.
>>> 
>>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>>> ---
>>> V1 -> V2: Update summary
>>> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
>>> V3 -> V4: Use ZEROIZE instead of NULLIFY. Clear r0 also.
>>> V4 -> V5: Move to end of patch series.
>>> ---
>> 
>> I think it looks okay. I'll have to take a better look with the series
>> applied.
> 
> 
> Your comments alerted me to the fact that general interrupt and syscalls
> share their exit code in interrupt_return and its derivatives. Meaning
> that disabling INTERRUPT_SANITIZE_REGISTERS also reverts restores of NVGPRS
> to being optional, which makes it possible to clobber NVGPRS and then not
> restore them. The cleanest way forward I belive is going to be to cause
> INTERRUPT_SANITIZE_REGISTERS to perform sanitisation on all interrupt sources
> rather than continuing with syscalls as their own special case. I’ll put
> this out in a v6 soon.

I think I managed to confuse myself here. Syscall handlers directly RFID, while
other interrupt sources share a common exit path.

>> 
>>> arch/powerpc/kernel/interrupt_64.S | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
>>> index 16a1b44088e7..40147558e1a6 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 */
>> 
>> These two comment changes could go in your system_call_exception API
>> change patch though.
>> 
>> Thanks,
>> Nick
>> 
>>> 	SAVE_GPRS(3, 8, r1)
>>> 	/* Zero r9-r12, this should only be required when restoring all GPRs */
>>> 	std	r11,GPR9(r1)
>>> @@ -110,6 +110,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>> 	 * Zero user registers to prevent influencing speculative execution
>>> 	 * state of kernel code.
>>> 	 */
>>> +	ZEROIZE_GPR(0)
>>> 	ZEROIZE_GPRS(5, 12)
>>> 	ZEROIZE_NVGPRS()
>>> 	bl	system_call_exception
>>> @@ -140,6 +141,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
>>> 
>>> @@ -243,7 +245,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 */
>>> 	SAVE_GPRS(3, 8, r1)
>>> 	/* Zero r9-r12, this should only be required when restoring all GPRs */
>>> 	std	r11,GPR9(r1)
>>> @@ -295,6 +297,7 @@ END_BTB_FLUSH_SECTION
>>> 	 * Zero user registers to prevent influencing speculative execution
>>> 	 * state of kernel code.
>>> 	 */
>>> +	ZEROIZE_GPR(0)
>>> 	ZEROIZE_GPRS(5, 12)
>>> 	ZEROIZE_NVGPRS()
>>> 	bl	system_call_exception
>>> @@ -337,6 +340,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 */
>>> @@ -364,7 +368,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
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index 16a1b44088e7..40147558e1a6 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 */
 	SAVE_GPRS(3, 8, r1)
 	/* Zero r9-r12, this should only be required when restoring all GPRs */
 	std	r11,GPR9(r1)
@@ -110,6 +110,7 @@  END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	 * Zero user registers to prevent influencing speculative execution
 	 * state of kernel code.
 	 */
+	ZEROIZE_GPR(0)
 	ZEROIZE_GPRS(5, 12)
 	ZEROIZE_NVGPRS()
 	bl	system_call_exception
@@ -140,6 +141,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
 
@@ -243,7 +245,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 */
 	SAVE_GPRS(3, 8, r1)
 	/* Zero r9-r12, this should only be required when restoring all GPRs */
 	std	r11,GPR9(r1)
@@ -295,6 +297,7 @@  END_BTB_FLUSH_SECTION
 	 * Zero user registers to prevent influencing speculative execution
 	 * state of kernel code.
 	 */
+	ZEROIZE_GPR(0)
 	ZEROIZE_GPRS(5, 12)
 	ZEROIZE_NVGPRS()
 	bl	system_call_exception
@@ -337,6 +340,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 */
@@ -364,7 +368,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)