[PATCH v2] powerpc: do not allow userspace to modify execute-only pkey

[PATCH v2] powerpc: do not allow userspace to modify execute-only pkey

[Ram Pai]

When mprotect(....,PROT_EXEC) is called, the kernel allocates a
execute-only pkey and associates the pkey with the given address space.
The permission of this key should not be modifiable from userspace.
However a bug in the current implementation lets the permissions on the
key modifiable from userspace.

Whenever a key is allocated through mm_pkey_alloc(), the kernel programs
the UAMOR register to allow userspace to change permissions on the key.
This is fine for keys explicitly allocated through the
sys_pkey_alloc(). But for execute-only pkey, it must be disallowed.
Restructured the code to fix the bug.

cc: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
cc: Michael Ellermen <mpe@ellerman.id.au>

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
History:

	v2: Thiago noticed a bug -- __execute_only_pkey() will always fail
	    since it calls is_pkey_enabled() which always returns false
	    for execute_only key. is_pkey_enabled() returns false
	    because UAMOR bit for the execute_only key is and never be set.
	    Fixed it.


 arch/powerpc/include/asm/pkeys.h |   24 ++++------------
 arch/powerpc/mm/pkeys.c          |   57 ++++++++++++++++++++++++++++++-------
 2 files changed, 52 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 31a6976..3a9b82b 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -113,6 +113,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 
 extern void __arch_activate_pkey(int pkey);
 extern void __arch_deactivate_pkey(int pkey);
+extern int __mm_pkey_alloc(struct mm_struct *mm);
+
 /*
  * Returns a positive, 5-bit key on success, or -1 on failure.
  * Relies on the mmap_sem to protect against concurrency in mm_pkey_alloc() and
@@ -120,29 +122,14 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
  */
 static inline int mm_pkey_alloc(struct mm_struct *mm)
 {
-	/*
-	 * Note: this is the one and only place we make sure that the pkey is
-	 * valid as far as the hardware is concerned. The rest of the kernel
-	 * trusts that only good, valid pkeys come out of here.
-	 */
-	u32 all_pkeys_mask = (u32)(~(0x0));
 	int ret;
 
 	if (static_branch_likely(&pkey_disabled))
 		return -1;
 
+	ret = __mm_pkey_alloc(mm);
 	/*
-	 * Are we out of pkeys? We must handle this specially because ffz()
-	 * behavior is undefined if there are no zeros.
-	 */
-	if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
-		return -1;
-
-	ret = ffz((u32)mm_pkey_allocation_map(mm));
-	__mm_pkey_allocated(mm, ret);
-
-	/*
-	 * Enable the key in the hardware
+	 * Enable userspace to modify the key permissions.
 	 */
 	if (ret > 0)
 		__arch_activate_pkey(ret);
@@ -158,7 +145,8 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
 		return -EINVAL;
 
 	/*
-	 * Disable the key in the hardware
+	 * Reset the key and disable userspace
+	 * from modifying the key permissions.
 	 */
 	__arch_deactivate_pkey(pkey);
 	__mm_pkey_free(mm, pkey);
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index d6873b4..e81d59e 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -190,6 +190,9 @@ static inline void write_uamor(u64 value)
 	mtspr(SPRN_UAMOR, value);
 }
 
+/*
+ * return true if userspace can modify the pkey permissions.
+ */
 static bool is_pkey_enabled(int pkey)
 {
 	u64 uamor = read_uamor();
@@ -228,7 +231,10 @@ static void pkey_status_change(int pkey, bool enable)
 	init_amr(pkey, 0x0);
 	init_iamr(pkey, 0x0);
 
-	/* Enable/disable key */
+	/*
+	 * Enable/disable userspace to/from modifying the permissions
+	 * on the key
+	 */
 	old_uamor = read_uamor();
 	if (enable)
 		old_uamor |= (0x3ul << pkeyshift(pkey));
@@ -247,19 +253,35 @@ void __arch_deactivate_pkey(int pkey)
 	pkey_status_change(pkey, false);
 }
 
-/*
- * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
- * specified in @init_val.
- */
-int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+int __mm_pkey_alloc(struct mm_struct *mm)
+{
+	/*
+	 * Note: this is the one and only place we make sure that the pkey is
+	 * valid as far as the hardware is concerned. The rest of the kernel
+	 * trusts that only good, valid pkeys come out of here.
+	 */
+	u32 all_pkeys_mask = (u32)(~(0x0));
+	int ret;
+
+	/*
+	 * Are we out of pkeys? We must handle this specially because ffz()
+	 * behavior is undefined if there are no zeros.
+	 */
+	if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+		return -1;
+
+	ret = ffz((u32)mm_pkey_allocation_map(mm));
+	__mm_pkey_allocated(mm, ret);
+
+	return ret;
+}
+
+static int set_user_pkey_access(struct task_struct *tsk, int pkey,
 				unsigned long init_val)
 {
 	u64 new_amr_bits = 0x0ul;
 	u64 new_iamr_bits = 0x0ul;
 
-	if (!is_pkey_enabled(pkey))
-		return -EINVAL;
-
 	if (init_val & PKEY_DISABLE_EXECUTE) {
 		if (!pkey_execute_disable_supported)
 			return -EINVAL;
@@ -277,6 +299,19 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 	return 0;
 }
 
+/*
+ * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
+ * specified in @init_val.
+ */
+int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+				unsigned long init_val)
+{
+	if (!is_pkey_enabled(pkey))
+		return -EINVAL;
+
+	return set_user_pkey_access(tsk, pkey, init_val);
+}
+
 void thread_pkey_regs_save(struct thread_struct *thread)
 {
 	if (static_branch_likely(&pkey_disabled))
@@ -336,7 +371,7 @@ int __execute_only_pkey(struct mm_struct *mm)
 	/* 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);
+		execute_only_pkey = __mm_pkey_alloc(mm);
 		if (execute_only_pkey < 0)
 			return -1;
 		need_to_set_mm_pkey = true;
@@ -355,7 +390,7 @@ int __execute_only_pkey(struct mm_struct *mm)
 	 * Set up AMR so that it denies access for everything other than
 	 * execution.
 	 */
-	ret = __arch_set_user_pkey_access(current, execute_only_pkey,
+	ret = set_user_pkey_access(current, execute_only_pkey,
 					  PKEY_DISABLE_ACCESS |
 					  PKEY_DISABLE_WRITE);
 	/*
-- 
1.7.1