On Tue, Oct 24, 2017 at 12:28:35PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linux...@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 <linux...@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 <linux...@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
