[PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Ram Pai]

Assortment of fixes to pkey.

Patch 1  makes pkey consumable in multithreaded applications.

Patch 2  fixes fork behavior to inherit the key attributes.

Patch 3  A off-by-one bug made one key unusable. Fixes it.

Patch 4  Execute-only key is preallocated.

Patch 5  Makes pkey-0 less special.

Patch 6  Deny by default permissions on all unallocated keys.

Passes all selftests on powerpc. Also behavior verified to be correct
by Florian.

Changelog:

	v2: . fixed merge conflict with upstream code.
	    . Add patch 6. Makes the behavior consistent
	      with that on x86.

Ram Pai (6):
  powerpc/pkeys: Enable all user-allocatable pkeys at init.
  powerpc/pkeys: Save the pkey registers before fork
  powerpc/pkeys: fix calculation of total pkeys.
  powerpc/pkeys: Preallocate execute-only key
  powerpc/pkeys: make protection key 0 less special
  powerpc/pkeys: Deny read/write/execute by default

 arch/powerpc/include/asm/pkeys.h |   29 +++++++++--
 arch/powerpc/kernel/process.c    |    1 +
 arch/powerpc/mm/pkeys.c          |  102 ++++++++++++-------------------------
 3 files changed, 57 insertions(+), 75 deletions(-)

[PATCH v2 1/6] powerpc/pkeys: Enable all user-allocatable pkeys at init.

[Ram Pai]

In a multithreaded application, a key allocated by one thread must
be activate and usable on all threads.

Currently this is not the case, because the UAMOR bits for all keys are
disabled by default. When a new key is allocated in one thread, though
the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
for all other existing threads continue to have their bits disabled.
Other threads have no way to set permissions on the key, effectively
making the key useless.

Enable the UAMOR bits for all keys, at process creation. Since the
contents of UAMOR are inherited at fork, all threads are capable of
modifying the permissions on any key.

BTW: changing the permission on unallocated keys has no effect, till
those keys are not associated with any PTEs. The kernel will anyway
disallow to association of unallocated keys with PTEs.

CC: Andy Lutomirski <luto@kernel.org>
CC: Florian Weimer <fweimer@redhat.com>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/mm/pkeys.c |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index e6f500f..6529f4e 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -15,8 +15,9 @@
 int  pkeys_total;		/* Total pkeys as per device tree */
 bool pkeys_devtree_defined;	/* pkey property exported by device tree */
 u32  initial_allocation_mask;	/* Bits set for reserved keys */
-u64  pkey_amr_uamor_mask;	/* Bits in AMR/UMOR not to be touched */
+u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
 u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
+u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
 
 #define AMR_BITS_PER_PKEY 2
 #define AMR_RD_BIT 0x1UL
@@ -119,20 +120,22 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	initial_allocation_mask = ~0x0;
-	pkey_amr_uamor_mask = ~0x0ul;
+	initial_allocation_mask  = (0x1 << 0) | (0x1 << 1);
+
+	/* register mask is in BE format */
+	pkey_amr_mask = ~0x0ul;
 	pkey_iamr_mask = ~0x0ul;
-	/*
-	 * key 0, 1 are reserved.
-	 * key 0 is the default key, which allows read/write/execute.
-	 * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
-	 * programming note.
-	 */
-	for (i = 2; i < (pkeys_total - os_reserved); i++) {
-		initial_allocation_mask &= ~(0x1 << i);
-		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
+
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+		pkey_amr_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
 	}
+
+	pkey_uamor_mask = ~0x0ul;
+	pkey_uamor_mask &= ~(0x3ul << pkeyshift(0));
+	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++)
+		pkey_uamor_mask &= ~(0x3ul << pkeyshift(i));
+
 	return 0;
 }
 
@@ -289,9 +292,6 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
 	if (static_branch_likely(&pkey_disabled))
 		return;
 
-	/*
-	 * TODO: Just set UAMOR to zero if @new_thread hasn't used any keys yet.
-	 */
 	if (old_thread->amr != new_thread->amr)
 		write_amr(new_thread->amr);
 	if (old_thread->iamr != new_thread->iamr)
@@ -305,9 +305,13 @@ void thread_pkey_regs_init(struct thread_struct *thread)
 	if (static_branch_likely(&pkey_disabled))
 		return;
 
-	thread->amr = read_amr() & pkey_amr_uamor_mask;
-	thread->iamr = read_iamr() & pkey_iamr_mask;
-	thread->uamor = read_uamor() & pkey_amr_uamor_mask;
+	thread->amr = pkey_amr_mask;
+	thread->iamr = pkey_iamr_mask;
+ 	thread->uamor = pkey_uamor_mask;
+
+	write_uamor(pkey_uamor_mask);
+	write_amr(pkey_amr_mask);
+	write_iamr(pkey_iamr_mask);
 }
 
 static inline bool pkey_allows_readwrite(int pkey)
-- 
1.7.1

[PATCH v2 2/6] powerpc/pkeys: Save the pkey registers before fork

[Ram Pai]

When a thread forks the contents of AMR, IAMR, UAMOR registers in the
newly forked thread are not inherited.

Save the registers before forking, for content of those
registers to be automatically copied into the new thread.

CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Florian Weimer <fweimer@redhat.com>
CC: Andy Lutomirski <luto@kernel.org>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/kernel/process.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 9ef4aea..991d097 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -583,6 +583,7 @@ static void save_all(struct task_struct *tsk)
 		__giveup_spe(tsk);
 
 	msr_check_and_clear(msr_all_available);
+	thread_pkey_regs_save(&tsk->thread);
 }
 
 void flush_all_to_thread(struct task_struct *tsk)
-- 
1.7.1

[PATCH v2 3/6] powerpc/pkeys: fix calculation of total pkeys.

[Ram Pai]

Total number of pkeys calculation is off by 1. Fix it.

CC: Florian Weimer <fweimer@redhat.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/mm/pkeys.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 6529f4e..b681bec 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -92,7 +92,7 @@ int pkey_initialize(void)
 	 * arch-neutral code.
 	 */
 	pkeys_total = min_t(int, pkeys_total,
-			(ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT));
+			((ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT)+1));
 
 	if (!pkey_mmu_enabled() || radix_enabled() || !pkeys_total)
 		static_branch_enable(&pkey_disabled);
-- 
1.7.1

[PATCH v2 4/6] powerpc/pkeys: Preallocate execute-only key

[Ram Pai]

execute-only key is allocated dynamically. This is a problem.  When a
thread implicitly creates a execute-only key, and resets UAMOR for that
key, the UAMOR value does not percolate to all the other threads.  Any
other thread may ignorantly change the permissions on the key.  This can
cause the key to be not execute-only for that thread.

Preallocate the execute-only key and ensure that no thread can change
the permission of the key, by resetting the corresponding bit in UAMOR.

CC: Andy Lutomirski <luto@kernel.org>
CC: Florian Weimer <fweimer@redhat.com>
CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/mm/pkeys.c |   53 +++++++---------------------------------------
 1 files changed, 8 insertions(+), 45 deletions(-)

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index b681bec..1f2389f 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -25,6 +25,7 @@
 #define IAMR_EX_BIT 0x1UL
 #define PKEY_REG_BITS (sizeof(u64)*8)
 #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
+#define EXECUTE_ONLY_KEY 2
 
 static void scan_pkey_feature(void)
 {
@@ -120,7 +121,8 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	initial_allocation_mask  = (0x1 << 0) | (0x1 << 1);
+	initial_allocation_mask  = (0x1 << 0) | (0x1 << 1) |
+					(0x1 << EXECUTE_ONLY_KEY);
 
 	/* register mask is in BE format */
 	pkey_amr_mask = ~0x0ul;
@@ -130,9 +132,12 @@ int pkey_initialize(void)
 		pkey_amr_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
 	}
+	pkey_amr_mask |= (AMR_RD_BIT|AMR_WR_BIT) << pkeyshift(EXECUTE_ONLY_KEY);
 
 	pkey_uamor_mask = ~0x0ul;
 	pkey_uamor_mask &= ~(0x3ul << pkeyshift(0));
+	pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
+
 	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++)
 		pkey_uamor_mask &= ~(0x3ul << pkeyshift(i));
 
@@ -146,8 +151,7 @@ void pkey_mm_init(struct mm_struct *mm)
 	if (static_branch_likely(&pkey_disabled))
 		return;
 	mm_pkey_allocation_map(mm) = initial_allocation_mask;
-	/* -1 means unallocated or invalid */
-	mm->context.execute_only_pkey = -1;
+	mm->context.execute_only_pkey = EXECUTE_ONLY_KEY;
 }
 
 static inline u64 read_amr(void)
@@ -326,48 +330,7 @@ static inline bool pkey_allows_readwrite(int pkey)
 
 int __execute_only_pkey(struct mm_struct *mm)
 {
-	bool need_to_set_mm_pkey = false;
-	int execute_only_pkey = mm->context.execute_only_pkey;
-	int ret;
-
-	/* Do we need to assign a pkey for mm's execute-only maps? */
-	if (execute_only_pkey == -1) {
-		/* Go allocate one to use, which might fail */
-		execute_only_pkey = mm_pkey_alloc(mm);
-		if (execute_only_pkey < 0)
-			return -1;
-		need_to_set_mm_pkey = true;
-	}
-
-	/*
-	 * We do not want to go through the relatively costly dance to set AMR
-	 * if we do not need to. Check it first and assume that if the
-	 * execute-only pkey is readwrite-disabled than we do not have to set it
-	 * ourselves.
-	 */
-	if (!need_to_set_mm_pkey && !pkey_allows_readwrite(execute_only_pkey))
-		return execute_only_pkey;
-
-	/*
-	 * Set up AMR so that it denies access for everything other than
-	 * execution.
-	 */
-	ret = __arch_set_user_pkey_access(current, execute_only_pkey,
-					  PKEY_DISABLE_ACCESS |
-					  PKEY_DISABLE_WRITE);
-	/*
-	 * If the AMR-set operation failed somehow, just return 0 and
-	 * effectively disable execute-only support.
-	 */
-	if (ret) {
-		mm_pkey_free(mm, execute_only_pkey);
-		return -1;
-	}
-
-	/* We got one, store it and use it from here on out */
-	if (need_to_set_mm_pkey)
-		mm->context.execute_only_pkey = execute_only_pkey;
-	return execute_only_pkey;
+	return mm->context.execute_only_pkey;
 }
 
 static inline bool vma_is_pkey_exec_only(struct vm_area_struct *vma)
-- 
1.7.1

[PATCH v2 5/6] powerpc/pkeys: make protection key 0 less special

[Ram Pai]

Applications need the ability to associate an address-range with some
key and latter revert to its initial default key. Pkey-0 comes close to
providing this function but falls short, because the current
implementation disallows applications to explicitly associate pkey-0 to
the address range.

Lets make pkey-0 less special and treat it almost like any other key.
Thus it can be explicitly associated with any address range, and can be
freed. This gives the application more flexibility and power.  The
ability to free pkey-0 must be used responsibily, since pkey-0 is
associated with almost all address-range by default.

Even with this change pkey-0 continues to be slightly more special
from the following point of view.
(a) it is implicitly allocated.
(b) it is the default key assigned to any address-range.
(c) its permissions cannot be modified by userspace.

NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
with all pages including kernel pages, and pkeys are also active in
kernel mode. If any permission is denied on pkey-0, the kernel running
in the context of the application will be unable to operate.

Tested on powerpc.

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: Thiago Jung Bauermann <bauerman@linux.ibm.com>
cc: Michal Such谩nek <msuchanek@suse.de
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
-----------------------------------------------------------------------
History:
	v5: . no changes since version.

	v4: . introduced PKEY_0 macro.  No bug fixes. Code
		re-arrangement to save a few cycles.

	v3: . Corrected a comment in arch_set_user_pkey_access().  .
	Clarified the header, to capture the notion that pkey-0
	permissions cannot be modified by userspace on powerpc.
      		-- comment from Thiago

	v2: . mm_pkey_is_allocated() continued to treat pkey-0 special.
		fixed it.
---
 arch/powerpc/include/asm/pkeys.h |   29 +++++++++++++++++++++++------
 arch/powerpc/mm/pkeys.c          |   13 ++++++-------
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 5ba80cf..c824528 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -13,7 +13,10 @@
 
 DECLARE_STATIC_KEY_TRUE(pkey_disabled);
 extern int pkeys_total; /* total pkeys as per device tree */
-extern u32 initial_allocation_mask; /* bits set for reserved keys */
+extern u32 initial_allocation_mask; /*  bits set for the initially allocated keys */
+extern u32 reserved_allocation_mask; /* bits set for reserved keys */
+
+#define PKEY_0	0
 
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
 			    VM_PKEY_BIT3 | VM_PKEY_BIT4)
@@ -83,15 +86,19 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
 #define __mm_pkey_is_allocated(mm, pkey)	\
 	(mm_pkey_allocation_map(mm) & pkey_alloc_mask(pkey))
 
-#define __mm_pkey_is_reserved(pkey) (initial_allocation_mask & \
+#define __mm_pkey_is_reserved(pkey) (reserved_allocation_mask & \
 				       pkey_alloc_mask(pkey))
 
 static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
-	/* A reserved key is never considered as 'explicitly allocated' */
-	return ((pkey < arch_max_pkey()) &&
-		!__mm_pkey_is_reserved(pkey) &&
-		__mm_pkey_is_allocated(mm, pkey));
+	if (pkey < 0 || pkey >= arch_max_pkey())
+		return false;
+
+	/* Reserved keys are never allocated. */
+	if (__mm_pkey_is_reserved(pkey))
+		return false;
+
+	return __mm_pkey_is_allocated(mm, pkey);
 }
 
 extern void __arch_activate_pkey(int pkey);
@@ -187,6 +194,16 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 {
 	if (static_branch_likely(&pkey_disabled))
 		return -EINVAL;
+
+	/*
+	 * userspace should not change pkey-0 permissions.
+	 * pkey-0 is associated with every page in the kernel.
+	 * If userspace denies any permission on pkey-0, the
+	 * kernel cannot operate.
+	 */
+	if (pkey == PKEY_0)
+		return init_val ? -EINVAL : 0;
+
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }
 
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 1f2389f..9098605 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -14,7 +14,8 @@
 bool pkey_execute_disable_supported;
 int  pkeys_total;		/* Total pkeys as per device tree */
 bool pkeys_devtree_defined;	/* pkey property exported by device tree */
-u32  initial_allocation_mask;	/* Bits set for reserved keys */
+u32  initial_allocation_mask;   /* Bits set for the initially allocated keys */
+u32  reserved_allocation_mask;  /* Bits set for reserved keys */
 u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
 u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
 u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
@@ -121,8 +122,9 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	initial_allocation_mask  = (0x1 << 0) | (0x1 << 1) |
-					(0x1 << EXECUTE_ONLY_KEY);
+	/* Bits are in LE format. */
+	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
+	initial_allocation_mask  = reserved_allocation_mask | (0x1 << PKEY_0);
 
 	/* register mask is in BE format */
 	pkey_amr_mask = ~0x0ul;
@@ -135,7 +137,7 @@ int pkey_initialize(void)
 	pkey_amr_mask |= (AMR_RD_BIT|AMR_WR_BIT) << pkeyshift(EXECUTE_ONLY_KEY);
 
 	pkey_uamor_mask = ~0x0ul;
-	pkey_uamor_mask &= ~(0x3ul << pkeyshift(0));
+	pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0));
 	pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
 
 	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++)
@@ -374,9 +376,6 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
 	int pkey_shift;
 	u64 amr;
 
-	if (!pkey)
-		return true;
-
 	if (!is_pkey_enabled(pkey))
 		return true;
 
-- 
1.7.1

[PATCH v2 6/6] powerpc/pkeys: Deny read/write/execute by default

[Ram Pai]

Deny all permissions on all keys, with some exceptions.  pkey-0 must
allow all permissions, or else everything comes to a screaching halt.
Execute-only key must allow execute permission.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/mm/pkeys.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 9098605..cec990c 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -128,13 +128,11 @@ int pkey_initialize(void)
 
 	/* register mask is in BE format */
 	pkey_amr_mask = ~0x0ul;
-	pkey_iamr_mask = ~0x0ul;
+	pkey_amr_mask &= ~(0x3ul << pkeyshift(PKEY_0));
 
-	for (i = 0; i < (pkeys_total - os_reserved); i++) {
-		pkey_amr_mask &= ~(0x3ul << pkeyshift(i));
-		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
-	}
-	pkey_amr_mask |= (AMR_RD_BIT|AMR_WR_BIT) << pkeyshift(EXECUTE_ONLY_KEY);
+	pkey_iamr_mask = ~0x0ul;
+	pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0));
+	pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
 
 	pkey_uamor_mask = ~0x0ul;
 	pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0));
-- 
1.7.1

Re: [PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Florian Weimer]

On 06/14/2018 02:28 AM, Ram Pai wrote:
> Assortment of fixes to pkey.
> 
> Patch 1  makes pkey consumable in multithreaded applications.
> 
> Patch 2  fixes fork behavior to inherit the key attributes.
> 
> Patch 3  A off-by-one bug made one key unusable. Fixes it.
> 
> Patch 4  Execute-only key is preallocated.
> 
> Patch 5  Makes pkey-0 less special.
> 
> Patch 6  Deny by default permissions on all unallocated keys.
> 
> Passes all selftests on powerpc. Also behavior verified to be correct
> by Florian.
> 
> Changelog:
> 
> 	v2: . fixed merge conflict with upstream code.
> 	    . Add patch 6. Makes the behavior consistent
> 	      with that on x86.

(Except signal handling, but I agree with Ram that the POWER behavior is 
the correct one.)

> Ram Pai (6):
>    powerpc/pkeys: Enable all user-allocatable pkeys at init.
>    powerpc/pkeys: Save the pkey registers before fork
>    powerpc/pkeys: fix calculation of total pkeys.
>    powerpc/pkeys: Preallocate execute-only key
>    powerpc/pkeys: make protection key 0 less special
>    powerpc/pkeys: Deny read/write/execute by default

I tested the whole series with the new selftests, with the printamr.c 
program I posted earlier, and the glibc test for pkey_alloc &c.  The 
latter required some test fixes, but now passes as well.  As far as I 
can tell, everything looks good now.

Tested-By: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian

Re: [PATCH v2 1/6] powerpc/pkeys: Enable all user-allocatable pkeys at init.

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:

> In a multithreaded application, a key allocated by one thread must
> be activate and usable on all threads.
>
> Currently this is not the case, because the UAMOR bits for all keys are
> disabled by default. When a new key is allocated in one thread, though
> the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
> for all other existing threads continue to have their bits disabled.
> Other threads have no way to set permissions on the key, effectively
> making the key useless.

This all seems a bit strongly worded to me. It's arguable whether a key
should be usable by the thread that allocated it or all threads.

You could conceivably have a design where threads are blocked from using
a key until they're given permission to do so by the thread that
allocated the key.

But we're changing the behaviour to match x86 and because we don't have
an API to grant another thread access to a key. Right?

> Enable the UAMOR bits for all keys, at process creation. Since the
> contents of UAMOR are inherited at fork, all threads are capable of
> modifying the permissions on any key.
>
> BTW: changing the permission on unallocated keys has no effect, till
> those keys are not associated with any PTEs. The kernel will anyway
> disallow to association of unallocated keys with PTEs.

This is an ABI change, which is bad, but I guess we call it a bug fix
because things didn't really work previously?

I'll tag it:

  Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
  Cc: stable@vger.kernel.org # v4.16+


cheers

Re: [PATCH v2 2/6] powerpc/pkeys: Save the pkey registers before fork

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:

> When a thread forks the contents of AMR, IAMR, UAMOR registers in the
> newly forked thread are not inherited.
>
> Save the registers before forking, for content of those
> registers to be automatically copied into the new thread.
>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: Florian Weimer <fweimer@redhat.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>

Again this is an ABI change but we'll call it a bug fix I guess.

I'll add:

  Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
  Cc: stable@vger.kernel.org # v4.16+


cheers

> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 9ef4aea..991d097 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -583,6 +583,7 @@ static void save_all(struct task_struct *tsk)
>  		__giveup_spe(tsk);
>  
>  	msr_check_and_clear(msr_all_available);
> +	thread_pkey_regs_save(&tsk->thread);
>  }
>  
>  void flush_all_to_thread(struct task_struct *tsk)
> -- 
> 1.7.1

Re: [PATCH v2 6/6] powerpc/pkeys: Deny read/write/execute by default

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:
> Deny all permissions on all keys, with some exceptions.  pkey-0 must
> allow all permissions, or else everything comes to a screaching halt.
> Execute-only key must allow execute permission.

Another ABI change.

Are we calling this a bug fix?

cheers

Re: [PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Michael Ellerman]

Florian Weimer <fweimer@redhat.com> writes:
> On 06/14/2018 02:28 AM, Ram Pai wrote:
>> Assortment of fixes to pkey.
>> 
>> Patch 1  makes pkey consumable in multithreaded applications.
>> 
>> Patch 2  fixes fork behavior to inherit the key attributes.
>> 
>> Patch 3  A off-by-one bug made one key unusable. Fixes it.
>> 
>> Patch 4  Execute-only key is preallocated.
>> 
>> Patch 5  Makes pkey-0 less special.
>> 
>> Patch 6  Deny by default permissions on all unallocated keys.
>> 
>> Passes all selftests on powerpc. Also behavior verified to be correct
>> by Florian.
>> 
>> Changelog:
>> 
>> 	v2: . fixed merge conflict with upstream code.
>> 	    . Add patch 6. Makes the behavior consistent
>> 	      with that on x86.
>
> (Except signal handling, but I agree with Ram that the POWER behavior is 
> the correct one.)
>
>> Ram Pai (6):
>>    powerpc/pkeys: Enable all user-allocatable pkeys at init.
>>    powerpc/pkeys: Save the pkey registers before fork
>>    powerpc/pkeys: fix calculation of total pkeys.
>>    powerpc/pkeys: Preallocate execute-only key
>>    powerpc/pkeys: make protection key 0 less special
>>    powerpc/pkeys: Deny read/write/execute by default
>
> I tested the whole series with the new selftests, with the printamr.c 
> program I posted earlier, and the glibc test for pkey_alloc &c.  The 
> latter required some test fixes, but now passes as well.  As far as I 
> can tell, everything looks good now.
>
> Tested-By: Florian Weimer <fweimer@redhat.com>

Thanks. I'll add that to each patch I guess, if you're happy with that?

cheers

Re: [PATCH v2 5/6] powerpc/pkeys: make protection key 0 less special

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:
> Applications need the ability to associate an address-range with some
> key and latter revert to its initial default key. Pkey-0 comes close to
> providing this function but falls short, because the current
> implementation disallows applications to explicitly associate pkey-0 to
> the address range.
>
> Lets make pkey-0 less special and treat it almost like any other key.
> Thus it can be explicitly associated with any address range, and can be
> freed. This gives the application more flexibility and power.  The
> ability to free pkey-0 must be used responsibily, since pkey-0 is
> associated with almost all address-range by default.
>
> Even with this change pkey-0 continues to be slightly more special
> from the following point of view.
> (a) it is implicitly allocated.
> (b) it is the default key assigned to any address-range.
> (c) its permissions cannot be modified by userspace.
>
> NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
> with all pages including kernel pages, and pkeys are also active in
> kernel mode. If any permission is denied on pkey-0, the kernel running
> in the context of the application will be unable to operate.

We could fix that by saving/restoring the AMR when we come into the
kernel, and switching to a kernel-AMR with all keys accessible.

We'd then need to think about copy_to/from_user() gup etc. So maybe we
don't want to do that. But it's not set in stone.

Are we calling this a bug fix?

cheers

Re: [PATCH v2 4/6] powerpc/pkeys: Preallocate execute-only key

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:

> execute-only key is allocated dynamically. This is a problem.  When a
> thread implicitly creates a execute-only key, and resets UAMOR for that
> key, the UAMOR value does not percolate to all the other threads.  Any
> other thread may ignorantly change the permissions on the key.  This can
> cause the key to be not execute-only for that thread.
>
> Preallocate the execute-only key and ensure that no thread can change
> the permission of the key, by resetting the corresponding bit in UAMOR.

OK this is a non-ABI changing bug fix AFAICS.

I'll add:

  Fixes: 5586cf61e108 ("powerpc: introduce execute-only pkey")
  Cc: stable@vger.kernel.org # v4.16+

> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index b681bec..1f2389f 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -25,6 +25,7 @@
>  #define IAMR_EX_BIT 0x1UL
>  #define PKEY_REG_BITS (sizeof(u64)*8)
>  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> +#define EXECUTE_ONLY_KEY 2

Do we ensure we have at least 3 keys anywhere?

cheers

Re: [PATCH v2 3/6] powerpc/pkeys: fix calculation of total pkeys.

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:

> Total number of pkeys calculation is off by 1. Fix it.

Oops.

Fixes: 4fb158f65ac5 ("powerpc: track allocation status of all pkeys")
Cc: stable@vger.kernel.org # v4.16+

cheers

Re: [PATCH v2 6/6] powerpc/pkeys: Deny read/write/execute by default

[Florian Weimer]

On 06/19/2018 02:39 PM, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
>> Deny all permissions on all keys, with some exceptions.  pkey-0 must
>> allow all permissions, or else everything comes to a screaching halt.
>> Execute-only key must allow execute permission.
> 
> Another ABI change.
> 
> Are we calling this a bug fix?

Yes.  Note that the default is configurable on x86 (default is deny), so 
it's not really an ABI change as such IMHO.

Thanks,
Florian

Re: [PATCH v2 1/6] powerpc/pkeys: Enable all user-allocatable pkeys at init.

[Ram Pai]

On Tue, Jun 19, 2018 at 10:39:52PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > In a multithreaded application, a key allocated by one thread must
> > be activate and usable on all threads.
> >
> > Currently this is not the case, because the UAMOR bits for all keys are
> > disabled by default. When a new key is allocated in one thread, though
> > the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
> > for all other existing threads continue to have their bits disabled.
> > Other threads have no way to set permissions on the key, effectively
> > making the key useless.
> 
> This all seems a bit strongly worded to me. It's arguable whether a key
> should be usable by the thread that allocated it or all threads.
> 
> You could conceivably have a design where threads are blocked from using
> a key until they're given permission to do so by the thread that
> allocated the key.
> 
> But we're changing the behaviour to match x86 and because we don't have
> an API to grant another thread access to a key. Right?
> 

correct. The other threads have no way to access or change the
permissions on the key.


> > Enable the UAMOR bits for all keys, at process creation. Since the
> > contents of UAMOR are inherited at fork, all threads are capable of
> > modifying the permissions on any key.
> >
> > BTW: changing the permission on unallocated keys has no effect, till
> > those keys are not associated with any PTEs. The kernel will anyway
> > disallow to association of unallocated keys with PTEs.
> 
> This is an ABI change, which is bad, but I guess we call it a bug fix
> because things didn't really work previously?

Yes its a behaviorial change for the better. There is no downside
to the change because no applications should break. Single threaded
apps will continue to just work fine. Multithreaded applications,
which were unable to consume the API/ABI, will now be able to do so.

> 
> I'll tag it:
> 
>   Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
>   Cc: stable@vger.kernel.org # v4.16+

thanks,
RP

Re: [PATCH v2 2/6] powerpc/pkeys: Save the pkey registers before fork

[Ram Pai]

On Tue, Jun 19, 2018 at 10:39:56PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > When a thread forks the contents of AMR, IAMR, UAMOR registers in the
> > newly forked thread are not inherited.
> >
> > Save the registers before forking, for content of those
> > registers to be automatically copied into the new thread.
> >
> > CC: Michael Ellerman <mpe@ellerman.id.au>
> > CC: Florian Weimer <fweimer@redhat.com>
> > CC: Andy Lutomirski <luto@kernel.org>
> > CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> 
> Again this is an ABI change but we'll call it a bug fix I guess.

yes. the same defense here too. its a behaviorial change for the better.
Single threaded applications will not see any behaviorial change.
Multithreaded apps, which were unable to consume, the behavior will now be
able to do so.

> 
> I'll add:
> 
>   Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
>   Cc: stable@vger.kernel.org # v4.16+

yes.  Thanks
RP

Re: [PATCH v2 6/6] powerpc/pkeys: Deny read/write/execute by default

[Ram Pai]

On Tue, Jun 19, 2018 at 10:39:59PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > Deny all permissions on all keys, with some exceptions.  pkey-0 must
> > allow all permissions, or else everything comes to a screaching halt.
> > Execute-only key must allow execute permission.
> 
> Another ABI change.
> 
> Are we calling this a bug fix?

It is a ABI change. There are two cases where this could break an
existing application.

a) single threaded application, depending on the AMR bits of
	unallocated keys to do something.
       
	Not sure what one can achieve doing so.


b) Multithreaded application could see the difference. The scenarios is 
	i) Thread T2 allocates a key and associates with Memory M1
	ii) Thread T1 accesses the memory M1.

	Without the patch step (ii) will be successful.
	With the patch step (ii) will fail.

	I doubt any multithreaded applications are out there depending
	on this particular behavior. And if it does, than it is
	depending on a buggy behavior.

RP

Re: [PATCH v2 5/6] powerpc/pkeys: make protection key 0 less special

[Ram Pai]

On Tue, Jun 19, 2018 at 10:40:08PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > Applications need the ability to associate an address-range with some
> > key and latter revert to its initial default key. Pkey-0 comes close to
> > providing this function but falls short, because the current
> > implementation disallows applications to explicitly associate pkey-0 to
> > the address range.
> >
> > Lets make pkey-0 less special and treat it almost like any other key.
> > Thus it can be explicitly associated with any address range, and can be
> > freed. This gives the application more flexibility and power.  The
> > ability to free pkey-0 must be used responsibily, since pkey-0 is
> > associated with almost all address-range by default.
> >
> > Even with this change pkey-0 continues to be slightly more special
> > from the following point of view.
> > (a) it is implicitly allocated.
> > (b) it is the default key assigned to any address-range.
> > (c) its permissions cannot be modified by userspace.
> >
> > NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
> > with all pages including kernel pages, and pkeys are also active in
> > kernel mode. If any permission is denied on pkey-0, the kernel running
> > in the context of the application will be unable to operate.
> 
> We could fix that by saving/restoring the AMR when we come into the
> kernel, and switching to a kernel-AMR with all keys accessible.
> 
> We'd then need to think about copy_to/from_user() gup etc. So maybe we
> don't want to do that. But it's not set in stone.
> 
> Are we calling this a bug fix?

Actually, I call it borderline bug fix. Its more of a feature.

RP

Re: [PATCH v2 4/6] powerpc/pkeys: Preallocate execute-only key

[Ram Pai]

On Tue, Jun 19, 2018 at 10:40:13PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > execute-only key is allocated dynamically. This is a problem.  When a
> > thread implicitly creates a execute-only key, and resets UAMOR for that
> > key, the UAMOR value does not percolate to all the other threads.  Any
> > other thread may ignorantly change the permissions on the key.  This can
> > cause the key to be not execute-only for that thread.
> >
> > Preallocate the execute-only key and ensure that no thread can change
> > the permission of the key, by resetting the corresponding bit in UAMOR.
> 
> OK this is a non-ABI changing bug fix AFAICS.
> 
> I'll add:
> 
>   Fixes: 5586cf61e108 ("powerpc: introduce execute-only pkey")
>   Cc: stable@vger.kernel.org # v4.16+
> 
> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> > index b681bec..1f2389f 100644
> > --- a/arch/powerpc/mm/pkeys.c
> > +++ b/arch/powerpc/mm/pkeys.c
> > @@ -25,6 +25,7 @@
> >  #define IAMR_EX_BIT 0x1UL
> >  #define PKEY_REG_BITS (sizeof(u64)*8)
> >  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> > +#define EXECUTE_ONLY_KEY 2
> 
> Do we ensure we have at least 3 keys anywhere?

No. Good to add. Can this be different patch?

However we do not have any systems with less than 16keys AFAICT.

RP

Re: [PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Florian Weimer]

On 06/19/2018 02:40 PM, Michael Ellerman wrote:
>> I tested the whole series with the new selftests, with the printamr.c
>> program I posted earlier, and the glibc test for pkey_alloc &c.  The
>> latter required some test fixes, but now passes as well.  As far as I
>> can tell, everything looks good now.
>>
>> Tested-By: Florian Weimer<fweimer@redhat.com>
> Thanks. I'll add that to each patch I guess, if you're happy with that?

Sure, but I only tested the whole series as a whole.

Thanks,
Florian

Re: [PATCH v2 4/6] powerpc/pkeys: Preallocate execute-only key

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:
> On Tue, Jun 19, 2018 at 10:40:13PM +1000, Michael Ellerman wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>>=20
>> > execute-only key is allocated dynamically. This is a problem.  When a
>> > thread implicitly creates a execute-only key, and resets UAMOR for that
>> > key, the UAMOR value does not percolate to all the other threads.  Any
>> > other thread may ignorantly change the permissions on the key.  This c=
an
>> > cause the key to be not execute-only for that thread.
>> >
>> > Preallocate the execute-only key and ensure that no thread can change
>> > the permission of the key, by resetting the corresponding bit in UAMOR.
>>=20
>> OK this is a non-ABI changing bug fix AFAICS.
>>=20
>> I'll add:
>>=20
>>   Fixes: 5586cf61e108 ("powerpc: introduce execute-only pkey")
>>   Cc: stable@vger.kernel.org # v4.16+
>>=20
>> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
>> > index b681bec..1f2389f 100644
>> > --- a/arch/powerpc/mm/pkeys.c
>> > +++ b/arch/powerpc/mm/pkeys.c
>> > @@ -25,6 +25,7 @@
>> >  #define IAMR_EX_BIT 0x1UL
>> >  #define PKEY_REG_BITS (sizeof(u64)*8)
>> >  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKE=
Y))
>> > +#define EXECUTE_ONLY_KEY 2
>>=20
>> Do we ensure we have at least 3 keys anywhere?
>
> No. Good to add. Can this be different patch?

Yes please.

> However we do not have any systems with less than 16keys AFAICT.

It's controllable by firmware so we have between 0 and =E2=88=9E keys :)

But yeah you're right it's unlikely to be a bug anyone hits in practice.

cheers

Re: [PATCH v2 2/6] powerpc/pkeys: Save the pkey registers before fork

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:

> On Tue, Jun 19, 2018 at 10:39:56PM +1000, Michael Ellerman wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>> 
>> > When a thread forks the contents of AMR, IAMR, UAMOR registers in the
>> > newly forked thread are not inherited.
>> >
>> > Save the registers before forking, for content of those
>> > registers to be automatically copied into the new thread.
>> >
>> > CC: Michael Ellerman <mpe@ellerman.id.au>
>> > CC: Florian Weimer <fweimer@redhat.com>
>> > CC: Andy Lutomirski <luto@kernel.org>
>> > CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> 
>> Again this is an ABI change but we'll call it a bug fix I guess.
>
> yes. the same defense here too. its a behaviorial change for the better.
> Single threaded applications will not see any behaviorial change.
> Multithreaded apps, which were unable to consume, the behavior will now be
> able to do so.

Well threads is one thing, but this also affects processes.

And actually without this fix it's possible that a child process could
fault on a region protected in the parent, if the value in the AMR in
the thread struct happens to block access at the time of fork(). The
value in the thread struct would be whatever was in the AMR the last
time the parent was scheduled in. I think?

cheers

Re: [PATCH v2 1/6] powerpc/pkeys: Enable all user-allocatable pkeys at init.

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:
> On Tue, Jun 19, 2018 at 10:39:52PM +1000, Michael Ellerman wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>> 
>> > In a multithreaded application, a key allocated by one thread must
>> > be activate and usable on all threads.
>> >
>> > Currently this is not the case, because the UAMOR bits for all keys are
>> > disabled by default. When a new key is allocated in one thread, though
>> > the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
>> > for all other existing threads continue to have their bits disabled.
>> > Other threads have no way to set permissions on the key, effectively
>> > making the key useless.
>> 
>> This all seems a bit strongly worded to me. It's arguable whether a key
>> should be usable by the thread that allocated it or all threads.
>> 
>> You could conceivably have a design where threads are blocked from using
>> a key until they're given permission to do so by the thread that
>> allocated the key.
>> 
>> But we're changing the behaviour to match x86 and because we don't have
>> an API to grant another thread access to a key. Right?
>
> correct. The other threads have no way to access or change the
> permissions on the key.

OK.

Though prior to patch 6 all threads have read/write permissions for all
keys, so they don't necessarily need to change permissions on a key
allocated by another thread.

>> > Enable the UAMOR bits for all keys, at process creation. Since the
>> > contents of UAMOR are inherited at fork, all threads are capable of
>> > modifying the permissions on any key.
>> >
>> > BTW: changing the permission on unallocated keys has no effect, till
>> > those keys are not associated with any PTEs. The kernel will anyway
>> > disallow to association of unallocated keys with PTEs.
>> 
>> This is an ABI change, which is bad, but I guess we call it a bug fix
>> because things didn't really work previously?
>
> Yes its a behaviorial change for the better. There is no downside
> to the change because no applications should break. Single threaded
> apps will continue to just work fine. Multithreaded applications,
> which were unable to consume the API/ABI, will now be able to do so.

Multi-threaded applications were able to use the API, as long as they
were satisfied with the semantics it provided, ie. that restrictions on
a key were only possible on the thread that allocated the key.

I'm not trying to argue for the sake of it, it's important that we
understand the subtleties of what we're changing and how it affects
existing software - even if we think there is essentially no existing
software.

I'll try and massage the change log to capture it.

I ended up with what's below.

cheers

  powerpc/pkeys: Give all threads control of their key permissions
  
  Currently in a multithreaded application, a key allocated by one
  thread is not usable by other threads. By "not usable" we mean that
  other threads are unable to change the access permissions for that
  key for themselves.
  
  When a new key is allocated in one thread, the corresponding UAMOR
  bits for that thread get enabled, however the UAMOR bits for that key
  for all other threads remain disabled.
  
  Other threads have no way to set permissions on the key, and the
  current default permissions are that read/write is enabled for all
  keys, which means the key has no effect for other threads. Although
  that may be the desired behaviour in some circumstances, having all
  threads able to control their permissions for the key is more
  flexible.
  
  The current behaviour also differs from the x86 behaviour, which is
  problematic for users.
  
  To fix this, enable the UAMOR bits for all keys, at process
  creation (in start_thread(), ie exec time). Since the contents of
  UAMOR are inherited at fork, all threads are capable of modifying the
  permissions on any key.
  
  This is technically an ABI break on powerpc, but pkey support is
  fairly new on powerpc and not widely used, and this brings us into
  line with x86.
  
  Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
  Cc: stable@vger.kernel.org # v4.16+
  Tested-by: Florian Weimer <fweimer@redhat.com>
  Signed-off-by: Ram Pai <linuxram@us.ibm.com>
  [mpe: Reword some of the changelog]
  Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Re: [PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Michael Ellerman]

Florian Weimer <fweimer@redhat.com> writes:

> On 06/19/2018 02:40 PM, Michael Ellerman wrote:
>>> I tested the whole series with the new selftests, with the printamr.c
>>> program I posted earlier, and the glibc test for pkey_alloc &c.  The
>>> latter required some test fixes, but now passes as well.  As far as I
>>> can tell, everything looks good now.
>>>
>>> Tested-By: Florian Weimer<fweimer@redhat.com>
>> Thanks. I'll add that to each patch I guess, if you're happy with that?
>
> Sure, but I only tested the whole series as a whole.

Yeah OK. We don't have a good way to express that, other than using a
merge which I'd prefer to avoid.

So I've tagged them all with your Tested-by. If any of them turn out to
have bugs you can blame me :)

cheers

Re: [PATCH v2 1/6] powerpc/pkeys: Enable all user-allocatable pkeys at init.

[Ram Pai]

On Thu, Jun 21, 2018 at 02:14:53PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > On Tue, Jun 19, 2018 at 10:39:52PM +1000, Michael Ellerman wrote:
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >> 
> >> > In a multithreaded application, a key allocated by one thread must
> >> > be activate and usable on all threads.
> >> >
> >> > Currently this is not the case, because the UAMOR bits for all keys are
> >> > disabled by default. When a new key is allocated in one thread, though
> >> > the corresponding UAMOR bits for that thread get enabled, the UAMOR bits
> >> > for all other existing threads continue to have their bits disabled.
> >> > Other threads have no way to set permissions on the key, effectively
> >> > making the key useless.
> >> 
> >> This all seems a bit strongly worded to me. It's arguable whether a key
> >> should be usable by the thread that allocated it or all threads.
> >> 
> >> You could conceivably have a design where threads are blocked from using
> >> a key until they're given permission to do so by the thread that
> >> allocated the key.
> >> 
> >> But we're changing the behaviour to match x86 and because we don't have
> >> an API to grant another thread access to a key. Right?
> >
> > correct. The other threads have no way to access or change the
> > permissions on the key.
> 
> OK.
> 
> Though prior to patch 6 all threads have read/write permissions for all
> keys, so they don't necessarily need to change permissions on a key
> allocated by another thread.
> 
> >> > Enable the UAMOR bits for all keys, at process creation. Since the
> >> > contents of UAMOR are inherited at fork, all threads are capable of
> >> > modifying the permissions on any key.
> >> >
> >> > BTW: changing the permission on unallocated keys has no effect, till
> >> > those keys are not associated with any PTEs. The kernel will anyway
> >> > disallow to association of unallocated keys with PTEs.
> >> 
> >> This is an ABI change, which is bad, but I guess we call it a bug fix
> >> because things didn't really work previously?
> >
> > Yes its a behaviorial change for the better. There is no downside
> > to the change because no applications should break. Single threaded
> > apps will continue to just work fine. Multithreaded applications,
> > which were unable to consume the API/ABI, will now be able to do so.
> 
> Multi-threaded applications were able to use the API, as long as they
> were satisfied with the semantics it provided, ie. that restrictions on
> a key were only possible on the thread that allocated the key.
> 
> I'm not trying to argue for the sake of it, it's important that we
> understand the subtleties of what we're changing and how it affects
> existing software - even if we think there is essentially no existing
> software.
> 
> I'll try and massage the change log to capture it.
> 
> I ended up with what's below.
> 
> cheers
> 
>   powerpc/pkeys: Give all threads control of their key permissions
>   
>   Currently in a multithreaded application, a key allocated by one
>   thread is not usable by other threads. By "not usable" we mean that
>   other threads are unable to change the access permissions for that
>   key for themselves.
>   
>   When a new key is allocated in one thread, the corresponding UAMOR
>   bits for that thread get enabled, however the UAMOR bits for that key
>   for all other threads remain disabled.
>   
>   Other threads have no way to set permissions on the key, and the
>   current default permissions are that read/write is enabled for all
>   keys, which means the key has no effect for other threads. Although
>   that may be the desired behaviour in some circumstances, having all
>   threads able to control their permissions for the key is more
>   flexible.
>   
>   The current behaviour also differs from the x86 behaviour, which is
>   problematic for users.
>   
>   To fix this, enable the UAMOR bits for all keys, at process
>   creation (in start_thread(), ie exec time). Since the contents of
>   UAMOR are inherited at fork, all threads are capable of modifying the
>   permissions on any key.
>   
>   This is technically an ABI break on powerpc, but pkey support is
>   fairly new on powerpc and not widely used, and this brings us into
>   line with x86.

Wow! yes it crisply captures the subtle API change and the reasoning
behind it.

RP

Re: [PATCH v2 2/6] powerpc/pkeys: Save the pkey registers before fork

[Ram Pai]

On Thu, Jun 21, 2018 at 02:13:40PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > On Tue, Jun 19, 2018 at 10:39:56PM +1000, Michael Ellerman wrote:
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >> 
> >> > When a thread forks the contents of AMR, IAMR, UAMOR registers in the
> >> > newly forked thread are not inherited.
> >> >
> >> > Save the registers before forking, for content of those
> >> > registers to be automatically copied into the new thread.
> >> >
> >> > CC: Michael Ellerman <mpe@ellerman.id.au>
> >> > CC: Florian Weimer <fweimer@redhat.com>
> >> > CC: Andy Lutomirski <luto@kernel.org>
> >> > CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> 
> >> Again this is an ABI change but we'll call it a bug fix I guess.
> >
> > yes. the same defense here too. its a behaviorial change for the better.
> > Single threaded applications will not see any behaviorial change.
> > Multithreaded apps, which were unable to consume, the behavior will now be
> > able to do so.
> 
> Well threads is one thing, but this also affects processes.
> 
> And actually without this fix it's possible that a child process could
> fault on a region protected in the parent, if the value in the AMR in
> the thread struct happens to block access at the time of fork(). The
> value in the thread struct would be whatever was in the AMR the last
> time the parent was scheduled in. I think?

right. Child processes will see stale value of AMR. Technically this
behavior is a bug, since existing applications; if any,  cannot rely on
this stale AMR value.

RP

Re: [PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Ram Pai]

On Thu, Jun 21, 2018 at 08:28:47PM +1000, Michael Ellerman wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
> > On 06/19/2018 02:40 PM, Michael Ellerman wrote:
> >>> I tested the whole series with the new selftests, with the printamr.c
> >>> program I posted earlier, and the glibc test for pkey_alloc &c.  The
> >>> latter required some test fixes, but now passes as well.  As far as I
> >>> can tell, everything looks good now.
> >>>
> >>> Tested-By: Florian Weimer<fweimer@redhat.com>
> >> Thanks. I'll add that to each patch I guess, if you're happy with that?
> >
> > Sure, but I only tested the whole series as a whole.
> 
> Yeah OK. We don't have a good way to express that, other than using a
> merge which I'd prefer to avoid.
> 
> So I've tagged them all with your Tested-by. If any of them turn out to
> have bugs you can blame me :)

I just tested the patches incrementally using the pkey selftests.

So I feel confident these patches are not bugs. I will take the blame
if the blame lands on Mpe  :)

RP

Re: [PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:
> On Thu, Jun 21, 2018 at 08:28:47PM +1000, Michael Ellerman wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>> > On 06/19/2018 02:40 PM, Michael Ellerman wrote:
>> >>> I tested the whole series with the new selftests, with the printamr.c
>> >>> program I posted earlier, and the glibc test for pkey_alloc &c.  The
>> >>> latter required some test fixes, but now passes as well.  As far as I
>> >>> can tell, everything looks good now.
>> >>>
>> >>> Tested-By: Florian Weimer<fweimer@redhat.com>
>> >> Thanks. I'll add that to each patch I guess, if you're happy with that?
>> >
>> > Sure, but I only tested the whole series as a whole.
>> 
>> Yeah OK. We don't have a good way to express that, other than using a
>> merge which I'd prefer to avoid.
>> 
>> So I've tagged them all with your Tested-by. If any of them turn out to
>> have bugs you can blame me :)
>
> I just tested the patches incrementally using the pkey selftests.
>
> So I feel confident these patches are not bugs. I will take the blame
> if the blame lands on Mpe  :)

Did you run core-pkey and ptrace-pkey?

The pkey selftests that are in tools/testing/selftests/powerpc/ptrace ?

Because those are failing for me:

  test: core_pkey
  tags: git_version:c899d94
  [FAIL] Test FAILED on line 245
  [Core Read (Running)]          AMR: 3fcfffffffffffff IAMR: 1105555555555555 UAMOR: 33cfffffffffffff
  failure: core_pkey
  
  test: ptrace_pkey
  tags: git_version:c899d94
  [FAIL] Test FAILED on line 214
  [Ptrace Read (Running)]        AMR: 3fcfffffffffffff IAMR: 1105555555555555 UAMOR: 33cfffffffffffff
  [User Write (Running)]         AMR: 3fffffffffffffff pkey1: 3 pkey2: 4 pkey3: 5
  failure: ptrace_pkey


Some of which is presumably test case bugs, but there's at least one
kernel bug with the UAMOR handling.

So this series will have to wait until next week :/

cheers

Re: [PATCH v2 0/6] powerpc/pkeys: fixes to pkeys

[Ram Pai]

On Sun, Jun 24, 2018 at 01:02:50AM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > On Thu, Jun 21, 2018 at 08:28:47PM +1000, Michael Ellerman wrote:
> >> Florian Weimer <fweimer@redhat.com> writes:
> >> > On 06/19/2018 02:40 PM, Michael Ellerman wrote:
> >> >>> I tested the whole series with the new selftests, with the printamr.c
> >> >>> program I posted earlier, and the glibc test for pkey_alloc &c.  The
> >> >>> latter required some test fixes, but now passes as well.  As far as I
> >> >>> can tell, everything looks good now.
> >> >>>
> >> >>> Tested-By: Florian Weimer<fweimer@redhat.com>
> >> >> Thanks. I'll add that to each patch I guess, if you're happy with that?
> >> >
> >> > Sure, but I only tested the whole series as a whole.
> >> 
> >> Yeah OK. We don't have a good way to express that, other than using a
> >> merge which I'd prefer to avoid.
> >> 
> >> So I've tagged them all with your Tested-by. If any of them turn out to
> >> have bugs you can blame me :)
> >
> > I just tested the patches incrementally using the pkey selftests.
> >
> > So I feel confident these patches are not bugs. I will take the blame
> > if the blame lands on Mpe  :)
> 
> Did you run core-pkey and ptrace-pkey?
> 
> The pkey selftests that are in tools/testing/selftests/powerpc/ptrace ?

No. Ran the tools/testing/selftests/vm/protection_keys.

> 
> Because those are failing for me:
> 
>   test: core_pkey
>   tags: git_version:c899d94
>   [FAIL] Test FAILED on line 245
>   [Core Read (Running)]          AMR: 3fcfffffffffffff IAMR: 1105555555555555 UAMOR: 33cfffffffffffff
>   failure: core_pkey
>   
>   test: ptrace_pkey
>   tags: git_version:c899d94
>   [FAIL] Test FAILED on line 214
>   [Ptrace Read (Running)]        AMR: 3fcfffffffffffff IAMR: 1105555555555555 UAMOR: 33cfffffffffffff
>   [User Write (Running)]         AMR: 3fffffffffffffff pkey1: 3 pkey2: 4 pkey3: 5
>   failure: ptrace_pkey
> 
> 
> Some of which is presumably test case bugs.

The test case is assuming execute-disable is disabled by default, i.e
all keys by default are execute-enabled.  The new behavior by default is
execute-disable.

The test case need to be made aware of that.



> but there's at least one
> kernel bug with the UAMOR handling.

hmm.. yes. The UAMOR of the key is getting reset when the key is freed.
An artifact of the old behavior. The new behavior should never touch the
UAMOR register after initialization.

will send fixes to the above two anomolies.
RP

Re: [PATCH v2 4/6] powerpc/pkeys: Preallocate execute-only key

[Thiago Jung Bauermann]

Hello,

My understanding is that this patch isn't upstream yet and it's not too
late for bikeshedding. Please ignore if this is not the case.

Ram Pai <linuxram@us.ibm.com> writes:

> @@ -326,48 +330,7 @@ static inline bool pkey_allows_readwrite(int pkey)
>
>  int __execute_only_pkey(struct mm_struct *mm)
>  {
> -	bool need_to_set_mm_pkey = false;
> -	int execute_only_pkey = mm->context.execute_only_pkey;
> -	int ret;
> -
> -	/* Do we need to assign a pkey for mm's execute-only maps? */
> -	if (execute_only_pkey == -1) {
> -		/* Go allocate one to use, which might fail */
> -		execute_only_pkey = mm_pkey_alloc(mm);
> -		if (execute_only_pkey < 0)
> -			return -1;
> -		need_to_set_mm_pkey = true;
> -	}
> -
> -	/*
> -	 * We do not want to go through the relatively costly dance to set AMR
> -	 * if we do not need to. Check it first and assume that if the
> -	 * execute-only pkey is readwrite-disabled than we do not have to set it
> -	 * ourselves.
> -	 */
> -	if (!need_to_set_mm_pkey && !pkey_allows_readwrite(execute_only_pkey))
> -		return execute_only_pkey;
> -
> -	/*
> -	 * Set up AMR so that it denies access for everything other than
> -	 * execution.
> -	 */
> -	ret = __arch_set_user_pkey_access(current, execute_only_pkey,
> -					  PKEY_DISABLE_ACCESS |
> -					  PKEY_DISABLE_WRITE);
> -	/*
> -	 * If the AMR-set operation failed somehow, just return 0 and
> -	 * effectively disable execute-only support.
> -	 */
> -	if (ret) {
> -		mm_pkey_free(mm, execute_only_pkey);
> -		return -1;
> -	}
> -
> -	/* We got one, store it and use it from here on out */
> -	if (need_to_set_mm_pkey)
> -		mm->context.execute_only_pkey = execute_only_pkey;
> -	return execute_only_pkey;
> +	return mm->context.execute_only_pkey;
>  }

There's no reason to have a separate  __execute_only_pkey() function
anymore. Its single line can go directly in execute_only_pkey(), defined
in <asm/pkeys.h>.
-- 
Thiago Jung Bauermann
IBM Linux Technology Center