[RFC][PATCH 1/2] x86, pkeys: default to a restrictive init PKRU

[RFC][PATCH 1/2] x86, pkeys: default to a restrictive init PKRU

[Dave Hansen]

Andy Lutomirski brought this up as a potential issue.  It's
straightforward to fix, but has potential performance
implications.

This applies on top of the previous pkeys syscall code that I
posted, but I think we should probably discuss these on their own
and not as a part of the larger series.

---

PKRU is the register that lets you disallow writes or all access
to a given protection key.

The XSAVE hardware defines an "init state" of 0 for PKRU: its
most permissive state, allowing access/writes to everything.
Since we start off all new processes with the init state, we
start all processes off with the most permissive possible PKRU.

This is unfortunate.  If a thread is clone()'d [1] before a
program has time to set PKRU to a restrictive value, that thread
will be able to write to all data, no matter what pkey is set on
it.  This weakens any integrity guarantees that we want pkeys to
provide.

To fix this, we define a very restrictive PKRU to override the
XSAVE-provided value when we create a new FPU context.  We choose
a value that only allows access to pkey 0, which is as
restrictive as we can practically make it.

This does not cause any practical problems with applications
using protection keys because we require them to specify initial
permissions for each key when it is allocated, which override the
restrictive default.

In the end, this ensures that threads which do not know how to
manage their own pkey rights can not do damage to data which is
pkey-protected.

1. I would have thought this was a pretty contrived scenario,
   except that I heard a bug report from an MPX user who was
   creating threads in some very early code before main().  It
   may be crazy, but folks evidently _do_ it.

---

 b/Documentation/kernel-parameters.txt |    5 ++++
 b/arch/x86/include/asm/pkeys.h        |    1 
 b/arch/x86/kernel/fpu/core.c          |    4 +++
 b/arch/x86/mm/pkeys.c                 |   38 ++++++++++++++++++++++++++++++++++
 b/include/linux/pkeys.h               |    4 +++
 5 files changed, 52 insertions(+)

diff -puN arch/x86/mm/pkeys.c~pkeys-140-restrictive-init-pkru arch/x86/mm/pkeys.c
--- a/arch/x86/mm/pkeys.c~pkeys-140-restrictive-init-pkru	2016-07-21 15:27:52.400662462 -0700
+++ b/arch/x86/mm/pkeys.c	2016-07-22 09:35:59.599369792 -0700
@@ -121,3 +121,41 @@ int __arch_override_mprotect_pkey(struct
 	 */
 	return vma_pkey(vma);
 }
+
+#define PKRU_AD_KEY(pkey)	(PKRU_AD_BIT << ((pkey) * PKRU_BITS_PER_PKEY))
+
+/*
+ * Make the default PKRU value (at execve() time) as restrictive
+ * as possible.  This ensures that any threads clone()'d early
+ * in the process's lifetime will not accidentally get access
+ * to data which is pkey-protected later on.
+ */
+u32 init_pkru_value = PKRU_AD_KEY( 1) | PKRU_AD_KEY( 2) | PKRU_AD_KEY( 3) |
+		      PKRU_AD_KEY( 4) | PKRU_AD_KEY( 5) | PKRU_AD_KEY( 6) |
+		      PKRU_AD_KEY( 7) | PKRU_AD_KEY( 8) | PKRU_AD_KEY( 9) |
+		      PKRU_AD_KEY(10) | PKRU_AD_KEY(11) | PKRU_AD_KEY(12) |
+		      PKRU_AD_KEY(13) | PKRU_AD_KEY(14) | PKRU_AD_KEY(15);
+
+/*
+ * Called from the FPU code when creating a fresh set of FPU
+ * registers.  This is called from a very specific context where
+ * we know the FPU regstiers are safe for use and we can use PKRU
+ * directly.  The fact that PKRU is only available when we are
+ * using eagerfpu mode makes this possible.
+ */
+void copy_init_pkru_to_fpregs(void)
+{
+	u32 init_pkru_value_snapshot = READ_ONCE(init_pkru_value);
+	/*
+	 * Any write to PKRU takes it out of the XSAVE 'init
+	 * state' which increases context switch cost.  Avoid
+	 * writing 0 when PKRU was already 0.
+	 */
+	if (!init_pkru_value_snapshot && !read_pkru())
+		return;
+	/*
+	 * Override the PKRU state that came from 'init_fpstate'
+	 * with the baseline from the process.
+	 */
+	write_pkru(init_pkru_value_snapshot);
+}
diff -puN arch/x86/include/asm/pkeys.h~pkeys-140-restrictive-init-pkru arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~pkeys-140-restrictive-init-pkru	2016-07-21 15:27:52.401662507 -0700
+++ b/arch/x86/include/asm/pkeys.h	2016-07-21 15:27:52.913685723 -0700
@@ -100,5 +100,6 @@ extern int arch_set_user_pkey_access(str
 		unsigned long init_val);
 extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val);
+extern void copy_init_pkru_to_fpregs(void);
 
 #endif /*_ASM_X86_PKEYS_H */
diff -puN arch/x86/kernel/fpu/core.c~pkeys-140-restrictive-init-pkru arch/x86/kernel/fpu/core.c
--- a/arch/x86/kernel/fpu/core.c~pkeys-140-restrictive-init-pkru	2016-07-21 15:27:52.908685496 -0700
+++ b/arch/x86/kernel/fpu/core.c	2016-07-21 15:27:52.914685768 -0700
@@ -12,6 +12,7 @@
 #include <asm/traps.h>
 
 #include <linux/hardirq.h>
+#include <linux/pkeys.h>
 
 #define CREATE_TRACE_POINTS
 #include <asm/trace/fpu.h>
@@ -508,6 +509,9 @@ static inline void copy_init_fpstate_to_
 		copy_kernel_to_fxregs(&init_fpstate.fxsave);
 	else
 		copy_kernel_to_fregs(&init_fpstate.fsave);
+
+	if (boot_cpu_has(X86_FEATURE_OSPKE))
+		copy_init_pkru_to_fpregs();
 }
 
 /*
diff -puN include/linux/pkeys.h~pkeys-140-restrictive-init-pkru include/linux/pkeys.h
--- a/include/linux/pkeys.h~pkeys-140-restrictive-init-pkru	2016-07-21 15:27:52.909685542 -0700
+++ b/include/linux/pkeys.h	2016-07-21 15:27:52.914685768 -0700
@@ -35,6 +35,10 @@ static inline int arch_set_user_pkey_acc
 	return 0;
 }
 
+static inline void copy_init_pkru_to_fpregs(void)
+{
+}
+
 #endif /* ! CONFIG_ARCH_HAS_PKEYS */
 
 #endif /* _LINUX_PKEYS_H */
diff -puN Documentation/kernel-parameters.txt~pkeys-140-restrictive-init-pkru Documentation/kernel-parameters.txt
--- a/Documentation/kernel-parameters.txt~pkeys-140-restrictive-init-pkru	2016-07-22 09:37:18.721957333 -0700
+++ b/Documentation/kernel-parameters.txt	2016-07-22 09:39:02.123645370 -0700
@@ -1624,6 +1624,11 @@ bytes respectively. Such letter suffixes
 
 	initrd=		[BOOT] Specify the location of the initial ramdisk
 
+	init_pkru=	[x86] Specify the default memory protection keys rights
+			register contents for all processes.  0x55555554 by
+			default (disallow access to all but pkey 0).  Can
+			override in debugfs after boot.
+
 	inport.irq=	[HW] Inport (ATI XL and Microsoft) busmouse driver
 			Format: <irq>
 
_

[RFC][PATCH 2/2] x86, pkeys: allow configuration of init_pkru

[Dave Hansen]

As discussed in the previous patch, there is a reliability
benefit to allowing an init value for the Protection Keys Rights
User register (PKRU) which differs from what the XSAVE hardware
provides.

But, having PKRU be 0 (its init value) provides some nonzero
amount of optimization potential to the hardware.  It can, for
instance, skip writes to the XSAVE buffer when it knows that PKRU
is in its init state.

The cost of losing this optimization is approximately 100 cycles
per context switch for a workload which lightly using XSAVE
state, basically something not using AVX much.  This is not huge,
but it's also not something that I think we should
unconditionally inflict on everyone.  So, make it configurable
both at boot-time and from debugfs.

Changes to the debugfs value affect all processes created after
the write to debugfs.

PKRU is somewhat special in that it might affect access to the
memory from which the XRSTOR instruction is reading.  Its
behavior is defined to only affect instructions *after* XRSTOR.

---

 b/arch/x86/mm/pkeys.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff -puN arch/x86/mm/pkeys.c~pkeys-141-restrictive-init-pkru-debugfs arch/x86/mm/pkeys.c
--- a/arch/x86/mm/pkeys.c~pkeys-141-restrictive-init-pkru-debugfs	2016-07-22 09:32:29.872859283 -0700
+++ b/arch/x86/mm/pkeys.c	2016-07-22 09:32:29.908860916 -0700
@@ -11,6 +11,7 @@
  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
  * more details.
  */
+#include <linux/debugfs.h>		/* debugfs_create_u32()		*/
 #include <linux/mm_types.h>             /* mm_struct, vma, etc...       */
 #include <linux/pkeys.h>                /* PKEY_*                       */
 #include <uapi/asm-generic/mman-common.h>
@@ -159,3 +160,69 @@ void copy_init_pkru_to_fpregs(void)
 	 */
 	write_pkru(init_pkru_value_snapshot);
 }
+
+static ssize_t init_pkru_read_file(struct file *file, char __user *user_buf,
+			     size_t count, loff_t *ppos)
+{
+	char buf[32];
+	unsigned int len;
+
+	len = sprintf(buf, "0x%x\n", init_pkru_value);
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t init_pkru_write_file(struct file *file,
+		 const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	char buf[32];
+	ssize_t len;
+	u32 new_init_pkru;
+
+	len = min(count, sizeof(buf) - 1);
+	if (copy_from_user(buf, user_buf, len))
+		return -EFAULT;
+
+	/* Make the buffer a valid string that we can not overrun */
+	buf[len] = '\0';
+	if (kstrtouint(buf, 0, &new_init_pkru))
+		return -EINVAL;
+
+	/*
+	 * Don't allow insane settings that will blow the system
+	 * up immediately if someone attempts to disable access
+	 * or writes to pkey 0.
+	 */
+	if (new_init_pkru & (PKRU_AD_BIT|PKRU_WD_BIT))
+		return -EINVAL;
+
+	WRITE_ONCE(init_pkru_value, new_init_pkru);
+	return count;
+}
+
+static const struct file_operations fops_init_pkru = {
+	.read = init_pkru_read_file,
+	.write = init_pkru_write_file,
+	.llseek = default_llseek,
+};
+
+static int __init create_init_pkru_value(void)
+{
+	debugfs_create_file("init_pkru", S_IRUSR | S_IWUSR,
+			arch_debugfs_dir, NULL, &fops_init_pkru);
+	return 0;
+}
+late_initcall(create_init_pkru_value);
+
+static __init int setup_init_pkru(char *opt)
+{
+	u32 new_init_pkru;
+
+	if (kstrtouint(opt, 0, &new_init_pkru))
+		return 1;
+
+	WRITE_ONCE(init_pkru_value, new_init_pkru);
+
+	return 1;
+}
+__setup("init_pkru=", setup_init_pkru);
+
_