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 <bauer...@linux.vnet.ibm.com> cc: Michael Ellermen <m...@ellerman.id.au> Signed-off-by: Ram Pai <linux...@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