Re: pkeys: Reserve PKEY_DISABLE_READ
On Wed, Dec 05, 2018 at 08:21:02AM -0800, Andy Lutomirski wrote: > > On Dec 2, 2018, at 8:02 PM, Ram Pai wrote: > > > >> On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote: > >> * Dave Hansen: > >> > On 11/27/18 3:57 AM, Florian Weimer wrote: > I would have expected something that translates PKEY_DISABLE_WRITE | > PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts > PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER. > > (My understanding is that PKEY_DISABLE_ACCESS does not disable all > access, but produces execute-only memory.) > >>> > >>> Correct, it disables all data access, but not execution. > >> > >> So I would expect something like this (completely untested, I did not > >> even compile this): > > > > > > Ok. I re-read through the entire email thread to understand the problem and > > the proposed solution. Let me summarize it below. Lets see if we are on the > > same > > plate. > > > > So the problem is as follows: > > > > Currently the kernel supports 'disable-write' and 'disable-access'. > > > > On x86, cpu supports 'disable-write' and 'disable-access'. This > > matches with what the kernel supports. All good. > > > > However on power, cpu supports 'disable-read' too. Since userspace can > > program the cpu directly, userspace has the ability to set > > 'disable-read' too. This can lead to inconsistency between the kernel > > and the userspace. > > > > We want the kernel to match userspace on all architectures. > > > > Proposed Solution: > > > > Enhance the kernel to understand 'disable-read', and facilitate > > architectures > > that understand 'disable-read' to allow it. > > > > Also explicitly define the semantics of disable-access as > > 'disable-read and disable-write' > > > > Did I get this right? Assuming I did, the implementation has to do > > the following -- > > > >On power, sys_pkey_alloc() should succeed if the init_val > >is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS > >or any combination of the three. > > > >On x86, sys_pkey_alloc() should succeed if the init_val is > >PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ > >or any combination of the three, except PKEY_DISABLE_READ > > specified all by itself. > > > >On all other arches, none of the flags are supported. > > I don’t really love having a situation where you can use different > flag combinations to refer to the same mode. true. But it is a side-effect of x86 cpu implicitly defining 'disable-access' as a combination of 'disable-read' and 'disable_write'. In other words, if you disable-access on a pte on x86, you are automatically disabling read and disabling write on that page. The software/kernel just happens to explicitly capture that implicit behavior. > > Also, we should document the effect these flags have on execute permission. Actually none of the above flags, interact with execute permission. They operate independently; both on x86 and on POWER. But yes, this statement needs to be documented somewhere. RP
Re: pkeys: Reserve PKEY_DISABLE_READ
On Wed, Dec 05, 2018 at 02:00:59PM +0100, Florian Weimer wrote: > * Ram Pai: > > > Ok. here is a patch, compiled but not tested. See if this meets the > > specifications. > > > > --- > > > > commit 3dc06e73f3795921265d5d1d935e428deab01616 > > Author: Ram Pai > > Date: Tue Dec 4 00:04:11 2018 -0500 > > > > pkeys: add support of PKEY_DISABLE_READ > > Thanks. In the x86 code, the translation of PKEY_DISABLE_READ | > PKEY_DISABLE_WRITE to PKEY_DISABLE_ACCESS appears to be missing. I > believe the existing code produces PKEY_DISABLE_WRITE, which is wrong. ah. yes. good point. > > Rest looks okay to me (again not tested). thanks, RP
Re: pkeys: Reserve PKEY_DISABLE_READ
> On Dec 2, 2018, at 8:02 PM, Ram Pai wrote: > >> On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote: >> * Dave Hansen: >> On 11/27/18 3:57 AM, Florian Weimer wrote: I would have expected something that translates PKEY_DISABLE_WRITE | PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER. (My understanding is that PKEY_DISABLE_ACCESS does not disable all access, but produces execute-only memory.) >>> >>> Correct, it disables all data access, but not execution. >> >> So I would expect something like this (completely untested, I did not >> even compile this): > > > Ok. I re-read through the entire email thread to understand the problem and > the proposed solution. Let me summarize it below. Lets see if we are on the > same > plate. > > So the problem is as follows: > > Currently the kernel supports 'disable-write' and 'disable-access'. > > On x86, cpu supports 'disable-write' and 'disable-access'. This > matches with what the kernel supports. All good. > > However on power, cpu supports 'disable-read' too. Since userspace can > program the cpu directly, userspace has the ability to set > 'disable-read' too. This can lead to inconsistency between the kernel > and the userspace. > > We want the kernel to match userspace on all architectures. > > Proposed Solution: > > Enhance the kernel to understand 'disable-read', and facilitate architectures > that understand 'disable-read' to allow it. > > Also explicitly define the semantics of disable-access as > 'disable-read and disable-write' > > Did I get this right? Assuming I did, the implementation has to do > the following -- > >On power, sys_pkey_alloc() should succeed if the init_val >is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS >or any combination of the three. > >On x86, sys_pkey_alloc() should succeed if the init_val is >PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ >or any combination of the three, except PKEY_DISABLE_READ > specified all by itself. > >On all other arches, none of the flags are supported. I don’t really love having a situation where you can use different flag combinations to refer to the same mode. Also, we should document the effect these flags have on execute permission.
Re: pkeys: Reserve PKEY_DISABLE_READ
* Ram Pai: > Ok. here is a patch, compiled but not tested. See if this meets the > specifications. > > --- > > commit 3dc06e73f3795921265d5d1d935e428deab01616 > Author: Ram Pai > Date: Tue Dec 4 00:04:11 2018 -0500 > > pkeys: add support of PKEY_DISABLE_READ Thanks. In the x86 code, the translation of PKEY_DISABLE_READ | PKEY_DISABLE_WRITE to PKEY_DISABLE_ACCESS appears to be missing. I believe the existing code produces PKEY_DISABLE_WRITE, which is wrong. Rest looks okay to me (again not tested). Thanks, Florian
Re: pkeys: Reserve PKEY_DISABLE_READ
On Mon, Dec 03, 2018 at 04:52:02PM +0100, Florian Weimer wrote:
> * Ram Pai:
>
> > So the problem is as follows:
> >
> > Currently the kernel supports 'disable-write' and 'disable-access'.
> >
> > On x86, cpu supports 'disable-write' and 'disable-access'. This
> > matches with what the kernel supports. All good.
> >
> > However on power, cpu supports 'disable-read' too. Since userspace can
> > program the cpu directly, userspace has the ability to set
> > 'disable-read' too. This can lead to inconsistency between the kernel
> > and the userspace.
> >
> > We want the kernel to match userspace on all architectures.
>
> Correct.
>
> > Proposed Solution:
> >
> > Enhance the kernel to understand 'disable-read', and facilitate
> > architectures
> > that understand 'disable-read' to allow it.
> >
> > Also explicitly define the semantics of disable-access as
> > 'disable-read and disable-write'
> >
> > Did I get this right? Assuming I did, the implementation has to do
> > the following --
> >
> > On power, sys_pkey_alloc() should succeed if the init_val
> > is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
> > or any combination of the three.
>
> Agreed.
>
> > On x86, sys_pkey_alloc() should succeed if the init_val is
> > PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
> > or any combination of the three, except PKEY_DISABLE_READ
> > specified all by itself.
>
> Again agreed. That's a clever way of phrasing it actually.
>
> > On all other arches, none of the flags are supported.
> >
> >
> > Are we on the same plate?
>
> I think so, thanks.
>
> Florian
Ok. here is a patch, compiled but not tested. See if this meets the
specifications.
---
commit 3dc06e73f3795921265d5d1d935e428deab01616
Author: Ram Pai
Date: Tue Dec 4 00:04:11 2018 -0500
pkeys: add support of PKEY_DISABLE_READ
Kernel supports 'disable-write' and 'disable-access'.
x86 cpu supports 'disable-write' and 'disable-access'. This
matches with the kernel support.
However POWER cpu supports 'disable-read' too. Since userspace can
program the cpu directly, userspace has the ability to set
'disable-read' too. This can lead to inconsistency between the kernel
and the userspace.
Make kernel match userspace on all architectures.
Enhance the kernel to understand 'disable-read', and facilitate
architectures
that understand 'disable-read' to allow it.
Define the semantics of disable-access as 'disable-read and disable-write'
Signed-off-by: Ram Pai
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 20ebf15..4bd09d0 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -19,11 +19,7 @@
#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
VM_PKEY_BIT3 | VM_PKEY_BIT4)
-/* Override any generic PKEY permission defines */
#define PKEY_DISABLE_EXECUTE 0x4
-#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS | \
- PKEY_DISABLE_WRITE | \
- PKEY_DISABLE_EXECUTE)
static inline u64 pkey_to_vmflag_bits(u16 pkey)
{
@@ -199,6 +195,16 @@ static inline bool arch_pkeys_enabled(void)
return !static_branch_likely(_disabled);
}
+extern bool __arch_pkey_access_rights_valid(unsigned long rights);
+
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ if (static_branch_likely(_disabled))
+ return false;
+
+ return __arch_pkey_access_rights_valid(rights);
+}
+
extern void pkey_mm_init(struct mm_struct *mm);
extern bool arch_supports_pkeys(int cap);
extern unsigned int arch_usable_pkeys(void);
diff --git a/arch/powerpc/include/uapi/asm/mman.h
b/arch/powerpc/include/uapi/asm/mman.h
index 65065ce..e63bc37 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -30,10 +30,4 @@
#define MAP_STACK 0x2 /* give out an address that is best
suited for process/thread stacks */
#define MAP_HUGETLB0x4 /* create a huge page mapping */
-/* 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 5d65c47..a4f288b 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -67,7 +67,7 @@ int pkey_initialize(void)
* Ensure that the bits a distinct.
*/
BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
-(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
+
Re: pkeys: Reserve PKEY_DISABLE_READ
* Ram Pai: > So the problem is as follows: > > Currently the kernel supports 'disable-write' and 'disable-access'. > > On x86, cpu supports 'disable-write' and 'disable-access'. This > matches with what the kernel supports. All good. > > However on power, cpu supports 'disable-read' too. Since userspace can > program the cpu directly, userspace has the ability to set > 'disable-read' too. This can lead to inconsistency between the kernel > and the userspace. > > We want the kernel to match userspace on all architectures. Correct. > Proposed Solution: > > Enhance the kernel to understand 'disable-read', and facilitate architectures > that understand 'disable-read' to allow it. > > Also explicitly define the semantics of disable-access as > 'disable-read and disable-write' > > Did I get this right? Assuming I did, the implementation has to do > the following -- > > On power, sys_pkey_alloc() should succeed if the init_val > is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS > or any combination of the three. Agreed. > On x86, sys_pkey_alloc() should succeed if the init_val is > PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ > or any combination of the three, except PKEY_DISABLE_READ > specified all by itself. Again agreed. That's a clever way of phrasing it actually. > On all other arches, none of the flags are supported. > > > Are we on the same plate? I think so, thanks. Florian
Re: pkeys: Reserve PKEY_DISABLE_READ
On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:
> * Dave Hansen:
>
> > On 11/27/18 3:57 AM, Florian Weimer wrote:
> >> I would have expected something that translates PKEY_DISABLE_WRITE |
> >> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
> >> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
> >>
> >> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
> >> access, but produces execute-only memory.)
> >
> > Correct, it disables all data access, but not execution.
>
> So I would expect something like this (completely untested, I did not
> even compile this):
Ok. I re-read through the entire email thread to understand the problem and
the proposed solution. Let me summarize it below. Lets see if we are on the same
plate.
So the problem is as follows:
Currently the kernel supports 'disable-write' and 'disable-access'.
On x86, cpu supports 'disable-write' and 'disable-access'. This
matches with what the kernel supports. All good.
However on power, cpu supports 'disable-read' too. Since userspace can
program the cpu directly, userspace has the ability to set
'disable-read' too. This can lead to inconsistency between the kernel
and the userspace.
We want the kernel to match userspace on all architectures.
Proposed Solution:
Enhance the kernel to understand 'disable-read', and facilitate architectures
that understand 'disable-read' to allow it.
Also explicitly define the semantics of disable-access as
'disable-read and disable-write'
Did I get this right? Assuming I did, the implementation has to do
the following --
On power, sys_pkey_alloc() should succeed if the init_val
is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
or any combination of the three.
On x86, sys_pkey_alloc() should succeed if the init_val is
PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
or any combination of the three, except PKEY_DISABLE_READ
specified all by itself.
On all other arches, none of the flags are supported.
Are we on the same plate?
RP
>
> diff --git a/arch/powerpc/include/asm/pkeys.h
> b/arch/powerpc/include/asm/pkeys.h
> index 20ebf153c871..bed23f9e8336 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -199,6 +199,11 @@ static inline bool arch_pkeys_enabled(void)
> return !static_branch_likely(_disabled);
> }
>
> +static inline bool arch_pkey_access_rights_valid(unsigned long rights)
> +{
> + return (rights & ~(unsigned long)PKEY_ACCESS_MASK) == 0;
> +}
> +
> extern void pkey_mm_init(struct mm_struct *mm);
> extern bool arch_supports_pkeys(int cap);
> extern unsigned int arch_usable_pkeys(void);
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index 19b137f1b3be..e3e1d5a316e8 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -14,6 +14,17 @@ static inline bool arch_pkeys_enabled(void)
> return boot_cpu_has(X86_FEATURE_OSPKE);
> }
>
> +static inline bool arch_pkey_access_rights_valid(unsigned long rights)
> +{
> + if (rights & ~(unsigned long)PKEY_ACCESS_MASK)
> + return false;
> + if (rights & PKEY_DISABLE_READ) {
> + /* x86 can only disable read access along with write access. */
> + return rights & (PKEY_DISABLE_WRITE | PKEY_DISABLE_ACCESS);
> + }
> + return true;
> +}
> +
> /*
> * Try to dedicate one of the protection keys to be used as an
> * execute-only protection key.
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 87a57b7642d3..b9b78145017f 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -928,7 +928,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk,
> int pkey,
> return -EINVAL;
>
> /* Set the bits we need in PKRU: */
> - if (init_val & PKEY_DISABLE_ACCESS)
> + if (init_val & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ))
> + /*
> + * arch_pkey_access_rights_valid checked that
> + * PKEY_DISABLE_READ is actually representable on x86
> + * (that is, it comes with PKEY_DISABLE_ACCESS or
> + * PKEY_DISABLE_WRITE).
> + */
> new_pkru_bits |= PKRU_AD_BIT;
> if (init_val & PKEY_DISABLE_WRITE)
> new_pkru_bits |= PKRU_WD_BIT;
> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 2955ba976048..2c330fabbe55 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -48,6 +48,11 @@ static inline void copy_init_pkru_to_fpregs(void)
> {
> }
>
> +static inline bool arch_pkey_access_rights_valid(unsigned long rights)
> +{
> + return false;
> +}
> +
> #endif /* ! CONFIG_ARCH_HAS_PKEYS */
>
> #endif /* _LINUX_PKEYS_H */
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 6d331620b9e5..f4cefc3540df 100644
> ---
Re: pkeys: Reserve PKEY_DISABLE_READ
* Dave Hansen:
> On 11/27/18 3:57 AM, Florian Weimer wrote:
>> I would have expected something that translates PKEY_DISABLE_WRITE |
>> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
>> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
>>
>> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
>> access, but produces execute-only memory.)
>
> Correct, it disables all data access, but not execution.
So I would expect something like this (completely untested, I did not
even compile this):
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 20ebf153c871..bed23f9e8336 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -199,6 +199,11 @@ static inline bool arch_pkeys_enabled(void)
return !static_branch_likely(_disabled);
}
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ return (rights & ~(unsigned long)PKEY_ACCESS_MASK) == 0;
+}
+
extern void pkey_mm_init(struct mm_struct *mm);
extern bool arch_supports_pkeys(int cap);
extern unsigned int arch_usable_pkeys(void);
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 19b137f1b3be..e3e1d5a316e8 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -14,6 +14,17 @@ static inline bool arch_pkeys_enabled(void)
return boot_cpu_has(X86_FEATURE_OSPKE);
}
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ if (rights & ~(unsigned long)PKEY_ACCESS_MASK)
+ return false;
+ if (rights & PKEY_DISABLE_READ) {
+ /* x86 can only disable read access along with write access. */
+ return rights & (PKEY_DISABLE_WRITE | PKEY_DISABLE_ACCESS);
+ }
+ return true;
+}
+
/*
* Try to dedicate one of the protection keys to be used as an
* execute-only protection key.
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 87a57b7642d3..b9b78145017f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -928,7 +928,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int
pkey,
return -EINVAL;
/* Set the bits we need in PKRU: */
- if (init_val & PKEY_DISABLE_ACCESS)
+ if (init_val & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ))
+ /*
+* arch_pkey_access_rights_valid checked that
+* PKEY_DISABLE_READ is actually representable on x86
+* (that is, it comes with PKEY_DISABLE_ACCESS or
+* PKEY_DISABLE_WRITE).
+*/
new_pkru_bits |= PKRU_AD_BIT;
if (init_val & PKEY_DISABLE_WRITE)
new_pkru_bits |= PKRU_WD_BIT;
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 2955ba976048..2c330fabbe55 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -48,6 +48,11 @@ static inline void copy_init_pkru_to_fpregs(void)
{
}
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ return false;
+}
+
#endif /* ! CONFIG_ARCH_HAS_PKEYS */
#endif /* _LINUX_PKEYS_H */
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 6d331620b9e5..f4cefc3540df 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -597,7 +597,7 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned
long, init_val)
if (flags)
return -EINVAL;
/* check for unsupported init values */
- if (init_val & ~PKEY_ACCESS_MASK)
+ if (!arch_pkey_access_rights_valid(init_val))
return -EINVAL;
down_write(>mm->mmap_sem);
Thanks,
Florian
Re: pkeys: Reserve PKEY_DISABLE_READ
On 11/27/18 3:57 AM, Florian Weimer wrote: > I would have expected something that translates PKEY_DISABLE_WRITE | > PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts > PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER. > > (My understanding is that PKEY_DISABLE_ACCESS does not disable all > access, but produces execute-only memory.) Correct, it disables all data access, but not execution.
Re: pkeys: Reserve PKEY_DISABLE_READ
* Ram Pai: > diff --git a/arch/x86/include/uapi/asm/mman.h > b/arch/x86/include/uapi/asm/mman.h > index d4a8d04..e9b121b 100644 > --- a/arch/x86/include/uapi/asm/mman.h > +++ b/arch/x86/include/uapi/asm/mman.h > @@ -24,6 +24,11 @@ > ((key) & 0x2 ? VM_PKEY_BIT1 : 0) | \ > ((key) & 0x4 ? VM_PKEY_BIT2 : 0) | \ > ((key) & 0x8 ? VM_PKEY_BIT3 : 0)) > + > +/* Override any generic PKEY permission defines */ > +#undef PKEY_ACCESS_MASK > +#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ > + PKEY_DISABLE_WRITE) > #endif I would have expected something that translates PKEY_DISABLE_WRITE | PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER. (My understanding is that PKEY_DISABLE_ACCESS does not disable all access, but produces execute-only memory.) > diff --git a/include/uapi/asm-generic/mman-common.h > b/include/uapi/asm-generic/mman-common.h > index e7ee328..61168e4 100644 > --- a/include/uapi/asm-generic/mman-common.h > +++ b/include/uapi/asm-generic/mman-common.h > @@ -71,7 +71,8 @@ > > #define PKEY_DISABLE_ACCESS 0x1 > #define PKEY_DISABLE_WRITE 0x2 > -#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ > - PKEY_DISABLE_WRITE) > - > +#define PKEY_DISABLE_EXECUTE 0x4 > +#define PKEY_DISABLE_READ0x8 > +#define PKEY_ACCESS_MASK 0x0 /* arch can override and define its own > +mask bits */ > #endif /* __ASM_GENERIC_MMAN_COMMON_H */ I think Dave requested a value for PKEY_DISABLE_READ which is further away from the existing bits. Thanks, Florian
Re: pkeys: Reserve PKEY_DISABLE_READ
On Mon, Nov 12, 2018 at 01:00:19PM +0100, Florian Weimer wrote:
> * Ram Pai:
>
> > On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
> >> * Ram Pai:
> >>
> >> > Florian,
> >> >
> >> > I can. But I am struggling to understand the requirement. Why is
> >> > this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
> >> > to be able to allocate keys that are initialied to disable-read
> >> > only?
> >>
> >> Yes, I think that would be a natural consequence.
> >>
> >> However, my immediate need comes from the fact that the AMR register can
> >> contain a flag combination that is not possible to represent with the
> >> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
> >> could write to AMR directly, so I cannot rule out that certain flag
> >> combinations exist there.
> >>
> >> So I came up with this:
> >>
> >> int
> >> pkey_get (int key)
> >> {
> >> if (key < 0 || key > PKEY_MAX)
> >> {
> >> __set_errno (EINVAL);
> >> return -1;
> >> }
> >> unsigned int index = pkey_index (key);
> >> unsigned long int amr = pkey_read ();
> >> unsigned int bits = (amr >> index) & 3;
> >>
> >> /* Translate from AMR values. PKEY_AMR_READ standing alone is not
> >> currently representable. */
> >> if (bits & PKEY_AMR_READ)
> >
> > this should be
> >if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))
>
> This would return zero for PKEY_AMR_READ alone.
>
> >> return PKEY_DISABLE_ACCESS;
> >
> >
> >> else if (bits == PKEY_AMR_WRITE)
> >> return PKEY_DISABLE_WRITE;
> >> return 0;
> >> }
>
> It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case.
> Which is why I want PKEY_DISABLE_READ.
>
> >> And this is not ideal. I would prefer something like this instead:
> >>
> >> switch (bits)
> >> {
> >> case PKEY_AMR_READ | PKEY_AMR_WRITE:
> >> return PKEY_DISABLE_ACCESS;
> >> case PKEY_AMR_READ:
> >> return PKEY_DISABLE_READ;
> >> case PKEY_AMR_WRITE:
> >> return PKEY_DISABLE_WRITE;
> >> case 0:
> >> return 0;
> >> }
> >
> > yes.
> > and on x86 it will be something like:
> >switch (bits)
> > {
> >case PKEY_PKRU_ACCESS :
> > return PKEY_DISABLE_ACCESS;
> >case PKEY_AMR_WRITE:
> > return PKEY_DISABLE_WRITE;
> >case 0:
> > return 0;
> > }
>
> x86 returns the PKRU bits directly, including the nonsensical case
> (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE).
>
> > But for this to work, why do you need to enhance the sys_pkey_alloc()
> > interface? Not that I am against it. Trying to understand if the
> > enhancement is really needed.
>
> sys_pkey_alloc performs an implicit pkey_set for the newly allocated key
> (that is, it updates the PKRU/AMR register). It makes sense to match
> the behavior of the userspace implementation.
Here is a untested patch. Does this meet your needs?
It defines the new flags. Each architecture will than define the set of flags
it supports through PKEY_ACCESS_MASK.
Signed-off-by: Ram Pai
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 92a9962..724ef43 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -21,11 +21,6 @@
#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
VM_PKEY_BIT3 | VM_PKEY_BIT4)
-/* Override any generic PKEY permission defines */
-#define PKEY_DISABLE_EXECUTE 0x4
-#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS | \
- PKEY_DISABLE_WRITE | \
- PKEY_DISABLE_EXECUTE)
static inline u64 pkey_to_vmflag_bits(u16 pkey)
{
diff --git a/arch/powerpc/include/uapi/asm/mman.h
b/arch/powerpc/include/uapi/asm/mman.h
index 65065ce..76237b3 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -31,9 +31,9 @@
#define MAP_HUGETLB0x4 /* create a huge page mapping */
/* 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_READ |\
PKEY_DISABLE_EXECUTE)
#endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 4860acd..c8b2540 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -62,14 +62,6 @@ int 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));
-
- /*
*
Re: pkeys: Reserve PKEY_DISABLE_READ
* Ram Pai:
> On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
>> * Ram Pai:
>>
>> > Florian,
>> >
>> >I can. But I am struggling to understand the requirement. Why is
>> >this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
>> >to be able to allocate keys that are initialied to disable-read
>> >only?
>>
>> Yes, I think that would be a natural consequence.
>>
>> However, my immediate need comes from the fact that the AMR register can
>> contain a flag combination that is not possible to represent with the
>> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
>> could write to AMR directly, so I cannot rule out that certain flag
>> combinations exist there.
>>
>> So I came up with this:
>>
>> int
>> pkey_get (int key)
>> {
>> if (key < 0 || key > PKEY_MAX)
>> {
>> __set_errno (EINVAL);
>> return -1;
>> }
>> unsigned int index = pkey_index (key);
>> unsigned long int amr = pkey_read ();
>> unsigned int bits = (amr >> index) & 3;
>>
>> /* Translate from AMR values. PKEY_AMR_READ standing alone is not
>> currently representable. */
>> if (bits & PKEY_AMR_READ)
>
> this should be
>if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))
This would return zero for PKEY_AMR_READ alone.
>> return PKEY_DISABLE_ACCESS;
>
>
>> else if (bits == PKEY_AMR_WRITE)
>> return PKEY_DISABLE_WRITE;
>> return 0;
>> }
It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case.
Which is why I want PKEY_DISABLE_READ.
>> And this is not ideal. I would prefer something like this instead:
>>
>> switch (bits)
>> {
>> case PKEY_AMR_READ | PKEY_AMR_WRITE:
>> return PKEY_DISABLE_ACCESS;
>> case PKEY_AMR_READ:
>> return PKEY_DISABLE_READ;
>> case PKEY_AMR_WRITE:
>> return PKEY_DISABLE_WRITE;
>> case 0:
>> return 0;
>> }
>
> yes.
> and on x86 it will be something like:
>switch (bits)
> {
>case PKEY_PKRU_ACCESS :
> return PKEY_DISABLE_ACCESS;
>case PKEY_AMR_WRITE:
> return PKEY_DISABLE_WRITE;
>case 0:
> return 0;
> }
x86 returns the PKRU bits directly, including the nonsensical case
(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE).
> But for this to work, why do you need to enhance the sys_pkey_alloc()
> interface? Not that I am against it. Trying to understand if the
> enhancement is really needed.
sys_pkey_alloc performs an implicit pkey_set for the newly allocated key
(that is, it updates the PKRU/AMR register). It makes sense to match
the behavior of the userspace implementation.
Thanks,
Florian
Re: pkeys: Reserve PKEY_DISABLE_READ
* Ram Pai: > On Thu, Nov 08, 2018 at 01:05:09PM +0100, Florian Weimer wrote: >> Would it be possible to reserve a bit for PKEY_DISABLE_READ? >> >> I think the POWER implementation can disable read access at the hardware >> level, but not write access, and that cannot be expressed with the >> current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits. > > POWER hardware can disable-read and can **also disable-write** > at the hardware level. It can disable-execute aswell at the > hardware level. For example if the key bits for a given key in the AMR > register is > 0b01 it is read-disable > 0b10 it is write-disable > > To support access-disable, we make the key value 0b11. > > So in case if you want to know if the key is read-disable 'bitwise-and' it > against 0x1. i.e (x & 0x1) Not sure if we covered that alreay, but my problem is that I cannot translate a 0b01 mask to a PKEY_DISABLE_* flag combination with the current flags. 0b10 and 0b11 are fine. POWER also loses the distinction between PKEY_DISABLE_ACCESS and PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE, but that's fine. This breaks the current glibc test case, but I have a patch for that. Arguably, the test is wrong or at least overly strict in what it accepts. Thanks, Florian
Re: pkeys: Reserve PKEY_DISABLE_READ
On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
> * Ram Pai:
>
> > Florian,
> >
> > I can. But I am struggling to understand the requirement. Why is
> > this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
> > to be able to allocate keys that are initialied to disable-read
> > only?
>
> Yes, I think that would be a natural consequence.
>
> However, my immediate need comes from the fact that the AMR register can
> contain a flag combination that is not possible to represent with the
> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
> could write to AMR directly, so I cannot rule out that certain flag
> combinations exist there.
>
> So I came up with this:
>
> int
> pkey_get (int key)
> {
> if (key < 0 || key > PKEY_MAX)
> {
> __set_errno (EINVAL);
> return -1;
> }
> unsigned int index = pkey_index (key);
> unsigned long int amr = pkey_read ();
> unsigned int bits = (amr >> index) & 3;
>
> /* Translate from AMR values. PKEY_AMR_READ standing alone is not
> currently representable. */
> if (bits & PKEY_AMR_READ)
this should be
if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))
> return PKEY_DISABLE_ACCESS;
> else if (bits == PKEY_AMR_WRITE)
> return PKEY_DISABLE_WRITE;
> return 0;
> }
>
> And this is not ideal. I would prefer something like this instead:
>
> switch (bits)
> {
> case PKEY_AMR_READ | PKEY_AMR_WRITE:
> return PKEY_DISABLE_ACCESS;
> case PKEY_AMR_READ:
> return PKEY_DISABLE_READ;
> case PKEY_AMR_WRITE:
> return PKEY_DISABLE_WRITE;
> case 0:
> return 0;
> }
yes.
and on x86 it will be something like:
switch (bits)
{
case PKEY_PKRU_ACCESS :
return PKEY_DISABLE_ACCESS;
case PKEY_AMR_WRITE:
return PKEY_DISABLE_WRITE;
case 0:
return 0;
}
But for this to work, why do you need to enhance the sys_pkey_alloc()
interface? Not that I am against it. Trying to understand if the
enhancement is really needed.
>
> By the way, is the AMR register 64-bit or 32-bit on 32-bit POWER?
It is 64-bit.
RP
Re: pkeys: Reserve PKEY_DISABLE_READ
* Ram Pai:
> Florian,
>
> I can. But I am struggling to understand the requirement. Why is
> this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
> to be able to allocate keys that are initialied to disable-read
> only?
Yes, I think that would be a natural consequence.
However, my immediate need comes from the fact that the AMR register can
contain a flag combination that is not possible to represent with the
existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
could write to AMR directly, so I cannot rule out that certain flag
combinations exist there.
So I came up with this:
int
pkey_get (int key)
{
if (key < 0 || key > PKEY_MAX)
{
__set_errno (EINVAL);
return -1;
}
unsigned int index = pkey_index (key);
unsigned long int amr = pkey_read ();
unsigned int bits = (amr >> index) & 3;
/* Translate from AMR values. PKEY_AMR_READ standing alone is not
currently representable. */
if (bits & PKEY_AMR_READ)
return PKEY_DISABLE_ACCESS;
else if (bits == PKEY_AMR_WRITE)
return PKEY_DISABLE_WRITE;
return 0;
}
And this is not ideal. I would prefer something like this instead:
switch (bits)
{
case PKEY_AMR_READ | PKEY_AMR_WRITE:
return PKEY_DISABLE_ACCESS;
case PKEY_AMR_READ:
return PKEY_DISABLE_READ;
case PKEY_AMR_WRITE:
return PKEY_DISABLE_WRITE;
case 0:
return 0;
}
By the way, is the AMR register 64-bit or 32-bit on 32-bit POWER?
Thanks,
Florian
Re: pkeys: Reserve PKEY_DISABLE_READ
On Thu, Nov 08, 2018 at 06:37:41PM +0100, Florian Weimer wrote: > * Dave Hansen: > > > On 11/8/18 7:01 AM, Florian Weimer wrote: > >> Ideally, PKEY_DISABLE_READ | PKEY_DISABLE_WRITE and PKEY_DISABLE_READ | > >> PKEY_DISABLE_ACCESS would be treated as PKEY_DISABLE_ACCESS both, and a > >> line PKEY_DISABLE_READ would result in an EINVAL failure. > > > > Sounds reasonable to me. > > > > I don't see any urgency to do this right now. It could easily go in > > alongside the ppc patches when those get merged. > > POWER support has already been merged, so we need to do something here > now, before I can complete the userspace side. > > > The only thing I'd suggest is that we make it something slightly > > higher than 0x4. It'll make the code easier to deal with in the > > kernel if we have the ABI and the hardware mirror each other, and if > > we pick 0x4 in the ABI for PKEY_DISABLE_READ, it might get messy if > > the harware choose 0x4 for PKEY_DISABLE_EXECUTE or something. > > > > So, let's make it 0x80 or something on x86 at least. > > I don't have a problem with that if that's what it takes. > > > Also, I'll be happy to review and ack the patch to do this, but I'd > > expect the ppc guys (hi Ram!) to actually put it together. > > Ram, do you want to write a patch? Florian, I can. But I am struggling to understand the requirement. Why is this needed? Are we proposing a enhancement to the sys_pkey_alloc(), to be able to allocate keys that are initialied to disable-read only? RP > > I'll promise I finish the glibc support for this. 8-) > > Thanks, > Florian -- Ram Pai
Re: pkeys: Reserve PKEY_DISABLE_READ
On Thu, Nov 08, 2018 at 01:05:09PM +0100, Florian Weimer wrote: > Would it be possible to reserve a bit for PKEY_DISABLE_READ? > > I think the POWER implementation can disable read access at the hardware > level, but not write access, and that cannot be expressed with the > current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits. POWER hardware can disable-read and can **also disable-write** at the hardware level. It can disable-execute aswell at the hardware level. For example if the key bits for a given key in the AMR register is 0b01 it is read-disable 0b10 it is write-disable To support access-disable, we make the key value 0b11. So in case if you want to know if the key is read-disable 'bitwise-and' it against 0x1. i.e (x & 0x1) RP
