linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] powerpc/powernv/idle: Restore IAMR after idle
@ 2019-02-06  6:28 Russell Currey
  2019-02-07  4:29 ` Michael Ellerman
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Russell Currey @ 2019-02-06  6:28 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Russell Currey

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: <stable@vger.kernel.org>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 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)
+
 	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)
+
 	ld	r4,PACAKMSR(r13)
 	ld	r5,_NIP(r1)
 	ld	r6,_CCR(r1)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-06  6:28 [PATCH] powerpc/powernv/idle: Restore IAMR after idle Russell Currey
@ 2019-02-07  4:29 ` Michael Ellerman
  2019-02-07  6:28   ` Russell Currey
  2019-02-07  5:08 ` Nicholas Piggin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Michael Ellerman @ 2019-02-07  4:29 UTC (permalink / raw)
  To: Russell Currey, linuxppc-dev

Russell Currey <ruscur@russell.cc> writes:
> 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")

Don't word wrap the fixes line please.

> Cc: <stable@vger.kernel.org>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>  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;
>  };

We don't actually need to put this in the paca anymore.

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

We have space for a full pt_regs on the stack, and we're not using it
all.

We don't have a specific slot for the IAMR (we may want to in future),
but for now you could follow the time-honoured tradition of (ab)using
the _DAR slot, with an appropriate comment.

cheers

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-06  6:28 [PATCH] powerpc/powernv/idle: Restore IAMR after idle Russell Currey
  2019-02-07  4:29 ` Michael Ellerman
@ 2019-02-07  5:08 ` Nicholas Piggin
  2019-02-07  6:33   ` Russell Currey
  2019-02-08  1:04   ` Michael Ellerman
  2019-02-20  7:15 ` Akshay Adiga
  2019-02-20  8:58 ` Akshay Adiga
  3 siblings, 2 replies; 15+ messages in thread
From: Nicholas Piggin @ 2019-02-07  5:08 UTC (permalink / raw)
  To: linuxppc-dev, Russell Currey

Russell Currey's on February 6, 2019 4:28 pm:
> 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: <stable@vger.kernel.org>
> Signed-off-by: Russell Currey <ruscur@russell.cc>

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?

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

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

Thanks,
Nick

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-07  4:29 ` Michael Ellerman
@ 2019-02-07  6:28   ` Russell Currey
  0 siblings, 0 replies; 15+ messages in thread
From: Russell Currey @ 2019-02-07  6:28 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev

On Thu, 2019-02-07 at 15:29 +1100, Michael Ellerman wrote:
> Russell Currey <ruscur@russell.cc> writes:
> > 
> > Fixes: 3b10d0095a1e ("powerpc/mm/radix: Prevent kernel execution of
> > user
> > space")
> 
> Don't word wrap the fixes line please.

My bad, will teach my editor :)

> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> >  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;
> >  };
> 
> We don't actually need to put this in the paca anymore.
> 
> > 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)
> 
> We have space for a full pt_regs on the stack, and we're not using it
> all.
> 
> We don't have a specific slot for the IAMR (we may want to in
> future),
> but for now you could follow the time-honoured tradition of (ab)using
> the _DAR slot, with an appropriate comment.

I read this, then did it, and when writing the comment I thought I was
clever using "(ab)use".  I then reread this and realised I just
subconsciously stole it.

Thanks for the review.

> cheers


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-07  5:08 ` Nicholas Piggin
@ 2019-02-07  6:33   ` Russell Currey
  2019-02-07 16:37     ` Thiago Jung Bauermann
  2019-02-08  1:04   ` Michael Ellerman
  1 sibling, 1 reply; 15+ messages in thread
From: Russell Currey @ 2019-02-07  6:33 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev

On Thu, 2019-02-07 at 15:08 +1000, Nicholas Piggin wrote:
> Russell Currey's on February 6, 2019 4:28 pm:
> > 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: <stable@vger.kernel.org>
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> 
> 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?

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

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-07  6:33   ` Russell Currey
@ 2019-02-07 16:37     ` Thiago Jung Bauermann
  2019-02-07 22:38       ` Russell Currey
  0 siblings, 1 reply; 15+ messages in thread
From: Thiago Jung Bauermann @ 2019-02-07 16:37 UTC (permalink / raw)
  To: Russell Currey; +Cc: linuxppc-dev, Nicholas Piggin


Russell Currey <ruscur@russell.cc> writes:

> On Thu, 2019-02-07 at 15:08 +1000, Nicholas Piggin wrote:
>> Russell Currey's on February 6, 2019 4:28 pm:
>> > 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: <stable@vger.kernel.org>
>> > Signed-off-by: Russell Currey <ruscur@russell.cc>
>>
>> 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?

On hash it's used for memory protection keys (code is in
arch/powerpc/mm/pkeys.c). The kernel doesn't use protection keys, but
userspace apps may use it explicitly via specific syscalls
(pkey_alloc(), pkey_mprotect, pkey_free()).

Also, the kernel may use a protection key if the process does an
mmap(PROT_EXEC).

--
Thiago Jung Bauermann
IBM Linux Technology Center


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-07 16:37     ` Thiago Jung Bauermann
@ 2019-02-07 22:38       ` Russell Currey
  0 siblings, 0 replies; 15+ messages in thread
From: Russell Currey @ 2019-02-07 22:38 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: linuxppc-dev, Nicholas Piggin

On Thu, 2019-02-07 at 14:37 -0200, Thiago Jung Bauermann wrote:
> Russell Currey <ruscur@russell.cc> writes:
> > On Thu, 2019-02-07 at 15:08 +1000, Nicholas Piggin wrote:
> > > Russell Currey's on February 6, 2019 4:28 pm:
> > > > 
> > > > Fixes: 3b10d0095a1e ("powerpc/mm/radix: Prevent kernel
> > > > execution of
> > > > user
> > > > space")
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > > 
> > > 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?
> 
> On hash it's used for memory protection keys (code is in
> arch/powerpc/mm/pkeys.c). The kernel doesn't use protection keys, but
> userspace apps may use it explicitly via specific syscalls
> (pkey_alloc(), pkey_mprotect, pkey_free()).
> 
> Also, the kernel may use a protection key if the process does an
> mmap(PROT_EXEC).

I don't understand how this would work, though - in this case we
wouldn't know on boot if we were going to use the IAMR or not.  On
radix (unless booting with nosmep) it would always be used, but on hash
it seems it depends on what userspace does.  How exactly would a
runtime toggle of "IAMR in use" work?

With a CONFIG option it would have to depend on PPC_MEM_KEYS ||
PPC_RADIX_MMU, but those are (pretty much) always going to be on in P8
and P9, which I already check for.

> 
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-07  5:08 ` Nicholas Piggin
  2019-02-07  6:33   ` Russell Currey
@ 2019-02-08  1:04   ` Michael Ellerman
  2019-02-19  4:21     ` Nicholas Piggin
  1 sibling, 1 reply; 15+ messages in thread
From: Michael Ellerman @ 2019-02-08  1:04 UTC (permalink / raw)
  To: Nicholas Piggin, linuxppc-dev, Russell Currey

Nicholas Piggin <npiggin@gmail.com> writes:
> Russell Currey's on February 6, 2019 4:28 pm:
>> 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: <stable@vger.kernel.org>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>
> Good catch and debugging. This really should be a quirk, we don't want 
> to have to restore this thing on a thread switch.

I'm not sure I follow. We don't context switch it on Radix, but we do
on hash if pkeys are enabled.

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

We'll always be using it with Radix, and we might be using it for pkeys
on hash, unless pkeys are compiled out. But I don't really expect anyone
to be running with pkeys compiled out.

So I think the only case we could optimise is that we're on hash and the
current thread has an IAMR of 0, then we could just not restore
(assuming we come out of idle with IAMR=0).

But maybe I'm not understanding.

cheers

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-08  1:04   ` Michael Ellerman
@ 2019-02-19  4:21     ` Nicholas Piggin
  2019-02-20  6:04       ` Akshay Adiga
  0 siblings, 1 reply; 15+ messages in thread
From: Nicholas Piggin @ 2019-02-19  4:21 UTC (permalink / raw)
  To: linuxppc-dev, Michael Ellerman, Russell Currey

Michael Ellerman's on February 8, 2019 11:04 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Russell Currey's on February 6, 2019 4:28 pm:
>>> 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: <stable@vger.kernel.org>
>>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>>
>> Good catch and debugging. This really should be a quirk, we don't want 
>> to have to restore this thing on a thread switch.
> 
> I'm not sure I follow. We don't context switch it on Radix, but we do
> on hash if pkeys are enabled.

Badly worded, I mean a hardware quirk. It should follow thread
switches. Still, avoiding it for the no-loss case is better than
nothing. We can just revisit it as an optimization if future
hardware does not require the restore.

>> Can we put it under a CONFIG option if we're not using IAMR?
> 
> We'll always be using it with Radix, and we might be using it for pkeys
> on hash, unless pkeys are compiled out. But I don't really expect anyone
> to be running with pkeys compiled out.
> 
> So I think the only case we could optimise is that we're on hash and the
> current thread has an IAMR of 0, then we could just not restore
> (assuming we come out of idle with IAMR=0).
> 
> But maybe I'm not understanding.

Nah it sounds like more trouble than it's worth in that case.

Thanks,
Nick

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-19  4:21     ` Nicholas Piggin
@ 2019-02-20  6:04       ` Akshay Adiga
  2019-02-20 11:18         ` Russell Currey
  0 siblings, 1 reply; 15+ messages in thread
From: Akshay Adiga @ 2019-02-20  6:04 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: linuxppc-dev

On Tue, Feb 19, 2019 at 02:21:04PM +1000, Nicholas Piggin wrote:
> Michael Ellerman's on February 8, 2019 11:04 am:
> > Nicholas Piggin <npiggin@gmail.com> writes:
> >> Russell Currey's on February 6, 2019 4:28 pm:
> >>> 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: <stable@vger.kernel.org>
> >>> Signed-off-by: Russell Currey <ruscur@russell.cc>
> >>
> >> Good catch and debugging. This really should be a quirk, we don't want 
> >> to have to restore this thing on a thread switch.
> > 
> > I'm not sure I follow. We don't context switch it on Radix, but we do
> > on hash if pkeys are enabled.
> 
> Badly worded, I mean a hardware quirk. It should follow thread
> switches. Still, avoiding it for the no-loss case is better than
> nothing. We can just revisit it as an optimization if future
> hardware does not require the restore.

Apparently, the POWER9 Processor User’s Manual v2.0 documents that
IAMR can be lost, and that is not just the end.

Pasting excerpt from "Section 23.5.9.2 State Loss and Restoration,Page 309"

  On the POWER9 core, the only state that can be lost for
  Stop levels less than four, when PSSCR[ESL] = ‘1’ are the
  following SPRs: CR, FPSCR, VSCR, XER, DSCR, AMR, IAMR, UAMOR,
  AMOR, DAWR, DAWRX.

My observation is that AMOR is being used in kernel as of today
and AMOR is also lost (recreated in similar scenarios where
IAMR is lost).


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-06  6:28 [PATCH] powerpc/powernv/idle: Restore IAMR after idle Russell Currey
  2019-02-07  4:29 ` Michael Ellerman
  2019-02-07  5:08 ` Nicholas Piggin
@ 2019-02-20  7:15 ` Akshay Adiga
  2019-02-20 11:25   ` Russell Currey
  2019-02-20  8:58 ` Akshay Adiga
  3 siblings, 1 reply; 15+ messages in thread
From: Akshay Adiga @ 2019-02-20  7:15 UTC (permalink / raw)
  To: Russell Currey; +Cc: linuxppc-dev

On Wed, Feb 06, 2019 at 05:28:37PM +1100, Russell Currey wrote:
> 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: <stable@vger.kernel.org>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>  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)
> +

Are we trying to add for both power8 and power9 ?
power9 would be CPU_FTR_ARCH_300.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-06  6:28 [PATCH] powerpc/powernv/idle: Restore IAMR after idle Russell Currey
                   ` (2 preceding siblings ...)
  2019-02-20  7:15 ` Akshay Adiga
@ 2019-02-20  8:58 ` Akshay Adiga
  2019-02-20 11:20   ` Russell Currey
  3 siblings, 1 reply; 15+ messages in thread
From: Akshay Adiga @ 2019-02-20  8:58 UTC (permalink / raw)
  To: Russell Currey; +Cc: linuxppc-dev

On Wed, Feb 06, 2019 at 05:28:37PM +1100, Russell Currey wrote:
> 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: <stable@vger.kernel.org>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>  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)
> +
>  	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)
> +

pnv_wakeup_noloss gets called from two paths:
1) cpu wakes up at 0x100  (ESL=EC=1)
2) cpu wakes up at next instruction  (ESL=EC=0)

We know for the fact that its not lost with ESL=EC=0 , so we
should put it somewhere else.

I would like to put the restore code in pnv_restore_hyp_resource_arch300
it already has some work arounds we can add another.

By this time we cr3 has comparison of SRR1[46:47] with 2

  542 pnv_restore_hyp_resource_arch300:
  543         /*
  544          * Workaround for POWER9, if we lost resources, the ERAT
  545          * might have been mixed up and needs flushing. We also need
  546          * to reload MMCR0 (see comment above). We also need to set
  547          * then clear bit 60 in MMCRA to ensure the PMU starts running.
  548          */
  549         blt     cr3,1f
  550 BEGIN_FTR_SECTION
  551         PPC_INVALIDATE_ERAT
  552         ld      r1,PACAR1(r13)
  553         ld      r4,_MMCR0(r1)
  554         mtspr   SPRN_MMCR0,r4
  555 END_FTR_SECTION_IFCLR(CPU_FTR_POWER9_DD2_1)
  556         mfspr   r4,SPRN_MMCRA
  557         ori     r4,r4,(1 << (63-60))
  558         mtspr   SPRN_MMCRA,r4
  559         xori    r4,r4,(1 << (63-60))
  560         mtspr   SPRN_MMCRA,r4
+ 561 BEGIN_FTR_SECTION
+ 562         ld      r4,STOP_IAMR(r13)
+ 563         mtspr   SPRN_IAMR,r4
+ 564 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
  565 1:
  566         /* 

I have a patchset to handle both AMOR and IAMR,
need to test it on power8 before posting.


>  	ld	r4,PACAKMSR(r13)
>  	ld	r5,_NIP(r1)
>  	ld	r6,_CCR(r1)
> -- 
> 2.20.1
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-20  6:04       ` Akshay Adiga
@ 2019-02-20 11:18         ` Russell Currey
  0 siblings, 0 replies; 15+ messages in thread
From: Russell Currey @ 2019-02-20 11:18 UTC (permalink / raw)
  To: Akshay Adiga, Nicholas Piggin; +Cc: linuxppc-dev

On Wed, 2019-02-20 at 11:34 +0530, Akshay Adiga wrote:
> On Tue, Feb 19, 2019 at 02:21:04PM +1000, Nicholas Piggin wrote:
> > Michael Ellerman's on February 8, 2019 11:04 am:
> > > Nicholas Piggin <npiggin@gmail.com> writes:
> > > > Russell Currey's on February 6, 2019 4:28 pm:
> > > > > 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: <stable@vger.kernel.org>
> > > > > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > > > 
> > > > Good catch and debugging. This really should be a quirk, we
> > > > don't want 
> > > > to have to restore this thing on a thread switch.
> > > 
> > > I'm not sure I follow. We don't context switch it on Radix, but
> > > we do
> > > on hash if pkeys are enabled.
> > 
> > Badly worded, I mean a hardware quirk. It should follow thread
> > switches. Still, avoiding it for the no-loss case is better than
> > nothing. We can just revisit it as an optimization if future
> > hardware does not require the restore.
> 
> Apparently, the POWER9 Processor User’s Manual v2.0 documents that
> IAMR can be lost, and that is not just the end.
> 
> Pasting excerpt from "Section 23.5.9.2 State Loss and
> Restoration,Page 309"
> 
>   On the POWER9 core, the only state that can be lost for
>   Stop levels less than four, when PSSCR[ESL] = ‘1’ are the
>   following SPRs: CR, FPSCR, VSCR, XER, DSCR, AMR, IAMR, UAMOR,
>   AMOR, DAWR, DAWRX.
> 
> My observation is that AMOR is being used in kernel as of today
> and AMOR is also lost (recreated in similar scenarios where
> IAMR is lost).
> 

I can add AMOR to this patch (or you can send a patch, either way).


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-20  8:58 ` Akshay Adiga
@ 2019-02-20 11:20   ` Russell Currey
  0 siblings, 0 replies; 15+ messages in thread
From: Russell Currey @ 2019-02-20 11:20 UTC (permalink / raw)
  To: Akshay Adiga; +Cc: linuxppc-dev

On Wed, 2019-02-20 at 14:28 +0530, Akshay Adiga wrote:
> On Wed, Feb 06, 2019 at 05:28:37PM +1100, Russell Currey wrote:
> > 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: <stable@vger.kernel.org>
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> >  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)
> > +
> >  	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)
> > +
> 
> pnv_wakeup_noloss gets called from two paths:
> 1) cpu wakes up at 0x100  (ESL=EC=1)
> 2) cpu wakes up at next instruction  (ESL=EC=0)
> 

In v2 I drop it from noloss, is that still correct?

> We know for the fact that its not lost with ESL=EC=0 , so we
> should put it somewhere else.
> 
> I would like to put the restore code in
> pnv_restore_hyp_resource_arch300
> it already has some work arounds we can add another.
> 
> By this time we cr3 has comparison of SRR1[46:47] with 2
> 
>   542 pnv_restore_hyp_resource_arch300:
>   543         /*
>   544          * Workaround for POWER9, if we lost resources, the
> ERAT
>   545          * might have been mixed up and needs flushing. We also
> need
>   546          * to reload MMCR0 (see comment above). We also need to
> set
>   547          * then clear bit 60 in MMCRA to ensure the PMU starts
> running.
>   548          */
>   549         blt     cr3,1f
>   550 BEGIN_FTR_SECTION
>   551         PPC_INVALIDATE_ERAT
>   552         ld      r1,PACAR1(r13)
>   553         ld      r4,_MMCR0(r1)
>   554         mtspr   SPRN_MMCR0,r4
>   555 END_FTR_SECTION_IFCLR(CPU_FTR_POWER9_DD2_1)
>   556         mfspr   r4,SPRN_MMCRA
>   557         ori     r4,r4,(1 << (63-60))
>   558         mtspr   SPRN_MMCRA,r4
>   559         xori    r4,r4,(1 << (63-60))
>   560         mtspr   SPRN_MMCRA,r4
> + 561 BEGIN_FTR_SECTION
> + 562         ld      r4,STOP_IAMR(r13)
> + 563         mtspr   SPRN_IAMR,r4
> + 564 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
>   565 1:
>   566         /* 
> 
> I have a patchset to handle both AMOR and IAMR,
> need to test it on power8 before posting.

If you think that's better, go ahead.

> 
> 
> >  	ld	r4,PACAKMSR(r13)
> >  	ld	r5,_NIP(r1)
> >  	ld	r6,_CCR(r1)
> > -- 
> > 2.20.1
> > 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
  2019-02-20  7:15 ` Akshay Adiga
@ 2019-02-20 11:25   ` Russell Currey
  0 siblings, 0 replies; 15+ messages in thread
From: Russell Currey @ 2019-02-20 11:25 UTC (permalink / raw)
  To: Akshay Adiga, Michael Ellerman; +Cc: linuxppc-dev

On Wed, 2019-02-20 at 12:45 +0530, Akshay Adiga wrote:
> On Wed, Feb 06, 2019 at 05:28:37PM +1100, Russell Currey wrote:
> > 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: <stable@vger.kernel.org>
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> >  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)
> > +
> 
> Are we trying to add for both power8 and power9 ?
> power9 would be CPU_FTR_ARCH_300.
> 

If I recall correctly I had this at P9 only but for some reason I
changed it - the reason is probably just that I had it confused for
something else.  Michael can you confirm this should be P9 only?


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-02-20 11:28 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-06  6:28 [PATCH] powerpc/powernv/idle: Restore IAMR after idle Russell Currey
2019-02-07  4:29 ` Michael Ellerman
2019-02-07  6:28   ` Russell Currey
2019-02-07  5:08 ` Nicholas Piggin
2019-02-07  6:33   ` Russell Currey
2019-02-07 16:37     ` Thiago Jung Bauermann
2019-02-07 22:38       ` Russell Currey
2019-02-08  1:04   ` Michael Ellerman
2019-02-19  4:21     ` Nicholas Piggin
2019-02-20  6:04       ` Akshay Adiga
2019-02-20 11:18         ` Russell Currey
2019-02-20  7:15 ` Akshay Adiga
2019-02-20 11:25   ` Russell Currey
2019-02-20  8:58 ` Akshay Adiga
2019-02-20 11:20   ` Russell Currey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).