[powerpc v6 0/3] Enable storage keys for radix

[powerpc v6 0/3] Enable storage keys for radix

[Balbir Singh]

The first patch sets up AMOR in hypervisor mode. AMOR
needs to be setup before IAMR (details of AMOR/IAMR in
each patch). The second patch enables detection of exceptions
generated due to instruction fetch violations caused
and OOPSs' the task. The third patch enables IAMR for
both hypervisor and guest kernels.

IAMR in radix mode, prevents the kernel from executing
code from user mode pages.

I've tested with patch series with a sample hack and
payload.

Chris Smart helped with the series, reviewing and
providing valuable feedback

Changelog from previous post
  Implement review comments and suggestions

Balbir Singh (3):
  powerpc:Setup AMOR in HV mode
  powerpc/mm/radix:Detect instruction fetch denied and report
  powerpc:Enable storage keys for radix - user mode execution

 arch/powerpc/mm/fault.c         |  8 ++++++++
 arch/powerpc/mm/pgtable-radix.c | 45 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

-- 
2.5.5

[powerpc v6 1/3] Setup AMOR in HV mode

[Balbir Singh]

AMOR should be setup in HV mode, we set it up once
and let the generic kernel handle IAMR. This patch is
used to enable storage keys in a following patch as
defined in ISA 3. We don't setup AMOR in DD1, since we
can't setup IAMR in DD1 (bits have to be 0). If we setup
AMOR some other code could potentially try to set IAMR
(guest kernel for example).

Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/pgtable-radix.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index ed7bddc..7aa104d 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -320,6 +320,27 @@ static void update_hid_for_radix(void)
 		cpu_relax();
 }
 
+/*
+ * In HV mode, we init AMOR so that the hypervisor
+ * and guest can setup IMAR, enable key 0 and set
+ * it to 1
+ * AMOR = 1100....00 (Mask for key 0 is 11)
+ */
+static void radix_init_amor(void)
+{
+	unsigned long amor_mask = 0xc000000000000000;
+	unsigned long amor;
+
+	/*
+	 * The amor bits are unused in DD1
+	 */
+	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
+		return;
+
+	amor = amor_mask;
+	mtspr(SPRN_AMOR, amor);
+}
+
 void __init radix__early_init_mmu(void)
 {
 	unsigned long lpcr;
@@ -376,6 +397,7 @@ void __init radix__early_init_mmu(void)
 		lpcr = mfspr(SPRN_LPCR);
 		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
 		radix_init_partition_table();
+		radix_init_amor();
 	}
 
 	radix_init_pgtable();
@@ -393,6 +415,7 @@ void radix__early_init_mmu_secondary(void)
 
 		mtspr(SPRN_PTCR,
 		      __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
+		radix_init_amor();
 	}
 }
 
-- 
2.5.5

Re: [powerpc v6 1/3] Setup AMOR in HV mode

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> AMOR should be setup in HV mode, we set it up once
> and let the generic kernel handle IAMR. This patch is
> used to enable storage keys in a following patch as
> defined in ISA 3. We don't setup AMOR in DD1, since we
> can't setup IAMR in DD1 (bits have to be 0). If we setup
> AMOR some other code could potentially try to set IAMR
> (guest kernel for example).
>
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/pgtable-radix.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index ed7bddc..7aa104d 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -320,6 +320,27 @@ static void update_hid_for_radix(void)
>  		cpu_relax();
>  }
>  
> +/*
> + * In HV mode, we init AMOR so that the hypervisor
> + * and guest can setup IMAR, enable key 0 and set
> + * it to 1
> + * AMOR = 1100....00 (Mask for key 0 is 11)
> + */
> +static void radix_init_amor(void)
> +{
> +	unsigned long amor_mask = 0xc000000000000000;
> +	unsigned long amor;

I guess michael mentioned this in another email, why two variables ?

> +
> +	/*
> +	 * The amor bits are unused in DD1
> +	 */
> +	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
> +		return;
> +
> +	amor = amor_mask;
> +	mtspr(SPRN_AMOR, amor);
> +}
> +
>  void __init radix__early_init_mmu(void)
>  {
>  	unsigned long lpcr;
> @@ -376,6 +397,7 @@ void __init radix__early_init_mmu(void)
>  		lpcr = mfspr(SPRN_LPCR);
>  		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>  		radix_init_partition_table();
> +		radix_init_amor();
>  	}
>  
>  	radix_init_pgtable();
> @@ -393,6 +415,7 @@ void radix__early_init_mmu_secondary(void)
>  
>  		mtspr(SPRN_PTCR,
>  		      __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
> +		radix_init_amor();
>  	}
>  }
>  
> -- 
> 2.5.5

Re: [powerpc v6 1/3] Setup AMOR in HV mode

[Balbir Singh]

On 16/11/16 18:58, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
>> AMOR should be setup in HV mode, we set it up once
>> and let the generic kernel handle IAMR. This patch is
>> used to enable storage keys in a following patch as
>> defined in ISA 3. We don't setup AMOR in DD1, since we
>> can't setup IAMR in DD1 (bits have to be 0). If we setup
>> AMOR some other code could potentially try to set IAMR
>> (guest kernel for example).
>>
>> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
>> ---
>>  arch/powerpc/mm/pgtable-radix.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
>> index ed7bddc..7aa104d 100644
>> --- a/arch/powerpc/mm/pgtable-radix.c
>> +++ b/arch/powerpc/mm/pgtable-radix.c
>> @@ -320,6 +320,27 @@ static void update_hid_for_radix(void)
>>  		cpu_relax();
>>  }
>>  
>> +/*
>> + * In HV mode, we init AMOR so that the hypervisor
>> + * and guest can setup IMAR, enable key 0 and set
>> + * it to 1
>> + * AMOR = 1100....00 (Mask for key 0 is 11)
>> + */
>> +static void radix_init_amor(void)
>> +{
>> +	unsigned long amor_mask = 0xc000000000000000;
>> +	unsigned long amor;
> 
> I guess michael mentioned this in another email, why two variables ?
> 
Left overs from when we did the OR'ing of the mask. But luckily
the compiler does the right thing when generating code

Balbir

Re: [powerpc,v6,1/3] Setup AMOR in HV mode

[Michael Ellerman]

On Tue, 2016-11-15 at 06:56:14 UTC, Balbir Singh wrote:
> AMOR should be setup in HV mode, we set it up once
> and let the generic kernel handle IAMR. This patch is
> used to enable storage keys in a following patch as
> defined in ISA 3. We don't setup AMOR in DD1, since we
> can't setup IAMR in DD1 (bits have to be 0). If we setup
> AMOR some other code could potentially try to set IAMR
> (guest kernel for example).
> 
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/ee97b6b99f42285d29d439f2e5376e

cheers

[powerpc v6 2/3] 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index d0b137d..d498e40 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -390,6 +390,13 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
 #endif /* CONFIG_8xx */
 
 	if (is_exec) {
+
+		/*
+		 * An execution fault + no execute ?
+		 */
+		if (regs->msr & SRR1_ISI_N_OR_G)
+			goto bad_area;
+
 		/*
 		 * Allow execution from readable areas if the MMU does not
 		 * provide separate controls over reading and executing.
@@ -404,6 +411,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
 		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
 		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
 			goto bad_area;
+
 #ifdef CONFIG_PPC_STD_MMU
 		/*
 		 * protfault should only happen due to us
-- 
2.5.5

Re: [powerpc v6 2/3] 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()
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/fault.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index d0b137d..d498e40 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -390,6 +390,13 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>  #endif /* CONFIG_8xx */
>  
>  	if (is_exec) {
> +
> +		/*
> +		 * An execution fault + no execute ?
> +		 */
> +		if (regs->msr & SRR1_ISI_N_OR_G)
> +			goto bad_area;
> +

Can we get that SRR1 value on cpu with  CPU_FTR_NOEXECUTE cleared ?
The comment below says, we should look at at VM_READ and VM_WRITE.

Also don't we need to look at user_mode(regs) here if we are moving this
above the vma check.

>  		/*
>  		 * Allow execution from readable areas if the MMU does not
>  		 * provide separate controls over reading and executing.
> @@ -404,6 +411,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;
> +
>  #ifdef CONFIG_PPC_STD_MMU
>  		/*
>  		 * protfault should only happen due to us
> -- 
> 2.5.5

[powerpc v6 3/3] 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 7aa104d..1a3ea06 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -341,6 +341,26 @@ static void radix_init_amor(void)
 	mtspr(SPRN_AMOR, amor);
 }
 
+/*
+ * For radix page tables we setup, the IAMR values as follows
+ * IMAR = 0100...00 (key 0 is set to 1)
+ * AMR, UAMR, UAMOR are not affected
+ */
+static void radix_init_iamr(void)
+{
+	unsigned long iamr_mask = 0x4000000000000000;
+	unsigned long iamr;
+
+	/*
+	 * The IAMR should set to 0 in DD1
+	 */
+	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
+		return;
+
+	iamr = iamr_mask;
+	mtspr(SPRN_IAMR, iamr);
+}
+
 void __init radix__early_init_mmu(void)
 {
 	unsigned long lpcr;
@@ -400,6 +420,7 @@ void __init radix__early_init_mmu(void)
 		radix_init_amor();
 	}
 
+	radix_init_iamr();
 	radix_init_pgtable();
 }
 
@@ -417,6 +438,7 @@ void radix__early_init_mmu_secondary(void)
 		      __pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
 		radix_init_amor();
 	}
+	radix_init_iamr();
 }
 
 void radix__mmu_cleanup_all(void)
-- 
2.5.5