On Mon, Mar 26, 2018 at 10:27:27AM -0700, Dave Hansen wrote:
>
> From: Dave Hansen <dave.han...@linux.intel.com>
>
> I got a bug report that the following code (roughly) was
> causing a SIGSEGV:
>
> mprotect(ptr, size, PROT_EXEC);
> mprotect(ptr, size, PROT_NONE);
> mprotect(ptr, size, PROT_READ);
> *ptr = 100;
>
> The problem is hit when the mprotect(PROT_EXEC)
> is implicitly assigned a protection key to the VMA, and made
> that key ACCESS_DENY|WRITE_DENY. The PROT_NONE mprotect()
> failed to remove the protection key, and the PROT_NONE->
> PROT_READ left the PTE usable, but the pkey still in place
> and left the memory inaccessible.
>
> To fix this, we ensure that we always "override" the pkee
> at mprotect() if the VMA does not have execute-only
> permissions, but the VMA has the execute-only pkey.
>
> We had a check for PROT_READ/WRITE, but it did not work
> for PROT_NONE. This entirely removes the PROT_* checks,
> which ensures that PROT_NONE now works.
>
> Reported-by: Shakeel Butt <shake...@google.com>
>
> Signed-off-by: Dave Hansen <dave.han...@linux.intel.com>
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys
> support")
> Cc: sta...@kernel.org
> Cc: Ram Pai <linux...@us.ibm.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Dave Hansen <dave.han...@intel.com>
> Cc: Michael Ellermen <m...@ellerman.id.au>
> Cc: Ingo Molnar <mi...@kernel.org>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Shuah Khan <sh...@kernel.org>
> ---
>
> b/arch/x86/include/asm/pkeys.h | 12 +++++++++++-
> b/arch/x86/mm/pkeys.c | 19 ++++++++++---------
> 2 files changed, 21 insertions(+), 10 deletions(-)
>
> diff -puN
> arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively
> arch/x86/include/asm/pkeys.h
> ---
> a/arch/x86/include/asm/pkeys.h~pkeys-abandon-exec-only-pkey-more-aggressively
> 2018-03-26 10:22:35.380170193 -0700
> +++ b/arch/x86/include/asm/pkeys.h 2018-03-26 10:22:35.385170193 -0700
> @@ -2,6 +2,8 @@
> #ifndef _ASM_X86_PKEYS_H
> #define _ASM_X86_PKEYS_H
>
> +#define ARCH_DEFAULT_PKEY 0
> +
> #define arch_max_pkey() (boot_cpu_has(X86_FEATURE_OSPKE) ? 16 : 1)
>
> extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> @@ -15,7 +17,7 @@ extern int __execute_only_pkey(struct mm
> static inline int execute_only_pkey(struct mm_struct *mm)
> {
> if (!boot_cpu_has(X86_FEATURE_OSPKE))
> - return 0;
> + return ARCH_DEFAULT_PKEY;
>
> return __execute_only_pkey(mm);
> }
> @@ -56,6 +58,14 @@ bool mm_pkey_is_allocated(struct mm_stru
> return false;
> if (pkey >= arch_max_pkey())
> return false;
> + /*
> + * The exec-only pkey is set in the allocation map, but
> + * is not available to any of the user interfaces like
> + * mprotect_pkey().
> + */
> + if (pkey == mm->context.execute_only_pkey)
> + return false;
> +
> return mm_pkey_allocation_map(mm) & (1U << pkey);
> }
>
> diff -puN arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively
> arch/x86/mm/pkeys.c
> --- a/arch/x86/mm/pkeys.c~pkeys-abandon-exec-only-pkey-more-aggressively
> 2018-03-26 10:22:35.381170193 -0700
> +++ b/arch/x86/mm/pkeys.c 2018-03-26 10:22:35.385170193 -0700
> @@ -94,15 +94,7 @@ int __arch_override_mprotect_pkey(struct
> */
> if (pkey != -1)
> return pkey;
> - /*
> - * Look for a protection-key-drive execute-only mapping
> - * which is now being given permissions that are not
> - * execute-only. Move it back to the default pkey.
> - */
> - if (vma_is_pkey_exec_only(vma) &&
> - (prot & (PROT_READ|PROT_WRITE))) {
> - return 0;
> - }
> +
Dave,
this can be simply:
if ((vma_is_pkey_exec_only(vma) && (prot != PROT_EXEC))
return ARCH_DEFAULT_PKEY;
No?
RP
