[PATCH 0/9] [v2] x86, pkeys: two protection keys bug fixes

[PATCH 0/9] [v2] x86, pkeys: two protection keys bug fixes

[Dave Hansen]

Changes from v1:
 * Added Fixes: and cc'd stable.  No code changes.

--

This fixes two bugs, and adds selftests to make sure they stay fixed:

1. pkey 0 was not usable via mprotect_pkey() because it had never
   been explicitly allocated.
2. mprotect(PROT_EXEC) memory could sometimes be left with the
   implicit exec-only protection key assigned.

I already posted #1 previously.  I'm including them both here because
I don't think it's been picked up in case folks want to pull these
all in a single bundle.

Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>p
Cc: Shuah Khan <shuah@kernel.org>
Cc: Shakeel Butt <shakeelb@google.com>

[PATCH 1/9] x86, pkeys: do not special case protection key 0

[Dave Hansen]

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

mm_pkey_is_allocated() treats pkey 0 as unallocated.  That is
inconsistent with the manpages, and also inconsistent with
mm->context.pkey_allocation_map.  Stop special casing it and only
disallow values that are actually bad (< 0).

The end-user visible effect of this is that you can now use
mprotect_pkey() to set pkey=0.

This is a bit nicer than what Ram proposed because it is simpler
and removes special-casing for pkey 0.  On the other hand, it does
allow applciations to pkey_free() pkey-0, but that's just a silly
thing to do, so we are not going to protect against it.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Fixes: 58ab9a088dda ("x86/pkeys: Check against max pkey to avoid overflows")
Cc: stable@kernel.org
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>p
Cc: Shuah Khan <shuah@kernel.org>
---

 b/arch/x86/include/asm/mmu_context.h |    2 +-
 b/arch/x86/include/asm/pkeys.h       |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff -puN arch/x86/include/asm/mmu_context.h~x86-pkey-0-default-allocated arch/x86/include/asm/mmu_context.h
--- a/arch/x86/include/asm/mmu_context.h~x86-pkey-0-default-allocated	2018-03-26 10:22:33.742170197 -0700
+++ b/arch/x86/include/asm/mmu_context.h	2018-03-26 10:22:33.747170197 -0700
@@ -192,7 +192,7 @@ static inline int init_new_context(struc
 
 #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
 	if (cpu_feature_enabled(X86_FEATURE_OSPKE)) {
-		/* pkey 0 is the default and always allocated */
+		/* pkey 0 is the default and allocated implicitly */
 		mm->context.pkey_allocation_map = 0x1;
 		/* -1 means unallocated or invalid */
 		mm->context.execute_only_pkey = -1;
diff -puN arch/x86/include/asm/pkeys.h~x86-pkey-0-default-allocated arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~x86-pkey-0-default-allocated	2018-03-26 10:22:33.744170197 -0700
+++ b/arch/x86/include/asm/pkeys.h	2018-03-26 10:22:33.747170197 -0700
@@ -49,10 +49,10 @@ bool mm_pkey_is_allocated(struct mm_stru
 {
 	/*
 	 * "Allocated" pkeys are those that have been returned
-	 * from pkey_alloc().  pkey 0 is special, and never
-	 * returned from pkey_alloc().
+	 * from pkey_alloc() or pkey 0 which is allocated
+	 * implicitly when the mm is created.
 	 */
-	if (pkey <= 0)
+	if (pkey < 0)
 		return false;
 	if (pkey >= arch_max_pkey())
 		return false;
_

[PATCH 2/9] x86, pkeys, selftests: save off 'prot' for allocations

[Dave Hansen]

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

This makes it possible to to tell what 'prot' a given allocation
is supposed to have.  That way, if we want to change just the
pkey, we know what 'prot' to pass to mprotect_pkey().

Also, keep a record of the most recent allocation so the tests
can easily find it.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

 b/tools/testing/selftests/x86/protection_keys.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff -puN tools/testing/selftests/x86/protection_keys.c~pkeys-update-selftests-store-malloc-record tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~pkeys-update-selftests-store-malloc-record	2018-03-26 10:22:34.301170195 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-03-26 10:22:34.305170195 -0700
@@ -674,10 +674,12 @@ int mprotect_pkey(void *ptr, size_t size
 struct pkey_malloc_record {
 	void *ptr;
 	long size;
+	int prot;
 };
 struct pkey_malloc_record *pkey_malloc_records;
+struct pkey_malloc_record *pkey_last_malloc_record;
 long nr_pkey_malloc_records;
-void record_pkey_malloc(void *ptr, long size)
+void record_pkey_malloc(void *ptr, long size, int prot)
 {
 	long i;
 	struct pkey_malloc_record *rec = NULL;
@@ -709,6 +711,8 @@ void record_pkey_malloc(void *ptr, long
 		(int)(rec - pkey_malloc_records), rec, ptr, size);
 	rec->ptr = ptr;
 	rec->size = size;
+	rec->prot = prot;
+	pkey_last_malloc_record = rec;
 	nr_pkey_malloc_records++;
 }
 
@@ -753,7 +757,7 @@ void *malloc_pkey_with_mprotect(long siz
 	pkey_assert(ptr != (void *)-1);
 	ret = mprotect_pkey((void *)ptr, PAGE_SIZE, prot, pkey);
 	pkey_assert(!ret);
-	record_pkey_malloc(ptr, size);
+	record_pkey_malloc(ptr, size, prot);
 	rdpkru();
 
 	dprintf1("%s() for pkey %d @ %p\n", __func__, pkey, ptr);
@@ -774,7 +778,7 @@ void *malloc_pkey_anon_huge(long size, i
 	size = ALIGN_UP(size, HPAGE_SIZE * 2);
 	ptr = mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
 	pkey_assert(ptr != (void *)-1);
-	record_pkey_malloc(ptr, size);
+	record_pkey_malloc(ptr, size, prot);
 	mprotect_pkey(ptr, size, prot, pkey);
 
 	dprintf1("unaligned ptr: %p\n", ptr);
@@ -847,7 +851,7 @@ void *malloc_pkey_hugetlb(long size, int
 	pkey_assert(ptr != (void *)-1);
 	mprotect_pkey(ptr, size, prot, pkey);
 
-	record_pkey_malloc(ptr, size);
+	record_pkey_malloc(ptr, size, prot);
 
 	dprintf1("mmap()'d hugetlbfs for pkey %d @ %p\n", pkey, ptr);
 	return ptr;
@@ -869,7 +873,7 @@ void *malloc_pkey_mmap_dax(long size, in
 
 	mprotect_pkey(ptr, size, prot, pkey);
 
-	record_pkey_malloc(ptr, size);
+	record_pkey_malloc(ptr, size, prot);
 
 	dprintf1("mmap()'d for pkey %d @ %p\n", pkey, ptr);
 	close(fd);
_

[PATCH 3/9] x86, pkeys, selftests: add a test for pkey 0

[Dave Hansen]

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

Protection key 0 is the default key for all memory and will
not normally come back from pkey_alloc().  But, you might
still want pass it to mprotect_pkey().

This check ensures that you can use pkey 0.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

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

diff -puN tools/testing/selftests/x86/protection_keys.c~pkeys-update-selftests-with-pkey-0-test tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~pkeys-update-selftests-with-pkey-0-test	2018-03-26 10:22:34.841170194 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-03-26 10:22:34.844170194 -0700
@@ -1169,6 +1169,35 @@ void test_pkey_alloc_exhaust(int *ptr, u
 	}
 }
 
+/*
+ * pkey 0 is special.  It is allocated by default, so you do not
+ * have to call pkey_alloc() to use it first.  Make sure that it
+ * is usable.
+ */
+void test_mprotect_with_pkey_0(int *ptr, u16 pkey)
+{
+	long size;
+	int prot;
+
+	assert(pkey_last_malloc_record);
+	size = pkey_last_malloc_record->size;
+	/*
+	 * This is a bit of a hack.  But mprotect() requires
+	 * huge-page-aligned sizes when operating on hugetlbfs.
+	 * So, make sure that we use something that's a multiple
+	 * of a huge page when we can.
+	 */
+	if (size >= HPAGE_SIZE)
+		size = HPAGE_SIZE;
+	prot = pkey_last_malloc_record->prot;
+
+	/* Use pkey 0 */
+	mprotect_pkey(ptr, size, prot, 0);
+
+	/* Make sure that we can set it back to the original pkey. */
+	mprotect_pkey(ptr, size, prot, pkey);
+}
+
 void test_ptrace_of_child(int *ptr, u16 pkey)
 {
 	__attribute__((__unused__)) int peek_result;
@@ -1306,6 +1335,7 @@ void (*pkey_tests[])(int *ptr, u16 pkey)
 	test_kernel_gup_of_access_disabled_region,
 	test_kernel_gup_write_to_write_disabled_region,
 	test_executing_on_unreadable_memory,
+	test_mprotect_with_pkey_0,
 	test_ptrace_of_child,
 	test_pkey_syscalls_on_non_allocated_pkey,
 	test_pkey_syscalls_bad_args,
_

[PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

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

I got a bug report that the following code (roughly) was
causing a SIGSEGV:

	mprotect(ptr, size, PROT_EXEC);
	mprotect(ptr, size, PROT_NONE);
	mprotect(ptr, size, PROT_READ);
	*ptr = 100;

The problem is hit when the mprotect(PROT_EXEC)
is implicitly assigned a protection key to the VMA, and made
that key ACCESS_DENY|WRITE_DENY.  The PROT_NONE mprotect()
failed to remove the protection key, and the PROT_NONE->
PROT_READ left the PTE usable, but the pkey still in place
and left the memory inaccessible.

To fix this, we ensure that we always "override" the pkee
at mprotect() if the VMA does not have execute-only
permissions, but the VMA has the execute-only pkey.

We had a check for PROT_READ/WRITE, but it did not work
for PROT_NONE.  This entirely removes the PROT_* checks,
which ensures that PROT_NONE now works.

Reported-by: Shakeel Butt <shakeelb@google.com>

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
Cc: stable@kernel.org
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

 b/arch/x86/include/asm/pkeys.h |   12 +++++++++++-
 b/arch/x86/mm/pkeys.c          |   19 ++++++++++---------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff -puN arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively	2018-03-26 10:22:35.380170193 -0700
+++ b/arch/x86/include/asm/pkeys.h	2018-03-26 10:22:35.385170193 -0700
@@ -2,6 +2,8 @@
 #ifndef _ASM_X86_PKEYS_H
 #define _ASM_X86_PKEYS_H
 
+#define ARCH_DEFAULT_PKEY	0
+
 #define arch_max_pkey() (boot_cpu_has(X86_FEATURE_OSPKE) ? 16 : 1)
 
 extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
@@ -15,7 +17,7 @@ extern int __execute_only_pkey(struct mm
 static inline int execute_only_pkey(struct mm_struct *mm)
 {
 	if (!boot_cpu_has(X86_FEATURE_OSPKE))
-		return 0;
+		return ARCH_DEFAULT_PKEY;
 
 	return __execute_only_pkey(mm);
 }
@@ -56,6 +58,14 @@ bool mm_pkey_is_allocated(struct mm_stru
 		return false;
 	if (pkey >= arch_max_pkey())
 		return false;
+	/*
+	 * The exec-only pkey is set in the allocation map, but
+	 * is not available to any of the user interfaces like
+	 * mprotect_pkey().
+	 */
+	if (pkey == mm->context.execute_only_pkey)
+		return false;
+
 	return mm_pkey_allocation_map(mm) & (1U << pkey);
 }
 
diff -puN arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/mm/pkeys.c
--- a/arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively	2018-03-26 10:22:35.381170193 -0700
+++ b/arch/x86/mm/pkeys.c	2018-03-26 10:22:35.385170193 -0700
@@ -94,15 +94,7 @@ int __arch_override_mprotect_pkey(struct
 	 */
 	if (pkey != -1)
 		return pkey;
-	/*
-	 * Look for a protection-key-drive execute-only mapping
-	 * which is now being given permissions that are not
-	 * execute-only.  Move it back to the default pkey.
-	 */
-	if (vma_is_pkey_exec_only(vma) &&
-	    (prot & (PROT_READ|PROT_WRITE))) {
-		return 0;
-	}
+
 	/*
 	 * The mapping is execute-only.  Go try to get the
 	 * execute-only protection key.  If we fail to do that,
@@ -113,7 +105,16 @@ int __arch_override_mprotect_pkey(struct
 		pkey = execute_only_pkey(vma->vm_mm);
 		if (pkey > 0)
 			return pkey;
+	} else if (vma_is_pkey_exec_only(vma)) {
+		/*
+		 * Protections are *not* PROT_EXEC, but the mapping
+		 * is using the exec-only pkey.  This mapping was
+		 * PROT_EXEC and will no longer be.  Move back to
+		 * the default pkey.
+		 */
+		return ARCH_DEFAULT_PKEY;
 	}
+
 	/*
 	 * This is a vanilla, non-pkey mprotect (or we failed to
 	 * setup execute-only), inherit the pkey from the VMA we
_

[PATCH 5/9] x86, pkeys, selftests: fix pointer math

[Dave Hansen]

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

We dump out the entire area of the siginfo where the
si_pkey_ptr is supposed to be.  But, we do some math
on the poitner, which is a u32.  We intended to do
byte math, not u32 math on the pointer.

Cast it over to a u8* so it works.

Also, move this block of code to below th si_code
check.  It doesn't hurt anything, but the si_pkey
field is gibberish for other signal types.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

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

diff -puN tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-fix-pointer-math tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-fix-pointer-math	2018-03-26 10:22:35.940170191 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-03-26 10:22:35.944170191 -0700
@@ -289,13 +289,6 @@ void signal_handler(int signum, siginfo_
 		dump_mem(pkru_ptr - 128, 256);
 	pkey_assert(*pkru_ptr);
 
-	si_pkey_ptr = (u32 *)(((u8 *)si) + si_pkey_offset);
-	dprintf1("si_pkey_ptr: %p\n", si_pkey_ptr);
-	dump_mem(si_pkey_ptr - 8, 24);
-	siginfo_pkey = *si_pkey_ptr;
-	pkey_assert(siginfo_pkey < NR_PKEYS);
-	last_si_pkey = siginfo_pkey;
-
 	if ((si->si_code == SEGV_MAPERR) ||
 	    (si->si_code == SEGV_ACCERR) ||
 	    (si->si_code == SEGV_BNDERR)) {
@@ -303,6 +296,13 @@ void signal_handler(int signum, siginfo_
 		exit(4);
 	}
 
+	si_pkey_ptr = (u32 *)(((u8 *)si) + si_pkey_offset);
+	dprintf1("si_pkey_ptr: %p\n", si_pkey_ptr);
+	dump_mem((u8 *)si_pkey_ptr - 8, 24);
+	siginfo_pkey = *si_pkey_ptr;
+	pkey_assert(siginfo_pkey < NR_PKEYS);
+	last_si_pkey = siginfo_pkey;
+
 	dprintf1("signal pkru from xsave: %08x\n", *pkru_ptr);
 	/* need __rdpkru() version so we do not do shadow_pkru checking */
 	dprintf1("signal pkru from  pkru: %08x\n", __rdpkru());
_

[PATCH 6/9] x86, pkeys, selftests: fix pkey exhaustion test off-by-one

[Dave Hansen]

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

In our "exhaust all pkeys" test, we make sure that there
is the expected number available.  Turns out that the
test did not cover the execute-only key, but discussed
it anyway.  It did *not* discuss the test-allocated
key.

Now that we have a test for the mprotect(PROT_EXEC) case,
this off-by-one issue showed itself.  Correct the off-by-
one and add the explanation for the case we missed.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

 b/tools/testing/selftests/x86/protection_keys.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff -puN tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-exhaust-off-by-one tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-exhaust-off-by-one	2018-03-26 10:22:36.477170190 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-03-26 10:22:36.480170190 -0700
@@ -1155,12 +1155,15 @@ void test_pkey_alloc_exhaust(int *ptr, u
 	pkey_assert(i < NR_PKEYS*2);
 
 	/*
-	 * There are 16 pkeys supported in hardware.  One is taken
-	 * up for the default (0) and another can be taken up by
-	 * an execute-only mapping.  Ensure that we can allocate
-	 * at least 14 (16-2).
+	 * There are 16 pkeys supported in hardware.  Three are
+	 * allocated by the time we get here:
+	 *   1. The default key (0)
+	 *   2. One possibly consumed by an execute-only mapping.
+	 *   3. One allocated by the test code and passed in via
+	 *      'pkey' to this function.
+	 * Ensure that we can allocate at least another 13 (16-3).
 	 */
-	pkey_assert(i >= NR_PKEYS-2);
+	pkey_assert(i >= NR_PKEYS-3);
 
 	for (i = 0; i < nr_allocated_pkeys; i++) {
 		err = sys_pkey_free(allocated_pkeys[i]);
_

[PATCH 7/9] x86, pkeys, selftests: factor out "instruction page"

[Dave Hansen]

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

We currently have an execute-only test, but it is for
the explicit mprotect_pkey() interface.  We will soon
add a test for the implicit mprotect(PROT_EXEC)
enterface.  We need this code in both tests.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

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

diff -puN tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-get_pointer_to_instructions tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-get_pointer_to_instructions	2018-03-26 10:22:37.012170189 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-03-26 10:22:37.015170189 -0700
@@ -1277,12 +1277,9 @@ void test_ptrace_of_child(int *ptr, u16
 	free(plain_ptr_unaligned);
 }
 
-void test_executing_on_unreadable_memory(int *ptr, u16 pkey)
+void *get_pointer_to_instructions(void)
 {
 	void *p1;
-	int scratch;
-	int ptr_contents;
-	int ret;
 
 	p1 = ALIGN_PTR_UP(&lots_o_noops_around_write, PAGE_SIZE);
 	dprintf3("&lots_o_noops: %p\n", &lots_o_noops_around_write);
@@ -1292,7 +1289,23 @@ void test_executing_on_unreadable_memory
 	/* Point 'p1' at the *second* page of the function: */
 	p1 += PAGE_SIZE;
 
+	/*
+	 * Try to ensure we fault this in on next touch to ensure
+	 * we get an instruction fault as opposed to a data one
+	 */
 	madvise(p1, PAGE_SIZE, MADV_DONTNEED);
+
+	return p1;
+}
+
+void test_executing_on_unreadable_memory(int *ptr, u16 pkey)
+{
+	void *p1;
+	int scratch;
+	int ptr_contents;
+	int ret;
+
+	p1 = get_pointer_to_instructions();
 	lots_o_noops_around_write(&scratch);
 	ptr_contents = read_ptr(p1);
 	dprintf2("ptr (%p) contents@%d: %x\n", p1, __LINE__, ptr_contents);
_

[PATCH 8/9] x86, pkeys, selftests: add allow faults on unknown keys

[Dave Hansen]

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

The exec-only pkey is allocated inside the kernel and userspace
is not told what it is.  So, allow PK faults to occur that have
an unknown key.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

 b/tools/testing/selftests/x86/protection_keys.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff -puN tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-unknown-exec-only-key tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-unknown-exec-only-key	2018-03-26 10:22:37.549170187 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-03-26 10:22:37.553170187 -0700
@@ -922,13 +922,21 @@ void *malloc_pkey(long size, int prot, u
 }
 
 int last_pkru_faults;
+#define UNKNOWN_PKEY -2
 void expected_pk_fault(int pkey)
 {
 	dprintf2("%s(): last_pkru_faults: %d pkru_faults: %d\n",
 			__func__, last_pkru_faults, pkru_faults);
 	dprintf2("%s(%d): last_si_pkey: %d\n", __func__, pkey, last_si_pkey);
 	pkey_assert(last_pkru_faults + 1 == pkru_faults);
-	pkey_assert(last_si_pkey == pkey);
+
+       /*
+	* For exec-only memory, we do not know the pkey in
+	* advance, so skip this check.
+	*/
+	if (pkey != UNKNOWN_PKEY)
+		pkey_assert(last_si_pkey == pkey);
+
 	/*
 	 * The signal handler shold have cleared out PKRU to let the
 	 * test program continue.  We now have to restore it.
_

[PATCH 9/9] x86, pkeys, selftests: add PROT_EXEC test

[Dave Hansen]

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

Under the covers, implement executable-only memory with
protection keys when userspace calls mprotect(PROT_EXEC).

But, we did not have a selftest for that.  Now we do.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

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

diff -puN tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-prot_exec tools/testing/selftests/x86/protection_keys.c
--- a/tools/testing/selftests/x86/protection_keys.c~pkeys-selftests-prot_exec	2018-03-26 10:22:38.087170186 -0700
+++ b/tools/testing/selftests/x86/protection_keys.c	2018-03-26 10:22:38.091170186 -0700
@@ -930,10 +930,10 @@ void expected_pk_fault(int pkey)
 	dprintf2("%s(%d): last_si_pkey: %d\n", __func__, pkey, last_si_pkey);
 	pkey_assert(last_pkru_faults + 1 == pkru_faults);
 
-       /*
-	* For exec-only memory, we do not know the pkey in
-	* advance, so skip this check.
-	*/
+	/*
+	 * For exec-only memory, we do not know the pkey in
+	 * advance, so skip this check.
+	 */
 	if (pkey != UNKNOWN_PKEY)
 		pkey_assert(last_si_pkey == pkey);
 
@@ -1335,6 +1335,49 @@ void test_executing_on_unreadable_memory
 	expected_pk_fault(pkey);
 }
 
+void test_implicit_mprotect_exec_only_memory(int *ptr, u16 pkey)
+{
+	void *p1;
+	int scratch;
+	int ptr_contents;
+	int ret;
+
+	dprintf1("%s() start\n", __func__);
+
+	p1 = get_pointer_to_instructions();
+	lots_o_noops_around_write(&scratch);
+	ptr_contents = read_ptr(p1);
+	dprintf2("ptr (%p) contents@%d: %x\n", p1, __LINE__, ptr_contents);
+
+	/* Use a *normal* mprotect(), not mprotect_pkey(): */
+	ret = mprotect(p1, PAGE_SIZE, PROT_EXEC);
+	pkey_assert(!ret);
+
+	dprintf2("pkru: %x\n", rdpkru());
+
+	/* Make sure this is an *instruction* fault */
+	madvise(p1, PAGE_SIZE, MADV_DONTNEED);
+	lots_o_noops_around_write(&scratch);
+	do_not_expect_pk_fault();
+	ptr_contents = read_ptr(p1);
+	dprintf2("ptr (%p) contents@%d: %x\n", p1, __LINE__, ptr_contents);
+	expected_pk_fault(UNKNOWN_PKEY);
+
+	/*
+	 * Put the memory back to non-PROT_EXEC.  Should clear the
+	 * exec-only pkey off the VMA and allow it to be readable
+	 * again.  Go to PROT_NONE first to check for a kernel bug
+	 * that did not clear the pkey when doing PROT_NONE.
+	 */
+	ret = mprotect(p1, PAGE_SIZE, PROT_NONE);
+	pkey_assert(!ret);
+
+	ret = mprotect(p1, PAGE_SIZE, PROT_READ|PROT_EXEC);
+	pkey_assert(!ret);
+	ptr_contents = read_ptr(p1);
+	do_not_expect_pk_fault();
+}
+
 void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey)
 {
 	int size = PAGE_SIZE;
_

Re: [PATCH 1/9] x86, pkeys: do not special case protection key 0

[Shuah Khan]

On 03/26/2018 11:27 AM, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> mm_pkey_is_allocated() treats pkey 0 as unallocated.  That is
> inconsistent with the manpages, and also inconsistent with
> mm->context.pkey_allocation_map.  Stop special casing it and only
> disallow values that are actually bad (< 0).
> 
> The end-user visible effect of this is that you can now use
> mprotect_pkey() to set pkey=0.
> 
> This is a bit nicer than what Ram proposed because it is simpler
> and removes special-casing for pkey 0.  On the other hand, it does
> allow applciations to pkey_free() pkey-0, but that's just a silly

applications - typo.

> thing to do, so we are not going to protect against it.

If you plan to compare proposals, it would be nicer to include the
details of what Ram proposed as well in the commit log or link to the
discussion.

Also what happens "pkey_free() pkey-0" - can you elaborate more on that
"silliness consequences"

> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Fixes: 58ab9a088dda ("x86/pkeys: Check against max pkey to avoid overflows")
> Cc: stable@kernel.org
> Cc: Ram Pai <linuxram@us.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Michael Ellermen <mpe@ellerman.id.au>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>p
> Cc: Shuah Khan <shuah@kernel.org>
> ---
> 
>  b/arch/x86/include/asm/mmu_context.h |    2 +-
>  b/arch/x86/include/asm/pkeys.h       |    6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff -puN arch/x86/include/asm/mmu_context.h~x86-pkey-0-default-allocated arch/x86/include/asm/mmu_context.h
> --- a/arch/x86/include/asm/mmu_context.h~x86-pkey-0-default-allocated	2018-03-26 10:22:33.742170197 -0700
> +++ b/arch/x86/include/asm/mmu_context.h	2018-03-26 10:22:33.747170197 -0700
> @@ -192,7 +192,7 @@ static inline int init_new_context(struc
>  
>  #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
>  	if (cpu_feature_enabled(X86_FEATURE_OSPKE)) {
> -		/* pkey 0 is the default and always allocated */
> +		/* pkey 0 is the default and allocated implicitly */
>  		mm->context.pkey_allocation_map = 0x1;
>  		/* -1 means unallocated or invalid */
>  		mm->context.execute_only_pkey = -1;
> diff -puN arch/x86/include/asm/pkeys.h~x86-pkey-0-default-allocated arch/x86/include/asm/pkeys.h
> --- a/arch/x86/include/asm/pkeys.h~x86-pkey-0-default-allocated	2018-03-26 10:22:33.744170197 -0700
> +++ b/arch/x86/include/asm/pkeys.h	2018-03-26 10:22:33.747170197 -0700
> @@ -49,10 +49,10 @@ bool mm_pkey_is_allocated(struct mm_stru
>  {
>  	/*
>  	 * "Allocated" pkeys are those that have been returned
> -	 * from pkey_alloc().  pkey 0 is special, and never
> -	 * returned from pkey_alloc().
> +	 * from pkey_alloc() or pkey 0 which is allocated
> +	 * implicitly when the mm is created.
>  	 */
> -	if (pkey <= 0)
> +	if (pkey < 0)
>  		return false;
>  	if (pkey >= arch_max_pkey())
>  		return false;
> _
> 

thanks,
-- Shuah

Re: [PATCH 1/9] x86, pkeys: do not special case protection key 0

[Dave Hansen]

On 03/26/2018 10:47 AM, Shuah Khan wrote:
> 
> Also what happens "pkey_free() pkey-0" - can you elaborate more on that
> "silliness consequences"

It's just what happens if you free any other pkey that is in use: it
might get reallocated later.  The most likely scenario is that you will
get pkey-0 back from pkey_alloc(), you will set an access-disable or
write-disable bit in PKRU for it, and your next stack access will SIGSEGV.

Re: [PATCH 1/9] x86, pkeys: do not special case protection key 0

[Shuah Khan]

On 03/26/2018 11:53 AM, Dave Hansen wrote:
> On 03/26/2018 10:47 AM, Shuah Khan wrote:
>>
>> Also what happens "pkey_free() pkey-0" - can you elaborate more on that
>> "silliness consequences"
> 
> It's just what happens if you free any other pkey that is in use: it
> might get reallocated later.  The most likely scenario is that you will
> get pkey-0 back from pkey_alloc(), you will set an access-disable or
> write-disable bit in PKRU for it, and your next stack access will SIGSEGV.
> 

Thanks. This will good information to include in the commit log.

-- Shuah

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Ram Pai]

On Mon, Mar 26, 2018 at 10:27:27AM -0700, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> I got a bug report that the following code (roughly) was
> causing a SIGSEGV:
> 
> 	mprotect(ptr, size, PROT_EXEC);
> 	mprotect(ptr, size, PROT_NONE);
> 	mprotect(ptr, size, PROT_READ);
> 	*ptr = 100;
> 
> The problem is hit when the mprotect(PROT_EXEC)
> is implicitly assigned a protection key to the VMA, and made
> that key ACCESS_DENY|WRITE_DENY.  The PROT_NONE mprotect()
> failed to remove the protection key, and the PROT_NONE->
> PROT_READ left the PTE usable, but the pkey still in place
> and left the memory inaccessible.
> 
> To fix this, we ensure that we always "override" the pkee
> at mprotect() if the VMA does not have execute-only
> permissions, but the VMA has the execute-only pkey.
> 
> We had a check for PROT_READ/WRITE, but it did not work
> for PROT_NONE.  This entirely removes the PROT_* checks,
> which ensures that PROT_NONE now works.
> 
> Reported-by: Shakeel Butt <shakeelb@google.com>
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
> Cc: stable@kernel.org
> Cc: Ram Pai <linuxram@us.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Michael Ellermen <mpe@ellerman.id.au>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
> 
>  b/arch/x86/include/asm/pkeys.h |   12 +++++++++++-
>  b/arch/x86/mm/pkeys.c          |   19 ++++++++++---------
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff -puN arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/include/asm/pkeys.h
> --- a/arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively	2018-03-26 10:22:35.380170193 -0700
> +++ b/arch/x86/include/asm/pkeys.h	2018-03-26 10:22:35.385170193 -0700
> @@ -2,6 +2,8 @@
>  #ifndef _ASM_X86_PKEYS_H
>  #define _ASM_X86_PKEYS_H
> 
> +#define ARCH_DEFAULT_PKEY	0
> +
>  #define arch_max_pkey() (boot_cpu_has(X86_FEATURE_OSPKE) ? 16 : 1)
> 
>  extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> @@ -15,7 +17,7 @@ extern int __execute_only_pkey(struct mm
>  static inline int execute_only_pkey(struct mm_struct *mm)
>  {
>  	if (!boot_cpu_has(X86_FEATURE_OSPKE))
> -		return 0;
> +		return ARCH_DEFAULT_PKEY;
> 
>  	return __execute_only_pkey(mm);
>  }
> @@ -56,6 +58,14 @@ bool mm_pkey_is_allocated(struct mm_stru
>  		return false;
>  	if (pkey >= arch_max_pkey())
>  		return false;
> +	/*
> +	 * The exec-only pkey is set in the allocation map, but
> +	 * is not available to any of the user interfaces like
> +	 * mprotect_pkey().
> +	 */
> +	if (pkey == mm->context.execute_only_pkey)
> +		return false;
> +
>  	return mm_pkey_allocation_map(mm) & (1U << pkey);
>  }
> 
> diff -puN arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/mm/pkeys.c
> --- a/arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively	2018-03-26 10:22:35.381170193 -0700
> +++ b/arch/x86/mm/pkeys.c	2018-03-26 10:22:35.385170193 -0700
> @@ -94,15 +94,7 @@ int __arch_override_mprotect_pkey(struct
>  	 */
>  	if (pkey != -1)
>  		return pkey;
> -	/*
> -	 * Look for a protection-key-drive execute-only mapping
> -	 * which is now being given permissions that are not
> -	 * execute-only.  Move it back to the default pkey.
> -	 */
> -	if (vma_is_pkey_exec_only(vma) &&
> -	    (prot & (PROT_READ|PROT_WRITE))) {
> -		return 0;
> -	}
> +

Dave,
	this can be simply:

	if ((vma_is_pkey_exec_only(vma) && (prot != PROT_EXEC))
		return ARCH_DEFAULT_PKEY;

No?
RP

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

On 04/06/2018 05:09 PM, Ram Pai wrote:
>> -	/*
>> -	 * Look for a protection-key-drive execute-only mapping
>> -	 * which is now being given permissions that are not
>> -	 * execute-only.  Move it back to the default pkey.
>> -	 */
>> -	if (vma_is_pkey_exec_only(vma) &&
>> -	    (prot & (PROT_READ|PROT_WRITE))) {
>> -		return 0;
>> -	}
>> +
> Dave,
> 	this can be simply:
> 
> 	if ((vma_is_pkey_exec_only(vma) && (prot != PROT_EXEC))
> 		return ARCH_DEFAULT_PKEY;

Yes, but we're removing that code entirely. :)

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Ram Pai]

On Fri, Apr 06, 2018 at 05:47:29PM -0700, Dave Hansen wrote:
> On 04/06/2018 05:09 PM, Ram Pai wrote:
> >> -	/*
> >> -	 * Look for a protection-key-drive execute-only mapping
> >> -	 * which is now being given permissions that are not
> >> -	 * execute-only.  Move it back to the default pkey.
> >> -	 */
> >> -	if (vma_is_pkey_exec_only(vma) &&
> >> -	    (prot & (PROT_READ|PROT_WRITE))) {
> >> -		return 0;
> >> -	}
> >> +
> > Dave,
> > 	this can be simply:
> > 
> > 	if ((vma_is_pkey_exec_only(vma) && (prot != PROT_EXEC))
> > 		return ARCH_DEFAULT_PKEY;
> 
> Yes, but we're removing that code entirely. :)

Well :). my point is add this code and delete the other
code that you add later in that function.

RP



-- 
Ram Pai

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Shakeel Butt]

On Mon, Mar 26, 2018 at 5:27 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> I got a bug report that the following code (roughly) was
> causing a SIGSEGV:
>
>         mprotect(ptr, size, PROT_EXEC);
>         mprotect(ptr, size, PROT_NONE);
>         mprotect(ptr, size, PROT_READ);
>         *ptr = 100;
>
> The problem is hit when the mprotect(PROT_EXEC)
> is implicitly assigned a protection key to the VMA, and made
> that key ACCESS_DENY|WRITE_DENY.  The PROT_NONE mprotect()
> failed to remove the protection key, and the PROT_NONE->
> PROT_READ left the PTE usable, but the pkey still in place
> and left the memory inaccessible.
>
> To fix this, we ensure that we always "override" the pkee
> at mprotect() if the VMA does not have execute-only
> permissions, but the VMA has the execute-only pkey.
>
> We had a check for PROT_READ/WRITE, but it did not work
> for PROT_NONE.  This entirely removes the PROT_* checks,
> which ensures that PROT_NONE now works.
>
> Reported-by: Shakeel Butt <shakeelb@google.com>
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")

Hi Dave, are you planning to send the next version of this patch or
going with this one?

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Thomas Gleixner]

On Wed, 25 Apr 2018, Shakeel Butt wrote:
> On Mon, Mar 26, 2018 at 5:27 PM, Dave Hansen
> <dave.hansen@linux.intel.com> wrote:
> >
> > From: Dave Hansen <dave.hansen@linux.intel.com>
> >
> > I got a bug report that the following code (roughly) was
> > causing a SIGSEGV:
> >
> >         mprotect(ptr, size, PROT_EXEC);
> >         mprotect(ptr, size, PROT_NONE);
> >         mprotect(ptr, size, PROT_READ);
> >         *ptr = 100;
> >
> > The problem is hit when the mprotect(PROT_EXEC)
> > is implicitly assigned a protection key to the VMA, and made
> > that key ACCESS_DENY|WRITE_DENY.  The PROT_NONE mprotect()
> > failed to remove the protection key, and the PROT_NONE->
> > PROT_READ left the PTE usable, but the pkey still in place
> > and left the memory inaccessible.
> >
> > To fix this, we ensure that we always "override" the pkee
> > at mprotect() if the VMA does not have execute-only
> > permissions, but the VMA has the execute-only pkey.
> >
> > We had a check for PROT_READ/WRITE, but it did not work
> > for PROT_NONE.  This entirely removes the PROT_* checks,
> > which ensures that PROT_NONE now works.
> >
> > Reported-by: Shakeel Butt <shakeelb@google.com>
> >
> > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> > Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
> 
> Hi Dave, are you planning to send the next version of this patch or
> going with this one?

Right, some enlightment would be appreciated. I'm lost in the dozen
different threads discussing this back and forth.

Thanks,

	tglx

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

On 04/06/2018 06:09 PM, Ram Pai wrote:
> Well :). my point is add this code and delete the other
> code that you add later in that function.

I don't think I'm understanding what your suggestion was.  I looked at
the code and I honestly do not think I can remove any of it.

For the plain (non-explicit pkey_mprotect()) case, there are exactly
four paths through __arch_override_mprotect_pkey(), resulting in three
different results.

1. New prot==PROT_EXEC, no pkey-exec support -> do not override
2. New prot!=PROT_EXEC, old VMA not PROT_EXEC-> do not override
3. New prot==PROT_EXEC, w/ pkey-exec support -> override to exec pkey
4. New prot!=PROT_EXEC, old VMA is PROT_EXEC -> override to default

I don't see any redundancy there, or any code that we can eliminate or
simplify.  It was simpler before, but that's what where bug was.

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

On 04/26/2018 01:55 AM, Thomas Gleixner wrote:
>> Hi Dave, are you planning to send the next version of this patch or
>> going with this one?
> Right, some enlightment would be appreciated. I'm lost in the dozen
> different threads discussing this back and forth.

Shakeel, thanks for the reminder!

I'll send an updated set.  I got lost myself and thought this had been
picked up.

There were a few minor comments on the [v2] set that I've addressed.
I'll also check with Ram to make sure he's OK with this on ppc.  We had
some dueling patches at some point.

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Ram Pai]

On Thu, Apr 26, 2018 at 10:57:31AM -0700, Dave Hansen wrote:
> On 04/06/2018 06:09 PM, Ram Pai wrote:
> > Well :). my point is add this code and delete the other
> > code that you add later in that function.
> 
> I don't think I'm understanding what your suggestion was.  I looked at
> the code and I honestly do not think I can remove any of it.
> 
> For the plain (non-explicit pkey_mprotect()) case, there are exactly
> four paths through __arch_override_mprotect_pkey(), resulting in three
> different results.
> 
> 1. New prot==PROT_EXEC, no pkey-exec support -> do not override
> 2. New prot!=PROT_EXEC, old VMA not PROT_EXEC-> do not override
> 3. New prot==PROT_EXEC, w/ pkey-exec support -> override to exec pkey
> 4. New prot!=PROT_EXEC, old VMA is PROT_EXEC -> override to default
> 
> I don't see any redundancy there, or any code that we can eliminate or
> simplify.  It was simpler before, but that's what where bug was.

Your code is fine.  But than the following code accomplishes the same
outcome; arguably with a one line change. Its not a big deal. Just
trying to clarify my comment.

int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot, int pkey)
{
	/*
	 * Is this an mprotect_pkey() call?  If so, never
	 * override the value that came from the user.
	 */
	if (pkey != -1)
		return pkey;
	/*
	 * Look for a protection-key-drive execute-only mapping
	 * which is now being given permissions that are not
	 * execute-only.  Move it back to the default pkey.
	 */
	if (vma_is_pkey_exec_only(vma) && (prot != PROT_EXEC)) <--------
		return ARCH_DEFAULT_PKEY;

	/*
	 * The mapping is execute-only.  Go try to get the
	 * execute-only protection key.  If we fail to do that,
	 * fall through as if we do not have execute-only
	 * support.
	 */
	if (prot == PROT_EXEC) {
		pkey = execute_only_pkey(vma->vm_mm);
		if (pkey > 0)
			return pkey;
	}
	/*
	 * This is a vanilla, non-pkey mprotect (or we failed to
	 * setup execute-only), inherit the pkey from the VMA we
	 * are working on.
	 */
	return vma_pkey(vma);
}

-- 
Ram Pai

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

On 04/30/2018 12:51 AM, Ram Pai wrote:
> 	/*
> 	 * Look for a protection-key-drive execute-only mapping
> 	 * which is now being given permissions that are not
> 	 * execute-only.  Move it back to the default pkey.
> 	 */
> 	if (vma_is_pkey_exec_only(vma) && (prot != PROT_EXEC)) <--------
> 		return ARCH_DEFAULT_PKEY;
> 
> 	/*
> 	 * The mapping is execute-only.  Go try to get the
> 	 * execute-only protection key.  If we fail to do that,
> 	 * fall through as if we do not have execute-only
> 	 * support.
> 	 */
> 	if (prot == PROT_EXEC) {
> 		pkey = execute_only_pkey(vma->vm_mm);
> 		if (pkey > 0)
> 			return pkey;
> 	}

Yes, that would also work.  It's just a matter of whether you prefer
having the prot==PROT_EXEC checks in one place or two.  I'd rather leave
it the way I've got it unless there are major objections since it's been
tested.

[PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

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

I got a bug report that the following code (roughly) was
causing a SIGSEGV:

	mprotect(ptr, size, PROT_EXEC);
	mprotect(ptr, size, PROT_NONE);
	mprotect(ptr, size, PROT_READ);
	*ptr = 100;

The problem is hit when the mprotect(PROT_EXEC)
is implicitly assigned a protection key to the VMA, and made
that key ACCESS_DENY|WRITE_DENY.  The PROT_NONE mprotect()
failed to remove the protection key, and the PROT_NONE->
PROT_READ left the PTE usable, but the pkey still in place
and left the memory inaccessible.

To fix this, we ensure that we always "override" the pkee
at mprotect() if the VMA does not have execute-only
permissions, but the VMA has the execute-only pkey.

We had a check for PROT_READ/WRITE, but it did not work
for PROT_NONE.  This entirely removes the PROT_* checks,
which ensures that PROT_NONE now works.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
Reported-by: Shakeel Butt <shakeelb@google.com>
Cc: stable@vger.kernel.org
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

 b/arch/x86/include/asm/pkeys.h |   12 +++++++++++-
 b/arch/x86/mm/pkeys.c          |   21 +++++++++++----------
 2 files changed, 22 insertions(+), 11 deletions(-)

diff -puN arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively	2018-04-26 10:42:18.971487371 -0700
+++ b/arch/x86/include/asm/pkeys.h	2018-04-26 10:42:18.977487371 -0700
@@ -2,6 +2,8 @@
 #ifndef _ASM_X86_PKEYS_H
 #define _ASM_X86_PKEYS_H
 
+#define ARCH_DEFAULT_PKEY	0
+
 #define arch_max_pkey() (boot_cpu_has(X86_FEATURE_OSPKE) ? 16 : 1)
 
 extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
@@ -15,7 +17,7 @@ extern int __execute_only_pkey(struct mm
 static inline int execute_only_pkey(struct mm_struct *mm)
 {
 	if (!boot_cpu_has(X86_FEATURE_OSPKE))
-		return 0;
+		return ARCH_DEFAULT_PKEY;
 
 	return __execute_only_pkey(mm);
 }
@@ -56,6 +58,14 @@ bool mm_pkey_is_allocated(struct mm_stru
 		return false;
 	if (pkey >= arch_max_pkey())
 		return false;
+	/*
+	 * The exec-only pkey is set in the allocation map, but
+	 * is not available to any of the user interfaces like
+	 * mprotect_pkey().
+	 */
+	if (pkey == mm->context.execute_only_pkey)
+		return false;
+
 	return mm_pkey_allocation_map(mm) & (1U << pkey);
 }
 
diff -puN arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/mm/pkeys.c
--- a/arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively	2018-04-26 10:42:18.973487371 -0700
+++ b/arch/x86/mm/pkeys.c	2018-04-26 10:47:34.806486584 -0700
@@ -94,26 +94,27 @@ int __arch_override_mprotect_pkey(struct
 	 */
 	if (pkey != -1)
 		return pkey;
-	/*
-	 * Look for a protection-key-drive execute-only mapping
-	 * which is now being given permissions that are not
-	 * execute-only.  Move it back to the default pkey.
-	 */
-	if (vma_is_pkey_exec_only(vma) &&
-	    (prot & (PROT_READ|PROT_WRITE))) {
-		return 0;
-	}
+
 	/*
 	 * The mapping is execute-only.  Go try to get the
 	 * execute-only protection key.  If we fail to do that,
 	 * fall through as if we do not have execute-only
-	 * support.
+	 * support in this mm.
 	 */
 	if (prot == PROT_EXEC) {
 		pkey = execute_only_pkey(vma->vm_mm);
 		if (pkey > 0)
 			return pkey;
+	} else if (vma_is_pkey_exec_only(vma)) {
+		/*
+		 * Protections are *not* PROT_EXEC, but the mapping
+		 * is using the exec-only pkey.  This mapping was
+		 * PROT_EXEC and will no longer be.  Move back to
+		 * the default pkey.
+		 */
+		return ARCH_DEFAULT_PKEY;
 	}
+
 	/*
 	 * This is a vanilla, non-pkey mprotect (or we failed to
 	 * setup execute-only), inherit the pkey from the VMA we
_

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

On 03/23/2018 12:45 PM, Thomas Gleixner wrote:
>> The fixes tag makes sense in general even if the patch is not tagged for
>> stable. It gives you immediate context and I use it a lot to look why this
>> went unnoticed or what the context of that change was.
> That said, I'm even lazier than you and prefer you to dig up the original
> commit :)

I'll have these tags in the next repost.

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Thomas Gleixner]

On Fri, 23 Mar 2018, Thomas Gleixner wrote:

> On Fri, 23 Mar 2018, Dave Hansen wrote:
> 
> > On 03/23/2018 12:15 PM, Shakeel Butt wrote:
> > >> We had a check for PROT_READ/WRITE, but it did not work
> > >> for PROT_NONE.  This entirely removes the PROT_* checks,
> > >> which ensures that PROT_NONE now works.
> > >>
> > >> Reported-by: Shakeel Butt <shakeelb@google.com>
> > >> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> > > Should there be a 'Fixes' tag? Also should this patch go to stable?
> > 
> > There could be, but I'm to lazy to dig up the original commit.  Does it
> > matter?
> > 
> > And, yes, I think it probably makes sense for -stable.  I'll add that if
> > I resend this series.
> 
> The fixes tag makes sense in general even if the patch is not tagged for
> stable. It gives you immediate context and I use it a lot to look why this
> went unnoticed or what the context of that change was.

That said, I'm even lazier than you and prefer you to dig up the original
commit :)

Thanks,

	tglx

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Thomas Gleixner]

On Fri, 23 Mar 2018, Dave Hansen wrote:

> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
> >> We had a check for PROT_READ/WRITE, but it did not work
> >> for PROT_NONE.  This entirely removes the PROT_* checks,
> >> which ensures that PROT_NONE now works.
> >>
> >> Reported-by: Shakeel Butt <shakeelb@google.com>
> >> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> > Should there be a 'Fixes' tag? Also should this patch go to stable?
> 
> There could be, but I'm to lazy to dig up the original commit.  Does it
> matter?
> 
> And, yes, I think it probably makes sense for -stable.  I'll add that if
> I resend this series.

The fixes tag makes sense in general even if the patch is not tagged for
stable. It gives you immediate context and I use it a lot to look why this
went unnoticed or what the context of that change was.

Thanks,

	tglx

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

On 03/23/2018 12:27 PM, Shakeel Butt wrote:
> On Fri, Mar 23, 2018 at 12:23 PM, Dave Hansen <dave.hansen@intel.com> wrote:
>> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
>>>> We had a check for PROT_READ/WRITE, but it did not work
>>>> for PROT_NONE.  This entirely removes the PROT_* checks,
>>>> which ensures that PROT_NONE now works.
>>>>
>>>> Reported-by: Shakeel Butt <shakeelb@google.com>
>>>> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
>>> Should there be a 'Fixes' tag? Also should this patch go to stable?
>> There could be, but I'm to lazy to dig up the original commit.  Does it
>> matter?
>>
> I think for stable 'Fixes' is usually preferable.

This one is a no-brainer.  If pkeys.c is there, it's necesary.

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Shakeel Butt]

On Fri, Mar 23, 2018 at 12:23 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
>>> We had a check for PROT_READ/WRITE, but it did not work
>>> for PROT_NONE.  This entirely removes the PROT_* checks,
>>> which ensures that PROT_NONE now works.
>>>
>>> Reported-by: Shakeel Butt <shakeelb@google.com>
>>> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
>> Should there be a 'Fixes' tag? Also should this patch go to stable?
>
> There could be, but I'm to lazy to dig up the original commit.  Does it
> matter?
>

I think for stable 'Fixes' is usually preferable.

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

On 03/23/2018 12:15 PM, Shakeel Butt wrote:
>> We had a check for PROT_READ/WRITE, but it did not work
>> for PROT_NONE.  This entirely removes the PROT_* checks,
>> which ensures that PROT_NONE now works.
>>
>> Reported-by: Shakeel Butt <shakeelb@google.com>
>> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Should there be a 'Fixes' tag? Also should this patch go to stable?

There could be, but I'm to lazy to dig up the original commit.  Does it
matter?

And, yes, I think it probably makes sense for -stable.  I'll add that if
I resend this series.

Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Shakeel Butt]

On Fri, Mar 23, 2018 at 11:09 AM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> I got a bug report that the following code (roughly) was
> causing a SIGSEGV:
>
>         mprotect(ptr, size, PROT_EXEC);
>         mprotect(ptr, size, PROT_NONE);
>         mprotect(ptr, size, PROT_READ);
>         *ptr = 100;
>
> The problem is hit when the mprotect(PROT_EXEC)
> is implicitly assigned a protection key to the VMA, and made
> that key ACCESS_DENY|WRITE_DENY.  The PROT_NONE mprotect()
> failed to remove the protection key, and the PROT_NONE->
> PROT_READ left the PTE usable, but the pkey still in place
> and left the memory inaccessible.
>
> To fix this, we ensure that we always "override" the pkee
> at mprotect() if the VMA does not have execute-only
> permissions, but the VMA has the execute-only pkey.
>
> We had a check for PROT_READ/WRITE, but it did not work
> for PROT_NONE.  This entirely removes the PROT_* checks,
> which ensures that PROT_NONE now works.
>
> Reported-by: Shakeel Butt <shakeelb@google.com>
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Should there be a 'Fixes' tag? Also should this patch go to stable?

> Cc: Ram Pai <linuxram@us.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Michael Ellermen <mpe@ellerman.id.au>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
>
>  b/arch/x86/include/asm/pkeys.h |   12 +++++++++++-
>  b/arch/x86/mm/pkeys.c          |   19 ++++++++++---------
>  2 files changed, 21 insertions(+), 10 deletions(-)
>
> diff -puN arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/include/asm/pkeys.h
> --- a/arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively       2018-03-21 15:47:49.810198922 -0700
> +++ b/arch/x86/include/asm/pkeys.h      2018-03-21 15:47:49.816198922 -0700
> @@ -2,6 +2,8 @@
>  #ifndef _ASM_X86_PKEYS_H
>  #define _ASM_X86_PKEYS_H
>
> +#define ARCH_DEFAULT_PKEY      0
> +
>  #define arch_max_pkey() (boot_cpu_has(X86_FEATURE_OSPKE) ? 16 : 1)
>
>  extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> @@ -15,7 +17,7 @@ extern int __execute_only_pkey(struct mm
>  static inline int execute_only_pkey(struct mm_struct *mm)
>  {
>         if (!boot_cpu_has(X86_FEATURE_OSPKE))
> -               return 0;
> +               return ARCH_DEFAULT_PKEY;
>
>         return __execute_only_pkey(mm);
>  }
> @@ -56,6 +58,14 @@ bool mm_pkey_is_allocated(struct mm_stru
>                 return false;
>         if (pkey >= arch_max_pkey())
>                 return false;
> +       /*
> +        * The exec-only pkey is set in the allocation map, but
> +        * is not available to any of the user interfaces like
> +        * mprotect_pkey().
> +        */
> +       if (pkey == mm->context.execute_only_pkey)
> +               return false;
> +
>         return mm_pkey_allocation_map(mm) & (1U << pkey);
>  }
>
> diff -puN arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/mm/pkeys.c
> --- a/arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively        2018-03-21 15:47:49.812198922 -0700
> +++ b/arch/x86/mm/pkeys.c       2018-03-21 15:47:49.816198922 -0700
> @@ -94,15 +94,7 @@ int __arch_override_mprotect_pkey(struct
>          */
>         if (pkey != -1)
>                 return pkey;
> -       /*
> -        * Look for a protection-key-drive execute-only mapping
> -        * which is now being given permissions that are not
> -        * execute-only.  Move it back to the default pkey.
> -        */
> -       if (vma_is_pkey_exec_only(vma) &&
> -           (prot & (PROT_READ|PROT_WRITE))) {
> -               return 0;
> -       }
> +
>         /*
>          * The mapping is execute-only.  Go try to get the
>          * execute-only protection key.  If we fail to do that,
> @@ -113,7 +105,16 @@ int __arch_override_mprotect_pkey(struct
>                 pkey = execute_only_pkey(vma->vm_mm);
>                 if (pkey > 0)
>                         return pkey;
> +       } else if (vma_is_pkey_exec_only(vma)) {
> +               /*
> +                * Protections are *not* PROT_EXEC, but the mapping
> +                * is using the exec-only pkey.  This mapping was
> +                * PROT_EXEC and will no longer be.  Move back to
> +                * the default pkey.
> +                */
> +               return ARCH_DEFAULT_PKEY;
>         }
> +
>         /*
>          * This is a vanilla, non-pkey mprotect (or we failed to
>          * setup execute-only), inherit the pkey from the VMA we
> _

[PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Dave Hansen]

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

I got a bug report that the following code (roughly) was
causing a SIGSEGV:

	mprotect(ptr, size, PROT_EXEC);
	mprotect(ptr, size, PROT_NONE);
	mprotect(ptr, size, PROT_READ);
	*ptr = 100;

The problem is hit when the mprotect(PROT_EXEC)
is implicitly assigned a protection key to the VMA, and made
that key ACCESS_DENY|WRITE_DENY.  The PROT_NONE mprotect()
failed to remove the protection key, and the PROT_NONE->
PROT_READ left the PTE usable, but the pkey still in place
and left the memory inaccessible.

To fix this, we ensure that we always "override" the pkee
at mprotect() if the VMA does not have execute-only
permissions, but the VMA has the execute-only pkey.

We had a check for PROT_READ/WRITE, but it did not work
for PROT_NONE.  This entirely removes the PROT_* checks,
which ensures that PROT_NONE now works.

Reported-by: Shakeel Butt <shakeelb@google.com>

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ram Pai <linuxram@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michael Ellermen <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Shuah Khan <shuah@kernel.org>
---

 b/arch/x86/include/asm/pkeys.h |   12 +++++++++++-
 b/arch/x86/mm/pkeys.c          |   19 ++++++++++---------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff -puN arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/include/asm/pkeys.h
--- a/arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively	2018-03-21 15:47:49.810198922 -0700
+++ b/arch/x86/include/asm/pkeys.h	2018-03-21 15:47:49.816198922 -0700
@@ -2,6 +2,8 @@
 #ifndef _ASM_X86_PKEYS_H
 #define _ASM_X86_PKEYS_H
 
+#define ARCH_DEFAULT_PKEY	0
+
 #define arch_max_pkey() (boot_cpu_has(X86_FEATURE_OSPKE) ? 16 : 1)
 
 extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
@@ -15,7 +17,7 @@ extern int __execute_only_pkey(struct mm
 static inline int execute_only_pkey(struct mm_struct *mm)
 {
 	if (!boot_cpu_has(X86_FEATURE_OSPKE))
-		return 0;
+		return ARCH_DEFAULT_PKEY;
 
 	return __execute_only_pkey(mm);
 }
@@ -56,6 +58,14 @@ bool mm_pkey_is_allocated(struct mm_stru
 		return false;
 	if (pkey >= arch_max_pkey())
 		return false;
+	/*
+	 * The exec-only pkey is set in the allocation map, but
+	 * is not available to any of the user interfaces like
+	 * mprotect_pkey().
+	 */
+	if (pkey == mm->context.execute_only_pkey)
+		return false;
+
 	return mm_pkey_allocation_map(mm) & (1U << pkey);
 }
 
diff -puN arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively arch/x86/mm/pkeys.c
--- a/arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively	2018-03-21 15:47:49.812198922 -0700
+++ b/arch/x86/mm/pkeys.c	2018-03-21 15:47:49.816198922 -0700
@@ -94,15 +94,7 @@ int __arch_override_mprotect_pkey(struct
 	 */
 	if (pkey != -1)
 		return pkey;
-	/*
-	 * Look for a protection-key-drive execute-only mapping
-	 * which is now being given permissions that are not
-	 * execute-only.  Move it back to the default pkey.
-	 */
-	if (vma_is_pkey_exec_only(vma) &&
-	    (prot & (PROT_READ|PROT_WRITE))) {
-		return 0;
-	}
+
 	/*
 	 * The mapping is execute-only.  Go try to get the
 	 * execute-only protection key.  If we fail to do that,
@@ -113,7 +105,16 @@ int __arch_override_mprotect_pkey(struct
 		pkey = execute_only_pkey(vma->vm_mm);
 		if (pkey > 0)
 			return pkey;
+	} else if (vma_is_pkey_exec_only(vma)) {
+		/*
+		 * Protections are *not* PROT_EXEC, but the mapping
+		 * is using the exec-only pkey.  This mapping was
+		 * PROT_EXEC and will no longer be.  Move back to
+		 * the default pkey.
+		 */
+		return ARCH_DEFAULT_PKEY;
 	}
+
 	/*
 	 * This is a vanilla, non-pkey mprotect (or we failed to
 	 * setup execute-only), inherit the pkey from the VMA we
_