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

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

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