Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle

On Thu, 2019-02-07 at 15:08 +1000, Nicholas Piggin wrote:

Russell Currey's on February 6, 2019 4:28 pm:

quoted

Without restoring the IAMR after idle, execution prevention on
POWER9
with Radix MMU is overwritten and the kernel can freely execute
userspace without
faulting.

This is necessary when returning from any stop state that modifies
user
state, as well as hypervisor state.

To test how this fails without this patch, load the lkdtm driver
and
do the following:

   echo EXEC_USERSPACE > /sys/kernel/debug/provoke-crash/DIRECT

which won't fault, then boot the kernel with powersave=off, where
it
will fault.  Applying this patch will fix this.

Fixes: 3b10d0095a1e ("powerpc/mm/radix: Prevent kernel execution of
user
space")
Cc: <redacted>
Signed-off-by: Russell Currey <redacted>

Good catch and debugging. This really should be a quirk, we don't
want 
to have to restore this thing on a thread switch.

Can we put it under a CONFIG option if we're not using IAMR?

I don't exactly know when we do or don't use the IAMR (since the only
thing I've used it for is radix).  When wouldn't we care about
restoring it on hash?

quoted

---
 arch/powerpc/include/asm/cpuidle.h |  1 +
 arch/powerpc/kernel/asm-offsets.c  |  1 +
 arch/powerpc/kernel/idle_book3s.S  | 20 ++++++++++++++++++++
 3 files changed, 22 insertions(+)

diff --git a/arch/powerpc/include/asm/cpuidle.h

b/arch/powerpc/include/asm/cpuidle.h
index 43e5f31fe64d..ad67dbe59498 100644

--- a/arch/powerpc/include/asm/cpuidle.h
+++ b/arch/powerpc/include/asm/cpuidle.h

@@ -77,6 +77,7 @@ struct stop_sprs {
 	u64 mmcr1;
 	u64 mmcr2;
 	u64 mmcra;
+	u64 iamr;
 };
 
 #define PNV_IDLE_NAME_LEN    16

diff --git a/arch/powerpc/kernel/asm-offsets.c

b/arch/powerpc/kernel/asm-offsets.c
index 9ffc72ded73a..10e0314c2b0d 100644

--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c

@@ -774,6 +774,7 @@ int main(void)
 	STOP_SPR(STOP_MMCR1, mmcr1);
 	STOP_SPR(STOP_MMCR2, mmcr2);
 	STOP_SPR(STOP_MMCRA, mmcra);
+	STOP_SPR(STOP_IAMR, iamr);
 #endif
 
 	DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);

diff --git a/arch/powerpc/kernel/idle_book3s.S

b/arch/powerpc/kernel/idle_book3s.S
index 7f5ac2e8581b..bb4f552f6c7e 100644

--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S

@@ -200,6 +200,12 @@ pnv_powersave_common:
 	/* Continue saving state */
 	SAVE_GPR(2, r1)
 	SAVE_NVGPRS(r1)
+
+BEGIN_FTR_SECTION
+	mfspr	r5, SPRN_IAMR
+	std	r5, STOP_IAMR(r13)
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
+
 	mfcr	r5
 	std	r5,_CCR(r1)
 	std	r1,PACAR1(r13)

@@ -924,6 +930,13 @@ BEGIN_FTR_SECTION
 END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
 	REST_NVGPRS(r1)
 	REST_GPR(2, r1)
+
+BEGIN_FTR_SECTION
+	ld	r4, STOP_IAMR(r13)
+	mtspr	SPRN_IAMR, r4
+	isync
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)

Sigh, good old isync. Suspect you'll get away without it, mtmsrd L=0 
just below is architecturally guaranteeing a CSI, so just add a
comment 
there, might save a flush.

Makes sense, I wanted to be super safe with this, will drop the isync
and try to execute userspace nonstop and see if there are any (non)
failures.

quoted

+
 	ld	r4,PACAKMSR(r13)
 	ld	r5,_LINK(r1)
 	ld	r6,_CCR(r1)

@@ -946,6 +959,13 @@ pnv_wakeup_noloss:
 BEGIN_FTR_SECTION
 	CHECK_HMI_INTERRUPT
 END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
+
+BEGIN_FTR_SECTION
+	ld	r4, STOP_IAMR(r13)
+	mtspr	SPRN_IAMR, r4
+	isync
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)

For the noloss part, it should mean really nothing lost including
GPRs, 
so I think IAMR *should* be okay here.

Sweet, will drop.

Thanks for the review!

Thanks,
Nick