Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452465 > unrolled thread

[PATCH 08/10] x86, pkeys: default to a restrictive init PKRU

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU Dave Hansen <dave@sr71.net> - 2016-07-29 18:40 +0200
    Re: [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU Andy Lutomirski <luto@amacapital.net> - 2016-07-29 19:40 +0200
      Re: [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU Dave Hansen <dave.hansen@intel.com> - 2016-07-29 20:00 +0200
        Re: [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU Andy Lutomirski <luto@amacapital.net> - 2016-07-29 21:50 +0200
    Re: [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU Vlastimil Babka <vbabka@suse.cz> - 2016-08-01 16:50 +0200
      Re: [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU Dave Hansen <dave@sr71.net> - 2016-08-01 17:10 +0200
        Re: [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU Vlastimil Babka <vbabka@suse.cz> - 2016-08-02 10:30 +0200

#1452465 — [PATCH 08/10] x86, pkeys: default to a restrictive init PKRU

From: Dave Hansen <dave.hansen@linux.intel.com>

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.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: x86@kernel.org
Cc: torvalds@linux-foundation.org
Cc: akpm@linux-foundation.org
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: mgorman@techsingularity.net
---

 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/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-29 09:18:59.277601034 -0700
+++ b/arch/x86/include/asm/pkeys.h	2016-07-29 09:18:59.289601577 -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-29 09:18:59.278601079 -0700
+++ b/arch/x86/kernel/fpu/core.c	2016-07-29 09:18:59.289601577 -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>
@@ -505,6 +506,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 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-29 09:18:59.281601215 -0700
+++ b/arch/x86/mm/pkeys.c	2016-07-29 09:18:59.290601622 -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 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-29 09:18:59.284601351 -0700
+++ b/Documentation/kernel-parameters.txt	2016-07-29 09:18:59.293601758 -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>
 
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-29 09:18:59.286601441 -0700
+++ b/include/linux/pkeys.h	2016-07-29 09:18:59.293601758 -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 */
_

[toc] | [next] | [standalone]

#1452491

On Fri, Jul 29, 2016 at 9:30 AM, Dave Hansen <dave@sr71.net> wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> 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.

I think you missed the fpu__clear() caller in kernel/fpu/signal.c.

ISTM it might be more comprehensible to change fpu__clear in general
and then special case things you want to behave differently.

[toc] | [prev] | [next] | [standalone]

#1452500

On 07/29/2016 10:29 AM, Andy Lutomirski wrote:
>> > 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.
> I think you missed the fpu__clear() caller in kernel/fpu/signal.c.
> 
> ISTM it might be more comprehensible to change fpu__clear in general
> and then special case things you want to behave differently.

The code actually already patched the generic fpu__clear():

	fpu__clear() ->
	copy_init_fpstate_to_fpregs() ->
	copy_init_pkru_to_fpregs()

So I think it hit the case you are talking about.

[toc] | [prev] | [next] | [standalone]

#1452532

On Fri, Jul 29, 2016 at 10:50 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> On 07/29/2016 10:29 AM, Andy Lutomirski wrote:
>>> > 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.
>> I think you missed the fpu__clear() caller in kernel/fpu/signal.c.
>>
>> ISTM it might be more comprehensible to change fpu__clear in general
>> and then special case things you want to behave differently.
>
> The code actually already patched the generic fpu__clear():
>
>         fpu__clear() ->
>         copy_init_fpstate_to_fpregs() ->
>         copy_init_pkru_to_fpregs()
>
> So I think it hit the case you are talking about.

Whoops, missed that.

-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1453255

On 07/29/2016 06:30 PM, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> 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.

Here you mean the init_access_rights parameter of pkey_alloc()? So will 
children of fork() after that pkey_alloc() inherit the new value or go 
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.
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

[toc] | [prev] | [next] | [standalone]

#1453263

On 08/01/2016 07:42 AM, Vlastimil Babka wrote:
> On 07/29/2016 06:30 PM, Dave Hansen wrote:
>> 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.
> 
> Here you mean the init_access_rights parameter of pkey_alloc()? So will
> children of fork() after that pkey_alloc() inherit the new value or go
> default?

Hi Vlastimil,

Yes, exactly, the initial permissions are provided via pkey_alloc()'s
'init_access_rights' argument.

Do you mean fork() or clone()?  In both cases, we actually copy the FPU
state from the parent, so children always inherit the state from their
parent which contains the permissions set by the parent's calls to
pkey_alloc().

[toc] | [prev] | [next] | [standalone]

#1453662

On 08/01/2016 04:58 PM, Dave Hansen wrote:
> On 08/01/2016 07:42 AM, Vlastimil Babka wrote:
>> On 07/29/2016 06:30 PM, Dave Hansen wrote:
>>> 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.
>>
>> Here you mean the init_access_rights parameter of pkey_alloc()? So will
>> children of fork() after that pkey_alloc() inherit the new value or go
>> default?
>
> Hi Vlastimil,
>
> Yes, exactly, the initial permissions are provided via pkey_alloc()'s
> 'init_access_rights' argument.

OK. I was a bit sceptical of that part of the syscall, as you removed 
other syscalls changing PKRU for the thread in kernel, so leaving this 
seemed odd. But it makes sense to me together with the restrictive default.

> Do you mean fork() or clone()?  In both cases, we actually copy the FPU
> state from the parent, so children always inherit the state from their
> parent which contains the permissions set by the parent's calls to
> pkey_alloc().

I meant just fork() as I misunderstood the changelog in that clone() is 
different. Thanks for clarifying.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web