[09/25] powerpc: ability to create execute-disabled pkeys

Message ID 1504910713-7094-18-git-send-email-linuxram@us.ibm.com
State Changes Requested
Headers show
Series
  • powerpc: Free up RPAGE_RSV bits
Related show

Commit Message

Ram Pai Sept. 8, 2017, 10:44 p.m.
powerpc has hardware support to disable execute on a pkey.
This patch enables the ability to create execute-disabled
keys.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/include/uapi/asm/mman.h |    6 ++++++
 arch/powerpc/mm/pkeys.c              |   16 ++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

Comments

Balbir Singh Oct. 18, 2017, 3:42 a.m. | #1
On Fri,  8 Sep 2017 15:44:57 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> powerpc has hardware support to disable execute on a pkey.
> This patch enables the ability to create execute-disabled
> keys.
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/uapi/asm/mman.h |    6 ++++++
>  arch/powerpc/mm/pkeys.c              |   16 ++++++++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
> index ab45cc2..f272b09 100644
> --- a/arch/powerpc/include/uapi/asm/mman.h
> +++ b/arch/powerpc/include/uapi/asm/mman.h
> @@ -45,4 +45,10 @@
>  #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
>  #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
>  
> +/* override any generic PKEY Permission defines */
> +#define PKEY_DISABLE_EXECUTE   0x4
> +#undef PKEY_ACCESS_MASK
> +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> +				PKEY_DISABLE_WRITE  |\
> +				PKEY_DISABLE_EXECUTE)
>  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index cc5be6a..2282864 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -24,6 +24,14 @@ void __init pkey_initialize(void)
>  {
>  	int os_reserved, i;
>  
> +	/*
> +	 * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
> +	 * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
> +	 * Ensure that the bits a distinct.
> +	 */
> +	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
> +		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));

Will these values every change? It's good to have I guess.

> +
>  	/* disable the pkey system till everything
>  	 * is in place. A patch further down the
>  	 * line will enable it.
> @@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>  		unsigned long init_val)
>  {
>  	u64 new_amr_bits = 0x0ul;
> +	u64 new_iamr_bits = 0x0ul;
>  
>  	if (!is_pkey_enabled(pkey))
>  		return -EINVAL;
>  
> +	if ((init_val & PKEY_DISABLE_EXECUTE)) {
> +		if (!pkey_execute_disable_support)
> +			return -EINVAL;
> +		new_iamr_bits |= IAMR_EX_BIT;
> +	}
> +	init_iamr(pkey, new_iamr_bits);
> +

Where do we check the reserved keys?

Balbir Singh.
Ram Pai Oct. 18, 2017, 5:15 a.m. | #2
On Wed, Oct 18, 2017 at 02:42:56PM +1100, Balbir Singh wrote:
> On Fri,  8 Sep 2017 15:44:57 -0700
> Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > powerpc has hardware support to disable execute on a pkey.
> > This patch enables the ability to create execute-disabled
> > keys.
> > 
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/include/uapi/asm/mman.h |    6 ++++++
> >  arch/powerpc/mm/pkeys.c              |   16 ++++++++++++++++
> >  2 files changed, 22 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
> > index ab45cc2..f272b09 100644
> > --- a/arch/powerpc/include/uapi/asm/mman.h
> > +++ b/arch/powerpc/include/uapi/asm/mman.h
> > @@ -45,4 +45,10 @@
> >  #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
> >  #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
> >  
> > +/* override any generic PKEY Permission defines */
> > +#define PKEY_DISABLE_EXECUTE   0x4
> > +#undef PKEY_ACCESS_MASK
> > +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> > +				PKEY_DISABLE_WRITE  |\
> > +				PKEY_DISABLE_EXECUTE)
> >  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> > index cc5be6a..2282864 100644
> > --- a/arch/powerpc/mm/pkeys.c
> > +++ b/arch/powerpc/mm/pkeys.c
> > @@ -24,6 +24,14 @@ void __init pkey_initialize(void)
> >  {
> >  	int os_reserved, i;
> >  
> > +	/*
> > +	 * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
> > +	 * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
> > +	 * Ensure that the bits a distinct.
> > +	 */
> > +	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
> > +		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
> 
> Will these values every change? It's good to have I guess.
> 
> > +
> >  	/* disable the pkey system till everything
> >  	 * is in place. A patch further down the
> >  	 * line will enable it.
> > @@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> >  		unsigned long init_val)
> >  {
> >  	u64 new_amr_bits = 0x0ul;
> > +	u64 new_iamr_bits = 0x0ul;
> >  
> >  	if (!is_pkey_enabled(pkey))
> >  		return -EINVAL;
> >  
> > +	if ((init_val & PKEY_DISABLE_EXECUTE)) {
> > +		if (!pkey_execute_disable_support)
> > +			return -EINVAL;
> > +		new_iamr_bits |= IAMR_EX_BIT;
> > +	}
> > +	init_iamr(pkey, new_iamr_bits);
> > +
> 
> Where do we check the reserved keys?

The main gate keeper against spurious keys are the system calls.
sys_pkey_mprotect(), sys_pkey_free() and sys_pkey_modify() are the one
that will check against reserved and unallocated keys.  Once it has
passed the check, all other internal functions trust the key values
provided to them. I can put in additional checks but that will
unnecessarily chew a few cpu cycles.

Agree?

BTW: you raise a good point though, I may have missed guarding against
unallocated or reserved keys in sys_pkey_modify(). That was a power
specific system call that I have introduced to change the permissions on
a key.

RP
Aneesh Kumar K.V Oct. 24, 2017, 4:36 a.m. | #3
Ram Pai <linuxram@us.ibm.com> writes:

> powerpc has hardware support to disable execute on a pkey.
> This patch enables the ability to create execute-disabled
> keys.

Can you summarize here how this works?  Access to IAMR is
privileged so how will keys framework work with IAMR? 

-aneesh
Aneesh Kumar K.V Oct. 24, 2017, 6:58 a.m. | #4
Ram Pai <linuxram@us.ibm.com> writes:

> On Wed, Oct 18, 2017 at 02:42:56PM +1100, Balbir Singh wrote:
>> On Fri,  8 Sep 2017 15:44:57 -0700
>> Ram Pai <linuxram@us.ibm.com> wrote:
>> 
>> > powerpc has hardware support to disable execute on a pkey.
>> > This patch enables the ability to create execute-disabled
>> > keys.
>> > 
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> > ---
>> >  arch/powerpc/include/uapi/asm/mman.h |    6 ++++++
>> >  arch/powerpc/mm/pkeys.c              |   16 ++++++++++++++++
>> >  2 files changed, 22 insertions(+), 0 deletions(-)
>> > 
>> > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
>> > index ab45cc2..f272b09 100644
>> > --- a/arch/powerpc/include/uapi/asm/mman.h
>> > +++ b/arch/powerpc/include/uapi/asm/mman.h
>> > @@ -45,4 +45,10 @@
>> >  #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
>> >  #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
>> >  
>> > +/* override any generic PKEY Permission defines */
>> > +#define PKEY_DISABLE_EXECUTE   0x4
>> > +#undef PKEY_ACCESS_MASK
>> > +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
>> > +				PKEY_DISABLE_WRITE  |\
>> > +				PKEY_DISABLE_EXECUTE)
>> >  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
>> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
>> > index cc5be6a..2282864 100644
>> > --- a/arch/powerpc/mm/pkeys.c
>> > +++ b/arch/powerpc/mm/pkeys.c
>> > @@ -24,6 +24,14 @@ void __init pkey_initialize(void)
>> >  {
>> >  	int os_reserved, i;
>> >  
>> > +	/*
>> > +	 * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
>> > +	 * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
>> > +	 * Ensure that the bits a distinct.
>> > +	 */
>> > +	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
>> > +		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
>> 
>> Will these values every change? It's good to have I guess.
>> 
>> > +
>> >  	/* disable the pkey system till everything
>> >  	 * is in place. A patch further down the
>> >  	 * line will enable it.
>> > @@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>> >  		unsigned long init_val)
>> >  {
>> >  	u64 new_amr_bits = 0x0ul;
>> > +	u64 new_iamr_bits = 0x0ul;
>> >  
>> >  	if (!is_pkey_enabled(pkey))
>> >  		return -EINVAL;
>> >  
>> > +	if ((init_val & PKEY_DISABLE_EXECUTE)) {
>> > +		if (!pkey_execute_disable_support)
>> > +			return -EINVAL;
>> > +		new_iamr_bits |= IAMR_EX_BIT;
>> > +	}
>> > +	init_iamr(pkey, new_iamr_bits);
>> > +
>> 
>> Where do we check the reserved keys?
>
> The main gate keeper against spurious keys are the system calls.
> sys_pkey_mprotect(), sys_pkey_free() and sys_pkey_modify() are the one
> that will check against reserved and unallocated keys.  Once it has
> passed the check, all other internal functions trust the key values
> provided to them. I can put in additional checks but that will
> unnecessarily chew a few cpu cycles.
>
> Agree?
>
> BTW: you raise a good point though, I may have missed guarding against
> unallocated or reserved keys in sys_pkey_modify(). That was a power
> specific system call that I have introduced to change the permissions on
> a key.

Why do you need a power specific syscall? We should ideally not require
anything powerpc specific in the application to use memory keys. If it
is for exectue only key, the programming model should remain same as the
other keys.

NOTE: I am not able to find patch that add sys_pkey_modify()

-aneesh
Ram Pai Oct. 24, 2017, 7:20 a.m. | #5
On Tue, Oct 24, 2017 at 12:28:35PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > On Wed, Oct 18, 2017 at 02:42:56PM +1100, Balbir Singh wrote:
> >> On Fri,  8 Sep 2017 15:44:57 -0700
> >> Ram Pai <linuxram@us.ibm.com> wrote:
> >> 
> >> > powerpc has hardware support to disable execute on a pkey.
> >> > This patch enables the ability to create execute-disabled
> >> > keys.
> >> > 
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> > ---
> >> >  arch/powerpc/include/uapi/asm/mman.h |    6 ++++++
> >> >  arch/powerpc/mm/pkeys.c              |   16 ++++++++++++++++
> >> >  2 files changed, 22 insertions(+), 0 deletions(-)
> >> > 
> >> > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
> >> > index ab45cc2..f272b09 100644
> >> > --- a/arch/powerpc/include/uapi/asm/mman.h
> >> > +++ b/arch/powerpc/include/uapi/asm/mman.h
> >> > @@ -45,4 +45,10 @@
> >> >  #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
> >> >  #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
> >> >  
> >> > +/* override any generic PKEY Permission defines */
> >> > +#define PKEY_DISABLE_EXECUTE   0x4
> >> > +#undef PKEY_ACCESS_MASK
> >> > +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> >> > +				PKEY_DISABLE_WRITE  |\
> >> > +				PKEY_DISABLE_EXECUTE)
> >> >  #endif /* _UAPI_ASM_POWERPC_MMAN_H */
> >> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> >> > index cc5be6a..2282864 100644
> >> > --- a/arch/powerpc/mm/pkeys.c
> >> > +++ b/arch/powerpc/mm/pkeys.c
> >> > @@ -24,6 +24,14 @@ void __init pkey_initialize(void)
> >> >  {
> >> >  	int os_reserved, i;
> >> >  
> >> > +	/*
> >> > +	 * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
> >> > +	 * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
> >> > +	 * Ensure that the bits a distinct.
> >> > +	 */
> >> > +	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
> >> > +		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
> >> 
> >> Will these values every change? It's good to have I guess.
> >> 
> >> > +
> >> >  	/* disable the pkey system till everything
> >> >  	 * is in place. A patch further down the
> >> >  	 * line will enable it.
> >> > @@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> >> >  		unsigned long init_val)
> >> >  {
> >> >  	u64 new_amr_bits = 0x0ul;
> >> > +	u64 new_iamr_bits = 0x0ul;
> >> >  
> >> >  	if (!is_pkey_enabled(pkey))
> >> >  		return -EINVAL;
> >> >  
> >> > +	if ((init_val & PKEY_DISABLE_EXECUTE)) {
> >> > +		if (!pkey_execute_disable_support)
> >> > +			return -EINVAL;
> >> > +		new_iamr_bits |= IAMR_EX_BIT;
> >> > +	}
> >> > +	init_iamr(pkey, new_iamr_bits);
> >> > +
> >> 
> >> Where do we check the reserved keys?
> >
> > The main gate keeper against spurious keys are the system calls.
> > sys_pkey_mprotect(), sys_pkey_free() and sys_pkey_modify() are the one
> > that will check against reserved and unallocated keys.  Once it has
> > passed the check, all other internal functions trust the key values
> > provided to them. I can put in additional checks but that will
> > unnecessarily chew a few cpu cycles.
> >
> > Agree?
> >
> > BTW: you raise a good point though, I may have missed guarding against
> > unallocated or reserved keys in sys_pkey_modify(). That was a power
> > specific system call that I have introduced to change the permissions on
> > a key.
> 
> Why do you need a power specific syscall? We should ideally not require
> anything powerpc specific in the application to use memory keys. If it
> is for exectue only key, the programming model should remain same as the
> other keys.

The programming model has not changed. It continues to be the
same. i.e 

a) allocate a key  through sys_pkey_alloc()
b) associate the key to a addressspace through sys_pkey_mprotect()
c) change the permissions on the key by programming the AMR register as
	and when needed.
d) free the key through sys_pkey_free() when done.


the problem is with the programming of execute-permission on the key. x86
does not support the execute-permission and does not have the issue.

powerpc supports execute-permission but unfortunately has not exposed
that capability to userspace, because IAMR cannot be programmed from
userspace. I have filled in that gap, by providing a power-specific
system call called sys_pkey_modify().  It is a way to enable the exact 
same programming model on keys for execute-permissions.


> 
> NOTE: I am not able to find patch that add sys_pkey_modify()

Yes that patch was added only recently to my tree after consulting
Michael Ellermen. I am yet to send out that patch. Will be doing so
in my next version.

RP

> 
> -aneesh
Ram Pai Oct. 28, 2017, 11:18 p.m. | #6
On Tue, Oct 24, 2017 at 10:06:18AM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > powerpc has hardware support to disable execute on a pkey.
> > This patch enables the ability to create execute-disabled
> > keys.
> 
> Can you summarize here how this works?  Access to IAMR is
> privileged so how will keys framework work with IAMR? 
> 
> -aneesh

right. IAMR will have to programmed through a system call.
I have introduced a sys_pkey_modify()  which takes a key value
and the permission that it wants to enable/disable on that key.
This syscall is powerpc specific only for now, since no other
arch's need it.

The patch is at http://patchwork.ozlabs.org/patch/817961/

Patch

diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index ab45cc2..f272b09 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -45,4 +45,10 @@ 
 #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
 #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
 
+/* override any generic PKEY Permission defines */
+#define PKEY_DISABLE_EXECUTE   0x4
+#undef PKEY_ACCESS_MASK
+#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
+				PKEY_DISABLE_WRITE  |\
+				PKEY_DISABLE_EXECUTE)
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index cc5be6a..2282864 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -24,6 +24,14 @@  void __init pkey_initialize(void)
 {
 	int os_reserved, i;
 
+	/*
+	 * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
+	 * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
+	 * Ensure that the bits a distinct.
+	 */
+	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
+		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
+
 	/* disable the pkey system till everything
 	 * is in place. A patch further down the
 	 * line will enable it.
@@ -120,10 +128,18 @@  int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val)
 {
 	u64 new_amr_bits = 0x0ul;
+	u64 new_iamr_bits = 0x0ul;
 
 	if (!is_pkey_enabled(pkey))
 		return -EINVAL;
 
+	if ((init_val & PKEY_DISABLE_EXECUTE)) {
+		if (!pkey_execute_disable_support)
+			return -EINVAL;
+		new_iamr_bits |= IAMR_EX_BIT;
+	}
+	init_iamr(pkey, new_iamr_bits);
+
 	/* Set the bits we need in AMR:  */
 	if (init_val & PKEY_DISABLE_ACCESS)
 		new_amr_bits |= AMR_RD_BIT | AMR_WR_BIT;