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

[Dave Hansen]

On 04/30/2018 12:51 AM, Ram Pai wrote:
>   /*
>* 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_EXEC)) <
>   return ARCH_DEFAULT_PKEY;
> 
>   /*
>* The mapping is execute-only.  Go try to get the
>* execute-only protection key.  If we fail to do that,
>* fall through as if we do not have execute-only
>* support.
>*/
>   if (prot == PROT_EXEC) {
>   pkey = execute_only_pkey(vma->vm_mm);
>   if (pkey > 0)
>   return pkey;
>   }

Yes, that would also work.  It's just a matter of whether you prefer
having the prot==PROT_EXEC checks in one place or two.  I'd rather leave
it the way I've got it unless there are major objections since it's been
tested.

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

[Dave Hansen]

On 04/30/2018 12:51 AM, Ram Pai wrote:
>   /*
>* 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_EXEC)) <
>   return ARCH_DEFAULT_PKEY;
> 
>   /*
>* The mapping is execute-only.  Go try to get the
>* execute-only protection key.  If we fail to do that,
>* fall through as if we do not have execute-only
>* support.
>*/
>   if (prot == PROT_EXEC) {
>   pkey = execute_only_pkey(vma->vm_mm);
>   if (pkey > 0)
>   return pkey;
>   }

Yes, that would also work.  It's just a matter of whether you prefer
having the prot==PROT_EXEC checks in one place or two.  I'd rather leave
it the way I've got it unless there are major objections since it's been
tested.

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

[Ram Pai]

On Thu, Apr 26, 2018 at 10:57:31AM -0700, Dave Hansen wrote:
> On 04/06/2018 06:09 PM, Ram Pai wrote:
> > Well :). my point is add this code and delete the other
> > code that you add later in that function.
> 
> I don't think I'm understanding what your suggestion was.  I looked at
> the code and I honestly do not think I can remove any of it.
> 
> For the plain (non-explicit pkey_mprotect()) case, there are exactly
> four paths through __arch_override_mprotect_pkey(), resulting in three
> different results.
> 
> 1. New prot==PROT_EXEC, no pkey-exec support -> do not override
> 2. New prot!=PROT_EXEC, old VMA not PROT_EXEC-> do not override
> 3. New prot==PROT_EXEC, w/ pkey-exec support -> override to exec pkey
> 4. New prot!=PROT_EXEC, old VMA is PROT_EXEC -> override to default
> 
> I don't see any redundancy there, or any code that we can eliminate or
> simplify.  It was simpler before, but that's what where bug was.

Your code is fine.  But than the following code accomplishes the same
outcome; arguably with a one line change. Its not a big deal. Just
trying to clarify my comment.

int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot, int 
pkey)
{
/*
 * Is this an mprotect_pkey() call?  If so, never
 * override the value that came from the user.
 */
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_EXEC)) <
return ARCH_DEFAULT_PKEY;

/*
 * The mapping is execute-only.  Go try to get the
 * execute-only protection key.  If we fail to do that,
 * fall through as if we do not have execute-only
 * support.
 */
if (prot == PROT_EXEC) {
pkey = execute_only_pkey(vma->vm_mm);
if (pkey > 0)
return pkey;
}
/*
 * This is a vanilla, non-pkey mprotect (or we failed to
 * setup execute-only), inherit the pkey from the VMA we
 * are working on.
 */
return vma_pkey(vma);
}

-- 
Ram Pai

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

[Ram Pai]

On Thu, Apr 26, 2018 at 10:57:31AM -0700, Dave Hansen wrote:
> On 04/06/2018 06:09 PM, Ram Pai wrote:
> > Well :). my point is add this code and delete the other
> > code that you add later in that function.
> 
> I don't think I'm understanding what your suggestion was.  I looked at
> the code and I honestly do not think I can remove any of it.
> 
> For the plain (non-explicit pkey_mprotect()) case, there are exactly
> four paths through __arch_override_mprotect_pkey(), resulting in three
> different results.
> 
> 1. New prot==PROT_EXEC, no pkey-exec support -> do not override
> 2. New prot!=PROT_EXEC, old VMA not PROT_EXEC-> do not override
> 3. New prot==PROT_EXEC, w/ pkey-exec support -> override to exec pkey
> 4. New prot!=PROT_EXEC, old VMA is PROT_EXEC -> override to default
> 
> I don't see any redundancy there, or any code that we can eliminate or
> simplify.  It was simpler before, but that's what where bug was.

Your code is fine.  But than the following code accomplishes the same
outcome; arguably with a one line change. Its not a big deal. Just
trying to clarify my comment.

int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot, int 
pkey)
{
/*
 * Is this an mprotect_pkey() call?  If so, never
 * override the value that came from the user.
 */
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_EXEC)) <
return ARCH_DEFAULT_PKEY;

/*
 * The mapping is execute-only.  Go try to get the
 * execute-only protection key.  If we fail to do that,
 * fall through as if we do not have execute-only
 * support.
 */
if (prot == PROT_EXEC) {
pkey = execute_only_pkey(vma->vm_mm);
if (pkey > 0)
return pkey;
}
/*
 * This is a vanilla, non-pkey mprotect (or we failed to
 * setup execute-only), inherit the pkey from the VMA we
 * are working on.
 */
return vma_pkey(vma);
}

-- 
Ram Pai

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

[Dave Hansen]

On 04/26/2018 01:55 AM, Thomas Gleixner wrote:
>> Hi Dave, are you planning to send the next version of this patch or
>> going with this one?
> Right, some enlightment would be appreciated. I'm lost in the dozen
> different threads discussing this back and forth.

Shakeel, thanks for the reminder!

I'll send an updated set.  I got lost myself and thought this had been
picked up.

There were a few minor comments on the [v2] set that I've addressed.
I'll also check with Ram to make sure he's OK with this on ppc.  We had
some dueling patches at some point.

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

[Dave Hansen]

On 04/26/2018 01:55 AM, Thomas Gleixner wrote:
>> Hi Dave, are you planning to send the next version of this patch or
>> going with this one?
> Right, some enlightment would be appreciated. I'm lost in the dozen
> different threads discussing this back and forth.

Shakeel, thanks for the reminder!

I'll send an updated set.  I got lost myself and thought this had been
picked up.

There were a few minor comments on the [v2] set that I've addressed.
I'll also check with Ram to make sure he's OK with this on ppc.  We had
some dueling patches at some point.

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

[Dave Hansen]

On 04/06/2018 06:09 PM, Ram Pai wrote:
> Well :). my point is add this code and delete the other
> code that you add later in that function.

I don't think I'm understanding what your suggestion was.  I looked at
the code and I honestly do not think I can remove any of it.

For the plain (non-explicit pkey_mprotect()) case, there are exactly
four paths through __arch_override_mprotect_pkey(), resulting in three
different results.

1. New prot==PROT_EXEC, no pkey-exec support -> do not override
2. New prot!=PROT_EXEC, old VMA not PROT_EXEC-> do not override
3. New prot==PROT_EXEC, w/ pkey-exec support -> override to exec pkey
4. New prot!=PROT_EXEC, old VMA is PROT_EXEC -> override to default

I don't see any redundancy there, or any code that we can eliminate or
simplify.  It was simpler before, but that's what where bug was.

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

[Dave Hansen]

On 04/06/2018 06:09 PM, Ram Pai wrote:
> Well :). my point is add this code and delete the other
> code that you add later in that function.

I don't think I'm understanding what your suggestion was.  I looked at
the code and I honestly do not think I can remove any of it.

For the plain (non-explicit pkey_mprotect()) case, there are exactly
four paths through __arch_override_mprotect_pkey(), resulting in three
different results.

1. New prot==PROT_EXEC, no pkey-exec support -> do not override
2. New prot!=PROT_EXEC, old VMA not PROT_EXEC-> do not override
3. New prot==PROT_EXEC, w/ pkey-exec support -> override to exec pkey
4. New prot!=PROT_EXEC, old VMA is PROT_EXEC -> override to default

I don't see any redundancy there, or any code that we can eliminate or
simplify.  It was simpler before, but that's what where bug was.

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

[Thomas Gleixner]

On Wed, 25 Apr 2018, Shakeel Butt wrote:
> On Mon, Mar 26, 2018 at 5:27 PM, Dave Hansen
>  wrote:
> >
> > From: Dave Hansen 
> >
> > 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 
> >
> > Signed-off-by: Dave Hansen 
> > Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection 
> > keys support")
> 
> Hi Dave, are you planning to send the next version of this patch or
> going with this one?

Right, some enlightment would be appreciated. I'm lost in the dozen
different threads discussing this back and forth.

Thanks,

tglx

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

[Thomas Gleixner]

On Wed, 25 Apr 2018, Shakeel Butt wrote:
> On Mon, Mar 26, 2018 at 5:27 PM, Dave Hansen
>  wrote:
> >
> > From: Dave Hansen 
> >
> > 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 
> >
> > Signed-off-by: Dave Hansen 
> > Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection 
> > keys support")
> 
> Hi Dave, are you planning to send the next version of this patch or
> going with this one?

Right, some enlightment would be appreciated. I'm lost in the dozen
different threads discussing this back and forth.

Thanks,

tglx

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

[Shakeel Butt]

On Mon, Mar 26, 2018 at 5:27 PM, Dave Hansen
 wrote:
>
> From: Dave Hansen 
>
> 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 
>
> Signed-off-by: Dave Hansen 
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys 
> support")

Hi Dave, are you planning to send the next version of this patch or
going with this one?

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

[Shakeel Butt]

On Mon, Mar 26, 2018 at 5:27 PM, Dave Hansen
 wrote:
>
> From: Dave Hansen 
>
> 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 
>
> Signed-off-by: Dave Hansen 
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys 
> support")

Hi Dave, are you planning to send the next version of this patch or
going with this one?

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

[Ram Pai]

On Fri, Apr 06, 2018 at 05:47:29PM -0700, Dave Hansen wrote:
> On 04/06/2018 05:09 PM, Ram Pai wrote:
> >> -  /*
> >> -   * 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;
> 
> Yes, but we're removing that code entirely. :)

Well :). my point is add this code and delete the other
code that you add later in that function.

RP



-- 
Ram Pai

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

[Ram Pai]

On Fri, Apr 06, 2018 at 05:47:29PM -0700, Dave Hansen wrote:
> On 04/06/2018 05:09 PM, Ram Pai wrote:
> >> -  /*
> >> -   * 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;
> 
> Yes, but we're removing that code entirely. :)

Well :). my point is add this code and delete the other
code that you add later in that function.

RP



-- 
Ram Pai

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

[Dave Hansen]

On 04/06/2018 05:09 PM, Ram Pai wrote:
>> -/*
>> - * 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;

Yes, but we're removing that code entirely. :)

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

[Dave Hansen]

On 04/06/2018 05:09 PM, Ram Pai wrote:
>> -/*
>> - * 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;

Yes, but we're removing that code entirely. :)

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 
> 
> 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 
> 
> Signed-off-by: Dave Hansen 
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys 
> support")
> Cc: sta...@kernel.org
> Cc: Ram Pai 
> Cc: Thomas Gleixner 
> Cc: Dave Hansen 
> Cc: Michael Ellermen 
> Cc: Ingo Molnar 
> Cc: Andrew Morton 
> Cc: Shuah Khan 
> ---
> 
>  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.h2018-03-26 10:22:35.385170193 -0700
> @@ -2,6 +2,8 @@
>  #ifndef _ASM_X86_PKEYS_H
>  #define _ASM_X86_PKEYS_H
> 
> +#define ARCH_DEFAULT_PKEY0
> +
>  #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

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 
> 
> 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 
> 
> Signed-off-by: Dave Hansen 
> Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys 
> support")
> Cc: sta...@kernel.org
> Cc: Ram Pai 
> Cc: Thomas Gleixner 
> Cc: Dave Hansen 
> Cc: Michael Ellermen 
> Cc: Ingo Molnar 
> Cc: Andrew Morton 
> Cc: Shuah Khan 
> ---
> 
>  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.h2018-03-26 10:22:35.385170193 -0700
> @@ -2,6 +2,8 @@
>  #ifndef _ASM_X86_PKEYS_H
>  #define _ASM_X86_PKEYS_H
> 
> +#define ARCH_DEFAULT_PKEY0
> +
>  #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

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

[Dave Hansen]

On 03/23/2018 12:45 PM, Thomas Gleixner wrote:
>> The fixes tag makes sense in general even if the patch is not tagged for
>> stable. It gives you immediate context and I use it a lot to look why this
>> went unnoticed or what the context of that change was.
> That said, I'm even lazier than you and prefer you to dig up the original
> commit :)

I'll have these tags in the next repost.

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

[Dave Hansen]

On 03/23/2018 12:45 PM, Thomas Gleixner wrote:
>> The fixes tag makes sense in general even if the patch is not tagged for
>> stable. It gives you immediate context and I use it a lot to look why this
>> went unnoticed or what the context of that change was.
> That said, I'm even lazier than you and prefer you to dig up the original
> commit :)

I'll have these tags in the next repost.

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

[Thomas Gleixner]

On Fri, 23 Mar 2018, Thomas Gleixner wrote:

> On Fri, 23 Mar 2018, Dave Hansen wrote:
> 
> > On 03/23/2018 12:15 PM, Shakeel Butt wrote:
> > >> 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 
> > >> Signed-off-by: Dave Hansen 
> > > Should there be a 'Fixes' tag? Also should this patch go to stable?
> > 
> > There could be, but I'm to lazy to dig up the original commit.  Does it
> > matter?
> > 
> > And, yes, I think it probably makes sense for -stable.  I'll add that if
> > I resend this series.
> 
> The fixes tag makes sense in general even if the patch is not tagged for
> stable. It gives you immediate context and I use it a lot to look why this
> went unnoticed or what the context of that change was.

That said, I'm even lazier than you and prefer you to dig up the original
commit :)

Thanks,

tglx

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

[Thomas Gleixner]

On Fri, 23 Mar 2018, Thomas Gleixner wrote:

> On Fri, 23 Mar 2018, Dave Hansen wrote:
> 
> > On 03/23/2018 12:15 PM, Shakeel Butt wrote:
> > >> 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 
> > >> Signed-off-by: Dave Hansen 
> > > Should there be a 'Fixes' tag? Also should this patch go to stable?
> > 
> > There could be, but I'm to lazy to dig up the original commit.  Does it
> > matter?
> > 
> > And, yes, I think it probably makes sense for -stable.  I'll add that if
> > I resend this series.
> 
> The fixes tag makes sense in general even if the patch is not tagged for
> stable. It gives you immediate context and I use it a lot to look why this
> went unnoticed or what the context of that change was.

That said, I'm even lazier than you and prefer you to dig up the original
commit :)

Thanks,

tglx

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

[Thomas Gleixner]

On Fri, 23 Mar 2018, Dave Hansen wrote:

> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
> >> 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 
> >> Signed-off-by: Dave Hansen 
> > Should there be a 'Fixes' tag? Also should this patch go to stable?
> 
> There could be, but I'm to lazy to dig up the original commit.  Does it
> matter?
> 
> And, yes, I think it probably makes sense for -stable.  I'll add that if
> I resend this series.

The fixes tag makes sense in general even if the patch is not tagged for
stable. It gives you immediate context and I use it a lot to look why this
went unnoticed or what the context of that change was.

Thanks,

tglx

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

[Thomas Gleixner]

On Fri, 23 Mar 2018, Dave Hansen wrote:

> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
> >> 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 
> >> Signed-off-by: Dave Hansen 
> > Should there be a 'Fixes' tag? Also should this patch go to stable?
> 
> There could be, but I'm to lazy to dig up the original commit.  Does it
> matter?
> 
> And, yes, I think it probably makes sense for -stable.  I'll add that if
> I resend this series.

The fixes tag makes sense in general even if the patch is not tagged for
stable. It gives you immediate context and I use it a lot to look why this
went unnoticed or what the context of that change was.

Thanks,

tglx

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

[Dave Hansen]

On 03/23/2018 12:27 PM, Shakeel Butt wrote:
> On Fri, Mar 23, 2018 at 12:23 PM, Dave Hansen  wrote:
>> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
 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 
 Signed-off-by: Dave Hansen 
>>> Should there be a 'Fixes' tag? Also should this patch go to stable?
>> There could be, but I'm to lazy to dig up the original commit.  Does it
>> matter?
>>
> I think for stable 'Fixes' is usually preferable.

This one is a no-brainer.  If pkeys.c is there, it's necesary.

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

[Dave Hansen]

On 03/23/2018 12:27 PM, Shakeel Butt wrote:
> On Fri, Mar 23, 2018 at 12:23 PM, Dave Hansen  wrote:
>> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
 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 
 Signed-off-by: Dave Hansen 
>>> Should there be a 'Fixes' tag? Also should this patch go to stable?
>> There could be, but I'm to lazy to dig up the original commit.  Does it
>> matter?
>>
> I think for stable 'Fixes' is usually preferable.

This one is a no-brainer.  If pkeys.c is there, it's necesary.

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

[Shakeel Butt]

On Fri, Mar 23, 2018 at 12:23 PM, Dave Hansen  wrote:
> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
>>> 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 
>>> Signed-off-by: Dave Hansen 
>> Should there be a 'Fixes' tag? Also should this patch go to stable?
>
> There could be, but I'm to lazy to dig up the original commit.  Does it
> matter?
>

I think for stable 'Fixes' is usually preferable.

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

[Shakeel Butt]

On Fri, Mar 23, 2018 at 12:23 PM, Dave Hansen  wrote:
> On 03/23/2018 12:15 PM, Shakeel Butt wrote:
>>> 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 
>>> Signed-off-by: Dave Hansen 
>> Should there be a 'Fixes' tag? Also should this patch go to stable?
>
> There could be, but I'm to lazy to dig up the original commit.  Does it
> matter?
>

I think for stable 'Fixes' is usually preferable.

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

[Dave Hansen]

On 03/23/2018 12:15 PM, Shakeel Butt wrote:
>> 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 
>> Signed-off-by: Dave Hansen 
> Should there be a 'Fixes' tag? Also should this patch go to stable?

There could be, but I'm to lazy to dig up the original commit.  Does it
matter?

And, yes, I think it probably makes sense for -stable.  I'll add that if
I resend this series.

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

[Dave Hansen]

On 03/23/2018 12:15 PM, Shakeel Butt wrote:
>> 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 
>> Signed-off-by: Dave Hansen 
> Should there be a 'Fixes' tag? Also should this patch go to stable?

There could be, but I'm to lazy to dig up the original commit.  Does it
matter?

And, yes, I think it probably makes sense for -stable.  I'll add that if
I resend this series.

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

[Shakeel Butt]

On Fri, Mar 23, 2018 at 11:09 AM, Dave Hansen
 wrote:
>
> From: Dave Hansen 
>
> 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 
>
> Signed-off-by: Dave Hansen 

Should there be a 'Fixes' tag? Also should this patch go to stable?

> Cc: Ram Pai 
> Cc: Thomas Gleixner 
> Cc: Dave Hansen 
> Cc: Michael Ellermen 
> Cc: Ingo Molnar 
> Cc: Andrew Morton 
> Cc: Shuah Khan 
> ---
>
>  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-21 15:47:49.810198922 -0700
> +++ b/arch/x86/include/asm/pkeys.h  2018-03-21 15:47:49.816198922 -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-21 15:47:49.812198922 -0700
> +++ b/arch/x86/mm/pkeys.c   2018-03-21 15:47:49.816198922 -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;
> -   }
> +
> /*
>  * The mapping is execute-only.  Go try to get the
>  * execute-only protection key.  If we fail to do that,
> @@ -113,7 +105,16 @@ int __arch_override_mprotect_pkey(struct
> pkey = execute_only_pkey(vma->vm_mm);
> if (pkey > 0)
> return pkey;
> +   } else if (vma_is_pkey_exec_only(vma)) {
> +   /*
> +* Protections are *not* PROT_EXEC, but the mapping
> +* is using the exec-only pkey.  This mapping was
> +* PROT_EXEC and will no longer be.  Move back to
> +* the default pkey.
> +*/
> +   return ARCH_DEFAULT_PKEY;
> }
> +
> /*
>  * This is a vanilla, non-pkey mprotect (or we failed to
>  * setup execute-only), inherit the pkey from the VMA we
> _

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

[Shakeel Butt]

On Fri, Mar 23, 2018 at 11:09 AM, Dave Hansen
 wrote:
>
> From: Dave Hansen 
>
> 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 
>
> Signed-off-by: Dave Hansen 

Should there be a 'Fixes' tag? Also should this patch go to stable?

> Cc: Ram Pai 
> Cc: Thomas Gleixner 
> Cc: Dave Hansen 
> Cc: Michael Ellermen 
> Cc: Ingo Molnar 
> Cc: Andrew Morton 
> Cc: Shuah Khan 
> ---
>
>  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-21 15:47:49.810198922 -0700
> +++ b/arch/x86/include/asm/pkeys.h  2018-03-21 15:47:49.816198922 -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-21 15:47:49.812198922 -0700
> +++ b/arch/x86/mm/pkeys.c   2018-03-21 15:47:49.816198922 -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;
> -   }
> +
> /*
>  * The mapping is execute-only.  Go try to get the
>  * execute-only protection key.  If we fail to do that,
> @@ -113,7 +105,16 @@ int __arch_override_mprotect_pkey(struct
> pkey = execute_only_pkey(vma->vm_mm);
> if (pkey > 0)
> return pkey;
> +   } else if (vma_is_pkey_exec_only(vma)) {
> +   /*
> +* Protections are *not* PROT_EXEC, but the mapping
> +* is using the exec-only pkey.  This mapping was
> +* PROT_EXEC and will no longer be.  Move back to
> +* the default pkey.
> +*/
> +   return ARCH_DEFAULT_PKEY;
> }
> +
> /*
>  * This is a vanilla, non-pkey mprotect (or we failed to
>  * setup execute-only), inherit the pkey from the VMA we
> _