[PATCH] powerpc: do not allow userspace to modify execute-only pkey
DORMANTno replies
From: Ram Pai <hidden>
Date: 2018-03-27 01:16:44
Subsystem:
linux for powerpc (32-bit and 64-bit), the rest · Maintainers:
Madhavan Srinivasan, Linus Torvalds
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 <redacted> --- 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