Re: pkeys: Reserve PKEY_DISABLE_READ

* Dave Hansen:

On 11/27/18 3:57 AM, Florian Weimer wrote:

quoted

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(&pkey_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(&current->mm->mmap_sem);

Thanks,
Florian

On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:

* Dave Hansen:

quoted

On 11/27/18 3:57 AM, Florian Weimer wrote:

quoted

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

-- 
Ram Pai

On Dec 2, 2018, at 8:02 PM, Ram Pai [off-list ref] wrote:

quoted

On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:
* Dave Hansen:

quoted

quoted

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.

On Wed, Dec 05, 2018 at 08:21:02AM -0800, Andy Lutomirski wrote:

quoted

On Dec 2, 2018, at 8:02 PM, Ram Pai [off-list ref] wrote:

quoted

On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:
* Dave Hansen:

quoted

quoted

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