[PATCH 1/2] x86/pkeys: copy pkey state at fork()

[PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Dave Hansen]

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

Our creation of new mm's is a bit convoluted.  At fork(), the code
does:

	1. memcpy() the parent mm to initialize child
	2. mm_init() to initalize some select stuff stuff
	3. dup_mmap() to create true copies that memcpy()
	   did not do right.

For pkeys, we need to preserve two bits of state across a fork:
'execute_only_pkey' and 'pkey_allocation_map'.  Those are preserved by
the memcpy(), which I thought did the right thing.  But, mm_init()
calls init_new_context(), which I thought was *only* for execve()-time
and overwrites 'execute_only_pkey' and 'pkey_allocation_map' with
"new" values.

The result is that, after a fork(), the child's pkey state ends up
looking like it does after an execve(), which is totally wrong.  pkeys
that are already allocated can be allocated again, for instance.

To fix this, add code called by dup_mmap() to copy the pkey state from
parent to child explicitly.  Also add a comment above init_new_context()
to make it more clear to the next poor sod what this code is used for.

Fixes: e8c24d3a23a ("x86/pkeys: Allocation/free syscalls")
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Joerg Roedel <jroedel@suse.de>
---

 b/arch/x86/include/asm/mmu_context.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff -puN arch/x86/include/asm/mmu_context.h~x86-pkeys-no-init-at-fork arch/x86/include/asm/mmu_context.h
--- a/arch/x86/include/asm/mmu_context.h~x86-pkeys-no-init-at-fork	2018-10-26 09:24:09.444102622 -0700
+++ b/arch/x86/include/asm/mmu_context.h	2018-10-26 09:24:09.451102622 -0700
@@ -178,6 +178,10 @@ static inline void switch_ldt(struct mm_
 
 void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk);
 
+/*
+ * Init a new mm.  Used on mm copies, like at fork()
+ * and on mm's that are brand-new, like at execve().
+ */
 static inline int init_new_context(struct task_struct *tsk,
 				   struct mm_struct *mm)
 {
@@ -228,8 +232,22 @@ do {						\
 } while (0)
 #endif
 
+static inline void arch_dup_pkeys(struct mm_struct *oldmm,
+				  struct mm_struct *mm)
+{
+#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
+	if (!cpu_feature_enabled(X86_FEATURE_OSPKE))
+		return;
+
+	/* Duplicate the oldmm pkey state in mm: */
+	mm->context.pkey_allocation_map = oldmm->context.pkey_allocation_map;
+	mm->context.execute_only_pkey   = oldmm->context.execute_only_pkey;
+#endif
+}
+
 static inline int arch_dup_mmap(struct mm_struct *oldmm, struct mm_struct *mm)
 {
+	arch_dup_pkeys(oldmm, mm);
 	paravirt_arch_dup_mmap(oldmm, mm);
 	return ldt_dup_context(oldmm, mm);
 }
_

[PATCH 2/2] x86/selftests/pkeys: fork() to check for state being preserved

[Dave Hansen]

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

There was a bug where the per-mm pkey state was not being preserved
across fork() in the child.  fork() is performed in the pkey
selftests, but all of our pkey activity is performed in the parent.
The child does not perform any actions sensitive to pkey state.

To make the test more sensitive to these kinds of bugs, add a
fork() where we let the parent exit, and continue execution in
the child.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Joerg Roedel <jroedel@suse.de>
---

 b/tools/testing/selftests/x86/protection_keys.c |   30 ++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff -puN tools/testing/selftests/x86/protection_keys.c~x86-pkeys-no-init-at-fork-selftests tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~x86-pkeys-no-init-at-fork-selftests	2018-10-26 09:26:51.970102217 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-10-26 09:26:51.974102217 -0700
@@ -1133,6 +1133,21 @@ void test_pkey_syscalls_bad_args(int *pt
 	pkey_assert(err);
 }
 
+void become_child(void)
+{
+	pid_t forkret;
+
+	forkret = fork();
+	pkey_assert(forkret >= 0);
+	dprintf3("[%d] fork() ret: %d\n", getpid(), forkret);
+
+	if (!forkret) {
+		/* in the child */
+		return;
+	}
+	exit(0);
+}
+
 /* Assumes that all pkeys other than 'pkey' are unallocated */
 void test_pkey_alloc_exhaust(int *ptr, u16 pkey)
 {
@@ -1141,7 +1156,7 @@ void test_pkey_alloc_exhaust(int *ptr, u
 	int nr_allocated_pkeys = 0;
 	int i;
 
-	for (i = 0; i < NR_PKEYS*2; i++) {
+	for (i = 0; i < NR_PKEYS*3; i++) {
 		int new_pkey;
 		dprintf1("%s() alloc loop: %d\n", __func__, i);
 		new_pkey = alloc_pkey();
@@ -1152,10 +1167,17 @@ void test_pkey_alloc_exhaust(int *ptr, u
 		if ((new_pkey == -1) && (errno == ENOSPC)) {
 			dprintf2("%s() failed to allocate pkey after %d tries\n",
 				__func__, nr_allocated_pkeys);
-			break;
+		} else {
+			pkey_assert(nr_allocated_pkeys < NR_PKEYS);
+			allocated_pkeys[nr_allocated_pkeys++] = new_pkey;
 		}
-		pkey_assert(nr_allocated_pkeys < NR_PKEYS);
-		allocated_pkeys[nr_allocated_pkeys++] = new_pkey;
+
+		/*
+		 * Make sure that allocation state is properly
+		 * preserved across fork().
+		 */
+		if (i == NR_PKEYS*2)
+			become_child();
 	}
 
 	dprintf3("%s()::%d\n", __func__, __LINE__);
_

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Dave Hansen]

On 10/26/18 12:51 PM, Dave Hansen wrote:
...
> The result is that, after a fork(), the child's pkey state ends up
> looking like it does after an execve(), which is totally wrong.  pkeys
> that are already allocated can be allocated again, for instance.

One thing I omitted.  This was very nicely discovered and reported by
danielmicay@gmail.com.  Thanks, Daniel!

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Daniel Micay]

I ended up working around this with a pthread_atfork handler disabling
my usage of the feature in the child process for the time being. I
don't have an easy way to detect if the bug is present within a
library so I'm going to need a kernel version check with a table of
kernel releases fixing the problem for each stable branch.

It would be helpful if there was a new cpuinfo flag to check if the
MPK state is preserved on fork in addition to the existing ospke flag.
The problem will fade away over time but in my experience there are a
lot of people using distributions with kernels not incorporating all
of the stable fixes. I expect other people will run into the problem
once hardware with MPK is more widely available and other people try
to use it for various things like moving GC or assorted security
features. Someone will end up running software adopting it on an older
kernel with the problem.

The clobbering issue I found with MAP_FIXED_NOREPLACE isn't quite
as annoying because it was easy to make a runtime test usable in a library
to see if the feature works properly.

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Andy Lutomirski]

> On Oct 26, 2018, at 2:39 PM, Daniel Micay <danielmicay@gmail.com> wrote:
> 
> I ended up working around this with a pthread_atfork handler disabling
> my usage of the feature in the child process for the time being. I
> don't have an easy way to detect if the bug is present within a
> library so

Can you not just make sure that the fix is backported to all relevant kernels?

I suppose we could add a new flag for pkey_get() or something.

> I'm going to need a kernel version check with a table of
> kernel releases fixing the problem for each stable branch.

That won’t work right on district kernels. Please don’t go there.

> 
> It would be helpful if there was a new cpuinfo flag to check if the
> MPK state is preserved on fork in addition to the existing ospke flag.
> The problem will fade away over time but in my experience there are a
> lot of people using distributions with kernels not incorporating all
> of the stable fixes. I expect other people will run into the problem
> once hardware with MPK is more widely available and other people try
> to use it for various things like moving GC or assorted security
> features. Someone will end up running software adopting it on an older
> kernel with the problem.
> 
> The clobbering issue I found with MAP_FIXED_NOREPLACE isn't quite
> as annoying because it was easy to make a runtime test usable in a library
> to see if the feature works properly.

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Daniel Micay]

On Fri, 26 Oct 2018 at 18:12, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Oct 26, 2018, at 2:39 PM, Daniel Micay <danielmicay@gmail.com> wrote:
> >
> > I ended up working around this with a pthread_atfork handler disabling
> > my usage of the feature in the child process for the time being. I
> > don't have an easy way to detect if the bug is present within a
> > library so
>
> Can you not just make sure that the fix is backported to all relevant kernels?

There are too many different distribution kernels and I won't be in
control of where the software is used.

> I suppose we could add a new flag for pkey_get() or something.

That would work, since I can apply the workaround (disabling the
feature in child processes) if I get EINVAL. The flag wouldn't need to
do anything, just existing and being tied to this patch so I have a
way to detect that I can safely use MPK after fork.

> > I'm going to need a kernel version check with a table of
> > kernel releases fixing the problem for each stable branch.
>
> That won’t work right on district kernels. Please don’t go there.

If it's backported to an earlier version, it will just mean missing a
chance to use it. I'm not going to assume that it behaves a certain
way based on having an old kernel, but rather I just won't use it in a
forked child on an older kernel version if I don't have a way to
detect the problem. It's for a few different uses in library code so
I can't have a runtime test trying to detect it with clone(...). I'd
definitely prefer a proper way to detect that I can use it after fork. I really
don't want to have a hack like that which is why I'm bringing it up.

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Ingo Molnar]

* Daniel Micay <danielmicay@gmail.com> wrote:

> > I suppose we could add a new flag for pkey_get() or something.
> 
> That would work, since I can apply the workaround (disabling the
> feature in child processes) if I get EINVAL. The flag wouldn't need to
> do anything, just existing and being tied to this patch so I have a
> way to detect that I can safely use MPK after fork.

A new flag for the pkey_alloc() syscall, right?

Thanks,

	Ingo

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Daniel Micay]

Yeah, a no-op pkey_alloc flag tied to this patch to provide a way to
detect if pkey state is preserved on fork, since kernels without the
patch would report EINVAL. Something like
PKEY_ASSERT_FORK_INHERIT_STATE would make sense. Otherwise, it's
going to be quite painful to adopt this in userspace software without kernel
version checks. Most software can't accept causing a crash / abort
after forking in environments not applying all the stable kernel
patches, or in fresh OS installs that aren't fully updated + rebooted yet.

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Kees Cook]

On Fri, Oct 26, 2018 at 12:59 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> On 10/26/18 12:51 PM, Dave Hansen wrote:
> ...
>> The result is that, after a fork(), the child's pkey state ends up
>> looking like it does after an execve(), which is totally wrong.  pkeys
>> that are already allocated can be allocated again, for instance.
>
> One thing I omitted.  This was very nicely discovered and reported by
> danielmicay@gmail.com.  Thanks, Daniel!

Thread ping. Is there a v2 of this, or can this go in as-is? Looks good to me:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook

Re: [PATCH 1/2] x86/pkeys: copy pkey state at fork()

[Kees Cook]

On Tue, Nov 20, 2018 at 1:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Oct 26, 2018 at 12:59 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> > On 10/26/18 12:51 PM, Dave Hansen wrote:
> > ...
> >> The result is that, after a fork(), the child's pkey state ends up
> >> looking like it does after an execve(), which is totally wrong.  pkeys
> >> that are already allocated can be allocated again, for instance.
> >
> > One thing I omitted.  This was very nicely discovered and reported by
> > danielmicay@gmail.com.  Thanks, Daniel!
>
> Thread ping. Is there a v2 of this, or can this go in as-is? Looks good to me:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>

Ingo, can you take these two patches? Use of pkeys is still buggy
without it. (And probably should have a Cc: stable tag too.)

-Kees

-- 
Kees Cook