| Message ID | 1504910713-7094-14-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
On Fri, 8 Sep 2017 15:44:53 -0700 Ram Pai <linuxram@us.ibm.com> wrote: > Introduce helper functions that can initialize the bits in the AMR, > IAMR and UAMOR register; the bits that correspond to the given pkey. > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > --- > arch/powerpc/include/asm/pkeys.h | 1 + > arch/powerpc/mm/pkeys.c | 46 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 47 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h > index 133f8c4..5a83ed7 100644 > --- a/arch/powerpc/include/asm/pkeys.h > +++ b/arch/powerpc/include/asm/pkeys.h > @@ -26,6 +26,7 @@ > #define arch_max_pkey() pkeys_total > #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \ > VM_PKEY_BIT3 | VM_PKEY_BIT4) > +#define AMR_BITS_PER_PKEY 2 > > #define pkey_alloc_mask(pkey) (0x1 << pkey) > > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > index ebc9e84..178aa33 100644 > --- a/arch/powerpc/mm/pkeys.c > +++ b/arch/powerpc/mm/pkeys.c > @@ -59,3 +59,49 @@ void __init pkey_initialize(void) > for (i = 2; i < (pkeys_total - os_reserved); i++) > initial_allocation_mask &= ~(0x1<<i); > } > + > +#define PKEY_REG_BITS (sizeof(u64)*8) > +#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > + > +static inline void init_amr(int pkey, u8 init_bits) > +{ > + u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > + u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > + Do we need to check for reserved keys or that is at a layer above? > + write_amr(old_amr | new_amr_bits); > +} > + > +static inline void init_iamr(int pkey, u8 init_bits) > +{ > + u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > + u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > + > + write_iamr(old_iamr | new_iamr_bits); Do we need to check for reserved keys here? > +} > + > +static void pkey_status_change(int pkey, bool enable) > +{ > + u64 old_uamor; > + > + /* reset the AMR and IAMR bits for this key */ > + init_amr(pkey, 0x0); > + init_iamr(pkey, 0x0); > + > + /* enable/disable key */ > + old_uamor = read_uamor(); > + if (enable) > + old_uamor |= (0x3ul << pkeyshift(pkey)); > + else > + old_uamor &= ~(0x3ul << pkeyshift(pkey)); > + write_uamor(old_uamor); > +} > + > +void __arch_activate_pkey(int pkey) > +{ > + pkey_status_change(pkey, true); > +} > + > +void __arch_deactivate_pkey(int pkey) > +{ > + pkey_status_change(pkey, false); > +} Balbir Singh.
On Wed, Oct 18, 2017 at 02:24:03PM +1100, Balbir Singh wrote: > On Fri, 8 Sep 2017 15:44:53 -0700 > Ram Pai <linuxram@us.ibm.com> wrote: > > > Introduce helper functions that can initialize the bits in the AMR, > > IAMR and UAMOR register; the bits that correspond to the given pkey. > > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > --- > > arch/powerpc/include/asm/pkeys.h | 1 + > > arch/powerpc/mm/pkeys.c | 46 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 47 insertions(+), 0 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h > > index 133f8c4..5a83ed7 100644 > > --- a/arch/powerpc/include/asm/pkeys.h > > +++ b/arch/powerpc/include/asm/pkeys.h > > @@ -26,6 +26,7 @@ > > #define arch_max_pkey() pkeys_total > > #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \ > > VM_PKEY_BIT3 | VM_PKEY_BIT4) > > +#define AMR_BITS_PER_PKEY 2 > > > > #define pkey_alloc_mask(pkey) (0x1 << pkey) > > > > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > > index ebc9e84..178aa33 100644 > > --- a/arch/powerpc/mm/pkeys.c > > +++ b/arch/powerpc/mm/pkeys.c > > @@ -59,3 +59,49 @@ void __init pkey_initialize(void) > > for (i = 2; i < (pkeys_total - os_reserved); i++) > > initial_allocation_mask &= ~(0x1<<i); > > } > > + > > +#define PKEY_REG_BITS (sizeof(u64)*8) > > +#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > > + > > +static inline void init_amr(int pkey, u8 init_bits) > > +{ > > + u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > > + u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > > + > > Do we need to check for reserved keys or that is at a layer above? These routines blindly trust the caller. The assumption is the system calls which are the gate-keepers for the keys validate the keys before calling any lower level functions. > > > + write_amr(old_amr | new_amr_bits); > > +} > > + > > +static inline void init_iamr(int pkey, u8 init_bits) > > +{ > > + u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > > + u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > > + > > + write_iamr(old_iamr | new_iamr_bits); > > Do we need to check for reserved keys here? > ditto.. > > +} > > + > > +static void pkey_status_change(int pkey, bool enable) > > +{ > > + u64 old_uamor; > > + > > + /* reset the AMR and IAMR bits for this key */ > > + init_amr(pkey, 0x0); > > + init_iamr(pkey, 0x0); > > + > > + /* enable/disable key */ > > + old_uamor = read_uamor(); > > + if (enable) > > + old_uamor |= (0x3ul << pkeyshift(pkey)); > > + else > > + old_uamor &= ~(0x3ul << pkeyshift(pkey)); > > + write_uamor(old_uamor); > > +} > > + > > +void __arch_activate_pkey(int pkey) > > +{ > > + pkey_status_change(pkey, true); > > +} > > + > > +void __arch_deactivate_pkey(int pkey) > > +{ > > + pkey_status_change(pkey, false); > > +} > > > Balbir Singh.
Ram Pai <linuxram@us.ibm.com> writes: > Introduce helper functions that can initialize the bits in the AMR, > IAMR and UAMOR register; the bits that correspond to the given pkey. > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > --- > arch/powerpc/include/asm/pkeys.h | 1 + > arch/powerpc/mm/pkeys.c | 46 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 47 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h > index 133f8c4..5a83ed7 100644 > --- a/arch/powerpc/include/asm/pkeys.h > +++ b/arch/powerpc/include/asm/pkeys.h > @@ -26,6 +26,7 @@ > #define arch_max_pkey() pkeys_total > #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \ > VM_PKEY_BIT3 | VM_PKEY_BIT4) > +#define AMR_BITS_PER_PKEY 2 > > #define pkey_alloc_mask(pkey) (0x1 << pkey) > > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > index ebc9e84..178aa33 100644 > --- a/arch/powerpc/mm/pkeys.c > +++ b/arch/powerpc/mm/pkeys.c > @@ -59,3 +59,49 @@ void __init pkey_initialize(void) > for (i = 2; i < (pkeys_total - os_reserved); i++) > initial_allocation_mask &= ~(0x1<<i); > } > + > +#define PKEY_REG_BITS (sizeof(u64)*8) > +#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > + > +static inline void init_amr(int pkey, u8 init_bits) > +{ > + u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > + u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > + > + write_amr(old_amr | new_amr_bits); > +} > + > +static inline void init_iamr(int pkey, u8 init_bits) > +{ > + u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > + u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > + > + write_iamr(old_iamr | new_iamr_bits); > +} > + > +static void pkey_status_change(int pkey, bool enable) > +{ > + u64 old_uamor; > + > + /* reset the AMR and IAMR bits for this key */ > + init_amr(pkey, 0x0); > + init_iamr(pkey, 0x0); > + > + /* enable/disable key */ > + old_uamor = read_uamor(); > + if (enable) > + old_uamor |= (0x3ul << pkeyshift(pkey)); > + else > + old_uamor &= ~(0x3ul << pkeyshift(pkey)); > + write_uamor(old_uamor); > +} That one is confusing, we discussed this outside the list, but want to bring the list to further discussion. So now the core kernel request for a key via mm_pkey_alloc(). Why not do the below there static inline int mm_pkey_alloc(struct mm_struct *mm) { /* * Note: this is the one and only place we make sure * that the pkey is valid as far as the hardware is * concerned. The rest of the kernel trusts that * only good, valid pkeys come out of here. */ u32 all_pkeys_mask = (u32)(~(0x0)); int ret; if (!pkey_inited) return -1; /* * Are we out of pkeys? We must handle this specially * because ffz() behavior is undefined if there are no * zeros. */ if (mm_pkey_allocation_map(mm) == all_pkeys_mask) return -1; ret = ffz((u32)mm_pkey_allocation_map(mm)); mm_set_pkey_allocated(mm, ret); return ret; } your mm_pkey_allocation_map() should have the keys specified in AMOR and UAMOR marked as allocatied. It is in use by hypervisor and OS respectively. There is no need of __arch_activate_key() etc. and by default if the OS has not requested for a key for its internal use UAMOR should be 0xFFFFFFFF and that AMOR value you derive from the device tree based of what keys hypervisor has reserved. -aneesh
On Tue, Oct 24, 2017 at 11:55:41AM +0530, Aneesh Kumar K.V wrote: > Ram Pai <linuxram@us.ibm.com> writes: > > > Introduce helper functions that can initialize the bits in the AMR, > > IAMR and UAMOR register; the bits that correspond to the given pkey. > > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > --- > > arch/powerpc/include/asm/pkeys.h | 1 + > > arch/powerpc/mm/pkeys.c | 46 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 47 insertions(+), 0 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h > > index 133f8c4..5a83ed7 100644 > > --- a/arch/powerpc/include/asm/pkeys.h > > +++ b/arch/powerpc/include/asm/pkeys.h > > @@ -26,6 +26,7 @@ > > #define arch_max_pkey() pkeys_total > > #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \ > > VM_PKEY_BIT3 | VM_PKEY_BIT4) > > +#define AMR_BITS_PER_PKEY 2 > > > > #define pkey_alloc_mask(pkey) (0x1 << pkey) > > > > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > > index ebc9e84..178aa33 100644 > > --- a/arch/powerpc/mm/pkeys.c > > +++ b/arch/powerpc/mm/pkeys.c > > @@ -59,3 +59,49 @@ void __init pkey_initialize(void) > > for (i = 2; i < (pkeys_total - os_reserved); i++) > > initial_allocation_mask &= ~(0x1<<i); > > } > > + > > +#define PKEY_REG_BITS (sizeof(u64)*8) > > +#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) > > + > > +static inline void init_amr(int pkey, u8 init_bits) > > +{ > > + u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > > + u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > > + > > + write_amr(old_amr | new_amr_bits); > > +} > > + > > +static inline void init_iamr(int pkey, u8 init_bits) > > +{ > > + u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); > > + u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey)); > > + > > + write_iamr(old_iamr | new_iamr_bits); > > +} > > + > > +static void pkey_status_change(int pkey, bool enable) > > +{ > > + u64 old_uamor; > > + > > + /* reset the AMR and IAMR bits for this key */ > > + init_amr(pkey, 0x0); > > + init_iamr(pkey, 0x0); > > + > > + /* enable/disable key */ > > + old_uamor = read_uamor(); > > + if (enable) > > + old_uamor |= (0x3ul << pkeyshift(pkey)); > > + else > > + old_uamor &= ~(0x3ul << pkeyshift(pkey)); > > + write_uamor(old_uamor); > > +} > > That one is confusing, we discussed this outside the list, but want to > bring the list to further discussion. So now the core kernel request > for a key via mm_pkey_alloc(). Why not do the below there > > static inline int mm_pkey_alloc(struct mm_struct *mm) > { > /* > * Note: this is the one and only place we make sure > * that the pkey is valid as far as the hardware is > * concerned. The rest of the kernel trusts that > * only good, valid pkeys come out of here. > */ > u32 all_pkeys_mask = (u32)(~(0x0)); > int ret; > > if (!pkey_inited) > return -1; > /* > * Are we out of pkeys? We must handle this specially > * because ffz() behavior is undefined if there are no > * zeros. > */ > if (mm_pkey_allocation_map(mm) == all_pkeys_mask) > return -1; > > ret = ffz((u32)mm_pkey_allocation_map(mm)); > mm_set_pkey_allocated(mm, ret); > > return ret; > } > > your mm_pkey_allocation_map() should have the keys specified in AMOR and > UAMOR marked as allocatied. It is in use by hypervisor and OS respectively. > > > There is no need of __arch_activate_key() etc. and by default if the OS > has not requested for a key for its internal use UAMOR should be > 0xFFFFFFFF and that AMOR value you derive from the device tree based of > what keys hypervisor has reserved. Ok. You are suggesting a programming model where a) keys that are reserved by hypervisor are enabled by default. b) keys that are reserved by the OS are enabled by default. c) keys that are not yet allocated to userspace is enabled by default. The problem with this model is that, the userspace can change the permissions of an unallocated key, and those permissions will go into effect immediately, because every key is enabled by default. If it happens to be a key that is reserved by the OS or the hypervisor, bad things can happen. the programming model that I have implemented; **which is the programming model expected by linux**, is a) keys that are reserved by hypervisor are left to the hypervisor to enable/disable whenever it needs to. b) keys that are reserved by the OS are left to the OS to enable/disable whenever it needs to. c) keys that are not yet allocated to userspace are not yet enabled. Will be enabled when the key is allocated to userspace through sys_pkey_alloc() syscall. In this programming model, userspace is expected to use only keys that are allocated. And in case it inadvertetly uses a key that is not allocated, nothing bad happens because that key is not activated unless it is allocated. And in case it uses a key that is reserved by the hypervisor or OS, bad things will still not happen because those keys should not be kept enabled when the process is in userspace. These are my thoughts, I will let the jury decide. :) RP > > -aneesh
Ram Pai <linuxram@us.ibm.com> writes: > On Tue, Oct 24, 2017 at 11:55:41AM +0530, Aneesh Kumar K.V wrote: >> Ram Pai <linuxram@us.ibm.com> writes: >> > + >> > +static void pkey_status_change(int pkey, bool enable) >> > +{ >> > + u64 old_uamor; >> > + >> > + /* reset the AMR and IAMR bits for this key */ >> > + init_amr(pkey, 0x0); >> > + init_iamr(pkey, 0x0); >> > + >> > + /* enable/disable key */ >> > + old_uamor = read_uamor(); >> > + if (enable) >> > + old_uamor |= (0x3ul << pkeyshift(pkey)); >> > + else >> > + old_uamor &= ~(0x3ul << pkeyshift(pkey)); >> > + write_uamor(old_uamor); >> > +} >> >> That one is confusing, we discussed this outside the list, but want to >> bring the list to further discussion. So now the core kernel request >> for a key via mm_pkey_alloc(). Why not do the below there >> >> static inline int mm_pkey_alloc(struct mm_struct *mm) >> { >> /* >> * Note: this is the one and only place we make sure >> * that the pkey is valid as far as the hardware is >> * concerned. The rest of the kernel trusts that >> * only good, valid pkeys come out of here. >> */ >> u32 all_pkeys_mask = (u32)(~(0x0)); >> int ret; >> >> if (!pkey_inited) >> return -1; >> /* >> * Are we out of pkeys? We must handle this specially >> * because ffz() behavior is undefined if there are no >> * zeros. >> */ >> if (mm_pkey_allocation_map(mm) == all_pkeys_mask) >> return -1; >> >> ret = ffz((u32)mm_pkey_allocation_map(mm)); >> mm_set_pkey_allocated(mm, ret); >> >> return ret; >> } >> >> your mm_pkey_allocation_map() should have the keys specified in AMOR and >> UAMOR marked as allocatied. It is in use by hypervisor and OS respectively. >> >> >> There is no need of __arch_activate_key() etc. and by default if the OS >> has not requested for a key for its internal use UAMOR should be >> 0xFFFFFFFF and that AMOR value you derive from the device tree based of >> what keys hypervisor has reserved. > > > Ok. You are suggesting a programming model where > a) keys that are reserved by hypervisor are enabled by default. No he's not saying that. > b) keys that are reserved by the OS are enabled by default. Or that. > c) keys that are not yet allocated to userspace is enabled by default. But he is saying this. > The problem with this model is that, the userspace can change the > permissions of an unallocated key, and those permissions will go into > effect immediately, Correct, but an unallocated key should not be used anywhere, so it should have no effect, unless there's a bug. > because every key is enabled by default. If it Not every key, every key that is not reserved by the hypervisor or OS. > happens to be a key that is reserved by the OS or the hypervisor, bad > things can happen. So no nothing bad should be able to happen. > the programming model that I have implemented; **which is the programming > model expected by linux**, is > > a) keys that are reserved by hypervisor are left to the hypervisor to > enable/disable whenever it needs to. We agree on that. > b) keys that are reserved by the OS are left to the OS to > enable/disable whenever it needs to. And that. > c) keys that are not yet allocated to userspace are not yet enabled. > Will be enabled when the key is allocated to userspace > through sys_pkey_alloc() syscall. This is the only part that is under discussion. > In this programming model, userspace is expected to use only keys that > are allocated. And in case it inadvertetly uses a key that is not > allocated, nothing bad happens because that key is not activated unless > it is allocated. This sames like a good design to me. The only downside I see is we have to switch an extra SPR, but that's not much in the scheme of things. cheers
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h index 133f8c4..5a83ed7 100644 --- a/arch/powerpc/include/asm/pkeys.h +++ b/arch/powerpc/include/asm/pkeys.h @@ -26,6 +26,7 @@ #define arch_max_pkey() pkeys_total #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \ VM_PKEY_BIT3 | VM_PKEY_BIT4) +#define AMR_BITS_PER_PKEY 2 #define pkey_alloc_mask(pkey) (0x1 << pkey) diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c index ebc9e84..178aa33 100644 --- a/arch/powerpc/mm/pkeys.c +++ b/arch/powerpc/mm/pkeys.c @@ -59,3 +59,49 @@ void __init pkey_initialize(void) for (i = 2; i < (pkeys_total - os_reserved); i++) initial_allocation_mask &= ~(0x1<<i); } + +#define PKEY_REG_BITS (sizeof(u64)*8) +#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY)) + +static inline void init_amr(int pkey, u8 init_bits) +{ + u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); + u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey)); + + write_amr(old_amr | new_amr_bits); +} + +static inline void init_iamr(int pkey, u8 init_bits) +{ + u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey)); + u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey)); + + write_iamr(old_iamr | new_iamr_bits); +} + +static void pkey_status_change(int pkey, bool enable) +{ + u64 old_uamor; + + /* reset the AMR and IAMR bits for this key */ + init_amr(pkey, 0x0); + init_iamr(pkey, 0x0); + + /* enable/disable key */ + old_uamor = read_uamor(); + if (enable) + old_uamor |= (0x3ul << pkeyshift(pkey)); + else + old_uamor &= ~(0x3ul << pkeyshift(pkey)); + write_uamor(old_uamor); +} + +void __arch_activate_pkey(int pkey) +{ + pkey_status_change(pkey, true); +} + +void __arch_deactivate_pkey(int pkey) +{ + pkey_status_change(pkey, false); +}
Introduce helper functions that can initialize the bits in the AMR, IAMR and UAMOR register; the bits that correspond to the given pkey. Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- arch/powerpc/include/asm/pkeys.h | 1 + arch/powerpc/mm/pkeys.c | 46 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 0 deletions(-)