Re: [PATCH 4/9] x86, pkeys: override pkey when moving away from PROT_EXEC

[Ram Pai]

On Mon, Mar 26, 2018 at 10:27:27AM -0700, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> 
> 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 <shakeelb@xxxxxxxxxx>
> 
> Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys support")
> Cc: stable@xxxxxxxxxx
> Cc: Ram Pai <linuxram@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
> Cc: Michael Ellermen <mpe@xxxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Shuah Khan <shuah@xxxxxxxxxx>
> ---
> 
>  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