LinuxPPC-Dev Archive on lore.kernel.org
 help / Atom feed
* [PATCH] powerpc/powernv/idle: Restore IAMR after idle
@ 2019-02-06  6:28 Russell Currey
  2019-02-07  4:29 ` Michael Ellerman
  2019-02-07  5:08 ` Nicholas Piggin
  0 siblings, 2 replies; 9+ 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	[flat|nested] 9+ 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
  1 sibling, 1 reply; 9+ 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] 9+ 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
  1 sibling, 2 replies; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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
  0 siblings, 0 replies; 9+ 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] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ 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

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org linuxppc-dev@archiver.kernel.org
	public-inbox-index linuxppc-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox