[PATCH 1/2] Enable storage keys for radix - user mode execution

[PATCH 1/2] Enable storage keys for radix - user mode execution

[Balbir Singh]

ISA 3 defines new encoded access authority that allows instruction
access prevention in privileged mode and allows normal access
to problem state. This patch just enables IAMR (Instruction Authority
Mask Register), enabling AMR would require more work.

I've tested this with a buggy driver and a simple payload. The payload
is specific to the build I've tested.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/pgtable-radix.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index af897d9..9e25663 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -294,6 +294,27 @@ found:
 	return;
 }
 
+/*
+ * For radix page tables we setup, the IAMR values as follows
+ * IMAR = 0100...00 (key 0 is set to 1)
+ * AMOR = 1100....00 (Mask for key 0 is 11)
+ * AMR, UAMR, UAMOR are not affected
+ */
+static void __init radix_init_iamr(void)
+{
+	unsigned long iamr_mask = 0x4000000000000000;
+	unsigned long iamr = mfspr(SPRN_IAMR);
+
+	unsigned long amor_mask = 0xc000000000000000;
+	unsigned long amor = mfspr(SPRN_AMOR);
+
+	iamr |= iamr_mask;
+	amor |= amor_mask;
+
+	mtspr(SPRN_AMOR, amor);
+	mtspr(SPRN_IAMR, iamr);
+}
+
 void __init radix__early_init_mmu(void)
 {
 	unsigned long lpcr;
@@ -350,6 +371,7 @@ void __init radix__early_init_mmu(void)
 		radix_init_partition_table();
 	}
 
+	radix_init_iamr();
 	radix_init_pgtable();
 }
 
-- 
2.5.5

[PATCH 2/2] Detect instruction fetch denied and report

[Balbir Singh]

ISA 3 allows for prevention of instruction fetch and execution
of user mode pages. If such an error occurs, SRR1 bit 35
reports the error. We catch and report the error in do_page_fault()

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/fault.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index a4db22f..f162e77 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -404,6 +404,10 @@ good_area:
 		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
 		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
 			goto bad_area;
+#ifdef CONFIG_PPC_RADIX_MMU
+		if (radix_enabled() && regs->msr & PPC_BIT(35))
+			goto bad_area;
+#endif
 #ifdef CONFIG_PPC_STD_MMU
 		/*
 		 * protfault should only happen due to us
-- 
2.5.5

Re: [PATCH 2/2] Detect instruction fetch denied and report

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> ISA 3 allows for prevention of instruction fetch and execution
> of user mode pages. If such an error occurs, SRR1 bit 35
> reports the error. We catch and report the error in do_page_fault()
>

But what does the error mean ? A buggy application ? IIUC, it indicate a
buggy kernel isn't it ?. So should we kill the application or panic() ?


> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/fault.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a4db22f..f162e77 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -404,6 +404,10 @@ good_area:
>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;
> +#ifdef CONFIG_PPC_RADIX_MMU
> +		if (radix_enabled() && regs->msr & PPC_BIT(35))
> +			goto bad_area;
> +#endif
>  #ifdef CONFIG_PPC_STD_MMU
>  		/*
>  		 * protfault should only happen due to us

-aneesh

Re: [PATCH 2/2] Detect instruction fetch denied and report

[Balbir Singh]

On Mon, Aug 22, 2016 at 11:35:36AM +0530, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
> > ISA 3 allows for prevention of instruction fetch and execution
> > of user mode pages. If such an error occurs, SRR1 bit 35
> > reports the error. We catch and report the error in do_page_fault()
> >
> 
> But what does the error mean ? A buggy application ? IIUC, it indicate a
> buggy kernel isn't it ?. So should we kill the application or panic() ?
>

We oops.. basically we get a kernel fault. We handle it the same way
we handle other kernel faults.
 
> 
> > Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> > ---
> >  arch/powerpc/mm/fault.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index a4db22f..f162e77 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -404,6 +404,10 @@ good_area:
> >  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
> >  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
> >  			goto bad_area;
> > +#ifdef CONFIG_PPC_RADIX_MMU
> > +		if (radix_enabled() && regs->msr & PPC_BIT(35))
> > +			goto bad_area;
> > +#endif
> >  #ifdef CONFIG_PPC_STD_MMU
> >  		/*
> >  		 * protfault should only happen due to us
> 
> -aneesh
> 

Re: [2/2] Detect instruction fetch denied and report

[Michael Ellerman]

On Mon, 2016-22-08 at 01:56:57 UTC, Balbir Singh wrote:
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a4db22f..f162e77 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -404,6 +404,10 @@ good_area:
>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;
> +#ifdef CONFIG_PPC_RADIX_MMU

We shouldn't need the #ifdef, radix_enabled() will be false.

> +		if (radix_enabled() && regs->msr & PPC_BIT(35))
> +			goto bad_area;

Is it really architected as radix only?

Personally I dislike PPC_BIT(), I'd rather you just used 0x10000000. That way
when I'm staring at a register dump I have some chance of spotting that mask.

Also brackets around the bitwise & would make me feel more comfortable.

cheers

Re: [2/2] Detect instruction fetch denied and report

[Balbir Singh]

On 20/09/16 16:35, Michael Ellerman wrote:
> On Mon, 2016-22-08 at 01:56:57 UTC, Balbir Singh wrote:
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index a4db22f..f162e77 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -404,6 +404,10 @@ good_area:
>>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>>  			goto bad_area;
>> +#ifdef CONFIG_PPC_RADIX_MMU
> 
> We shouldn't need the #ifdef, radix_enabled() will be false.
> 
>> +		if (radix_enabled() && regs->msr & PPC_BIT(35))
>> +			goto bad_area;
> 
> Is it really architected as radix only?
> 
> Personally I dislike PPC_BIT(), I'd rather you just used 0x10000000. That way
> when I'm staring at a register dump I have some chance of spotting that mask.
> 
> Also brackets around the bitwise & would make me feel more comfortable.
> 
> cheers
> 


Will make the changes and submit

Balbir

Re: [PATCH 1/2] Enable storage keys for radix - user mode execution

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> ISA 3 defines new encoded access authority that allows instruction
> access prevention in privileged mode and allows normal access
> to problem state. This patch just enables IAMR (Instruction Authority
> Mask Register), enabling AMR would require more work.
>

Don't we need to do them in hypervisor mode. Ie, the hypervisor setup
things such that guest privileged mode cannot execute guest userspace.

> I've tested this with a buggy driver and a simple payload. The payload
> is specific to the build I've tested.
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/pgtable-radix.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index af897d9..9e25663 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -294,6 +294,27 @@ found:
>  	return;
>  }
>
> +/*
> + * For radix page tables we setup, the IAMR values as follows
> + * IMAR = 0100...00 (key 0 is set to 1)
> + * AMOR = 1100....00 (Mask for key 0 is 11)
> + * AMR, UAMR, UAMOR are not affected
> + */
> +static void __init radix_init_iamr(void)
> +{
> +	unsigned long iamr_mask = 0x4000000000000000;
> +	unsigned long iamr = mfspr(SPRN_IAMR);
> +
> +	unsigned long amor_mask = 0xc000000000000000;
> +	unsigned long amor = mfspr(SPRN_AMOR);

Isn't AMOR hypervisor privileged ?.

> +
> +	iamr |= iamr_mask;
> +	amor |= amor_mask;
> +
> +	mtspr(SPRN_AMOR, amor);
> +	mtspr(SPRN_IAMR, iamr);
> +}
> +
>  void __init radix__early_init_mmu(void)
>  {
>  	unsigned long lpcr;
> @@ -350,6 +371,7 @@ void __init radix__early_init_mmu(void)
>  		radix_init_partition_table();
>  	}
>
> +	radix_init_iamr();
>  	radix_init_pgtable();
>  }
>
> -- 
> 2.5.5

Re: [PATCH 1/2] Enable storage keys for radix - user mode execution

[Balbir Singh]

On Mon, Aug 22, 2016 at 11:32:44AM +0530, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
> > ISA 3 defines new encoded access authority that allows instruction
> > access prevention in privileged mode and allows normal access
> > to problem state. This patch just enables IAMR (Instruction Authority
> > Mask Register), enabling AMR would require more work.
> >
> 
> Don't we need to do them in hypervisor mode. Ie, the hypervisor setup
> things such that guest privileged mode cannot execute guest userspace.

Yes, true!

> 
> > I've tested this with a buggy driver and a simple payload. The payload
> > is specific to the build I've tested.
> >
> > Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> > ---
> >  arch/powerpc/mm/pgtable-radix.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> > index af897d9..9e25663 100644
> > --- a/arch/powerpc/mm/pgtable-radix.c
> > +++ b/arch/powerpc/mm/pgtable-radix.c
> > @@ -294,6 +294,27 @@ found:
> >  	return;
> >  }
> >
> > +/*
> > + * For radix page tables we setup, the IAMR values as follows
> > + * IMAR = 0100...00 (key 0 is set to 1)
> > + * AMOR = 1100....00 (Mask for key 0 is 11)
> > + * AMR, UAMR, UAMOR are not affected
> > + */
> > +static void __init radix_init_iamr(void)
> > +{
> > +	unsigned long iamr_mask = 0x4000000000000000;
> > +	unsigned long iamr = mfspr(SPRN_IAMR);
> > +
> > +	unsigned long amor_mask = 0xc000000000000000;
> > +	unsigned long amor = mfspr(SPRN_AMOR);
> 
> Isn't AMOR hypervisor privileged ?.
>

You are right, I should split the AMOR initialization
to be HV only. IAMR is saved/restored during guest exit/entry.

So, the AMOR initialization needs to move.

I'll post a v2

Balbir Singh.