[PATCH] 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 the keys explicitly allocated through the
sys_pkey_alloc(). But for execute-only pkey, this should not be allowed.
Restructured the code to fix the bug.

Signed-off-by: Ram Pai <linuxram at us.ibm.com>
---
 arch/powerpc/include/asm/pkeys.h | 24 ++++++------------------
 arch/powerpc/mm/pkeys.c          | 30 ++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index b598fa9..0d3c630 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 e7a9e34..58bbb55 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -228,7 +228,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,6 +250,29 @@ void __arch_deactivate_pkey(int pkey)
 	pkey_status_change(pkey, false);
 }
 
+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;
+}
+
 /*
  * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
  * specified in @init_val.
@@ -336,7 +362,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;
-- 
1.8.3.1