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