| Message ID | 1504910713-7094-18-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc: Free up RPAGE_RSV bits | expand |
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.
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
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
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
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
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/
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;
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(-)