[RFC,v6,27/62] powerpc: helper to validate key-access permissions of a pte

Submitted by Ram Pai on July 16, 2017, 3:56 a.m.

Details

Message ID 1500177424-13695-28-git-send-email-linuxram@us.ibm.com
State RFC
Headers show

Commit Message

Ram Pai July 16, 2017, 3:56 a.m.
helper function that checks if the read/write/execute is allowed
on the pte.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
 arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
 arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 0 deletions(-)

Comments

Aneesh Kumar K.V July 20, 2017, 6:42 a.m.
Ram Pai <linuxram@us.ibm.com> writes:

> helper function that checks if the read/write/execute is allowed
> on the pte.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
>  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
>  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 30d7f55..0056e58 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -472,6 +472,10 @@ static inline void write_uamor(u64 value)
>  	mtspr(SPRN_UAMOR, value);
>  }
>
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> +extern bool arch_pte_access_permitted(u64 pte, bool write, bool execute);
> +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> +
>  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
>  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
>  				       unsigned long addr, pte_t *ptep)
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index bbb5d85..7a9aade 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -53,6 +53,18 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>  		((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
>  }
>
> +static inline u16 pte_to_pkey_bits(u64 pteflags)
> +{
> +	if (!pkey_inited)
> +		return 0x0UL;

Do we really need that above check ? We should always find it
peky_inited to be set. 

> +
> +	return (((pteflags & H_PAGE_PKEY_BIT0) ? 0x10 : 0x0UL) |
> +		((pteflags & H_PAGE_PKEY_BIT1) ? 0x8 : 0x0UL) |
> +		((pteflags & H_PAGE_PKEY_BIT2) ? 0x4 : 0x0UL) |
> +		((pteflags & H_PAGE_PKEY_BIT3) ? 0x2 : 0x0UL) |
> +		((pteflags & H_PAGE_PKEY_BIT4) ? 0x1 : 0x0UL));
> +}
> +


-aneesh
Ram Pai July 20, 2017, 10:15 p.m.
On Thu, Jul 20, 2017 at 12:12:47PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > helper function that checks if the read/write/execute is allowed
> > on the pte.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
> >  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
> >  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
> >  3 files changed, 49 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > index 30d7f55..0056e58 100644
> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> > @@ -472,6 +472,10 @@ static inline void write_uamor(u64 value)
> >  	mtspr(SPRN_UAMOR, value);
> >  }
> >
> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > +extern bool arch_pte_access_permitted(u64 pte, bool write, bool execute);
> > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> > +
> >  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
> >  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
> >  				       unsigned long addr, pte_t *ptep)
> > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> > index bbb5d85..7a9aade 100644
> > --- a/arch/powerpc/include/asm/pkeys.h
> > +++ b/arch/powerpc/include/asm/pkeys.h
> > @@ -53,6 +53,18 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
> >  		((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
> >  }
> >
> > +static inline u16 pte_to_pkey_bits(u64 pteflags)
> > +{
> > +	if (!pkey_inited)
> > +		return 0x0UL;
> 
> Do we really need that above check ? We should always find it
> peky_inited to be set. 

Yes. there are cases where pkey_inited is not enabled. 
a) if the MMU is radix.
b) if the PAGE size is 4k.
c) if the device tree says the feature is not available
d) if the CPU is of a older generation. P6 and older.

RP
Aneesh Kumar K.V July 21, 2017, 6:51 a.m.
Ram Pai <linuxram@us.ibm.com> writes:

> On Thu, Jul 20, 2017 at 12:12:47PM +0530, Aneesh Kumar K.V wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>> 
>> > helper function that checks if the read/write/execute is allowed
>> > on the pte.
>> >
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> > ---
>> >  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
>> >  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
>> >  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
>> >  3 files changed, 49 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> > index 30d7f55..0056e58 100644
>> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> > @@ -472,6 +472,10 @@ static inline void write_uamor(u64 value)
>> >  	mtspr(SPRN_UAMOR, value);
>> >  }
>> >
>> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
>> > +extern bool arch_pte_access_permitted(u64 pte, bool write, bool execute);
>> > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
>> > +
>> >  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
>> >  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
>> >  				       unsigned long addr, pte_t *ptep)
>> > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
>> > index bbb5d85..7a9aade 100644
>> > --- a/arch/powerpc/include/asm/pkeys.h
>> > +++ b/arch/powerpc/include/asm/pkeys.h
>> > @@ -53,6 +53,18 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>> >  		((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
>> >  }
>> >
>> > +static inline u16 pte_to_pkey_bits(u64 pteflags)
>> > +{
>> > +	if (!pkey_inited)
>> > +		return 0x0UL;
>> 
>> Do we really need that above check ? We should always find it
>> peky_inited to be set. 
>
> Yes. there are cases where pkey_inited is not enabled. 
> a) if the MMU is radix.
That should be be a feature check

> b) if the PAGE size is 4k.

That is a kernel config change

> c) if the device tree says the feature is not available
> d) if the CPU is of a older generation. P6 and older.

Both feature check.

how about doing something like

static inline u16 pte_to_pkey_bits(u64 pteflags)
{
	if (!(pteflags & H_PAGE_KEY_MASK))
		return 0x0UL;

	return (((pteflags & H_PAGE_PKEY_BIT0) ? 0x10 : 0x0UL) |
		((pteflags & H_PAGE_PKEY_BIT1) ? 0x8 : 0x0UL) |
		((pteflags & H_PAGE_PKEY_BIT2) ? 0x4 : 0x0UL) |
		((pteflags & H_PAGE_PKEY_BIT3) ? 0x2 : 0x0UL) |
		((pteflags & H_PAGE_PKEY_BIT4) ? 0x1 : 0x0UL));
}

We still have to look at the code generated to see it is really saving
something.

-aneesh
Ram Pai July 21, 2017, 4:42 p.m.
On Fri, Jul 21, 2017 at 12:21:50PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > On Thu, Jul 20, 2017 at 12:12:47PM +0530, Aneesh Kumar K.V wrote:
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >> 
> >> > helper function that checks if the read/write/execute is allowed
> >> > on the pte.
> >> >
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> > ---
> >> >  arch/powerpc/include/asm/book3s/64/pgtable.h |    4 +++
> >> >  arch/powerpc/include/asm/pkeys.h             |   12 +++++++++
> >> >  arch/powerpc/mm/pkeys.c                      |   33 ++++++++++++++++++++++++++
> >> >  3 files changed, 49 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> > index 30d7f55..0056e58 100644
> >> > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> >> > @@ -472,6 +472,10 @@ static inline void write_uamor(u64 value)
> >> >  	mtspr(SPRN_UAMOR, value);
> >> >  }
> >> >
> >> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> >> > +extern bool arch_pte_access_permitted(u64 pte, bool write, bool execute);
> >> > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> >> > +
> >> >  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
> >> >  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
> >> >  				       unsigned long addr, pte_t *ptep)
> >> > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> >> > index bbb5d85..7a9aade 100644
> >> > --- a/arch/powerpc/include/asm/pkeys.h
> >> > +++ b/arch/powerpc/include/asm/pkeys.h
> >> > @@ -53,6 +53,18 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
> >> >  		((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
> >> >  }
> >> >
> >> > +static inline u16 pte_to_pkey_bits(u64 pteflags)
> >> > +{
> >> > +	if (!pkey_inited)
> >> > +		return 0x0UL;
> >> 
> >> Do we really need that above check ? We should always find it
> >> peky_inited to be set. 
> >
> > Yes. there are cases where pkey_inited is not enabled. 
> > a) if the MMU is radix.
> That should be be a feature check
> 
> > b) if the PAGE size is 4k.
> 
> That is a kernel config change
> 
> > c) if the device tree says the feature is not available
> > d) if the CPU is of a older generation. P6 and older.
> 
> Both feature check.
> 
> how about doing something like
> 
> static inline u16 pte_to_pkey_bits(u64 pteflags)
> {
> 	if (!(pteflags & H_PAGE_KEY_MASK))
> 		return 0x0UL;

This check accomplishes the same thing as the return below.
When (pteflag & H_PAGE_KEY_MASK) is 0,
the code below returns the same 0x0UL. 



> 
> 	return (((pteflags & H_PAGE_PKEY_BIT0) ? 0x10 : 0x0UL) |
> 		((pteflags & H_PAGE_PKEY_BIT1) ? 0x8 : 0x0UL) |
> 		((pteflags & H_PAGE_PKEY_BIT2) ? 0x4 : 0x0UL) |
> 		((pteflags & H_PAGE_PKEY_BIT3) ? 0x2 : 0x0UL) |
> 		((pteflags & H_PAGE_PKEY_BIT4) ? 0x1 : 0x0UL));
> }

The idea  behind
	       if (!pkey_inited)
	               return 0x0UL;

was to not interpret the ptebits if we knew they were not initialized
to begin with.

Patch hide | download patch | download mbox

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 30d7f55..0056e58 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -472,6 +472,10 @@  static inline void write_uamor(u64 value)
 	mtspr(SPRN_UAMOR, value);
 }
 
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+extern bool arch_pte_access_permitted(u64 pte, bool write, bool execute);
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
+
 #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
 static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
 				       unsigned long addr, pte_t *ptep)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index bbb5d85..7a9aade 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -53,6 +53,18 @@  static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
 		((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
 }
 
+static inline u16 pte_to_pkey_bits(u64 pteflags)
+{
+	if (!pkey_inited)
+		return 0x0UL;
+
+	return (((pteflags & H_PAGE_PKEY_BIT0) ? 0x10 : 0x0UL) |
+		((pteflags & H_PAGE_PKEY_BIT1) ? 0x8 : 0x0UL) |
+		((pteflags & H_PAGE_PKEY_BIT2) ? 0x4 : 0x0UL) |
+		((pteflags & H_PAGE_PKEY_BIT3) ? 0x2 : 0x0UL) |
+		((pteflags & H_PAGE_PKEY_BIT4) ? 0x1 : 0x0UL));
+}
+
 static inline int vma_pkey(struct vm_area_struct *vma)
 {
 	if (!pkey_inited)
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 403f5ae..1794e17 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -201,3 +201,36 @@  int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot,
 	 */
 	return vma_pkey(vma);
 }
+
+static bool pkey_access_permitted(int pkey, bool write, bool execute)
+{
+	int pkey_shift;
+	u64 amr;
+
+	if (!pkey)
+		return true;
+
+	pkey_shift = pkeyshift(pkey);
+	if (!(read_uamor() & (0x3UL << pkey_shift)))
+		return true;
+
+	if (execute && !(read_iamr() & (IAMR_EX_BIT << pkey_shift)))
+		return true;
+
+	if (!write) {
+		amr = read_amr();
+		if (!(amr & (AMR_RD_BIT << pkey_shift)))
+			return true;
+	}
+
+	amr = read_amr(); /* delay reading amr uptil absolutely needed */
+	return (write && !(amr & (AMR_WR_BIT << pkey_shift)));
+}
+
+bool arch_pte_access_permitted(u64 pte, bool write, bool execute)
+{
+	if (!pkey_inited)
+		return true;
+	return pkey_access_permitted(pte_to_pkey_bits(pte),
+			write, execute);
+}