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

[Ram Pai <linuxram@us.ibm.com>]

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