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

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

[Dave Hansen]

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.

Signed-off-by: Dave Hansen 
Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys 
support")
Reported-by: Shakeel Butt 
Cc: sta...@vger.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  |   21 +++--
 2 files changed, 22 insertions(+), 11 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-04-26 10:42:18.971487371 -0700
+++ b/arch/x86/include/asm/pkeys.h  2018-04-26 10:42:18.977487371 -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-04-26 10:42:18.973487371 -0700
+++ b/arch/x86/mm/pkeys.c   2018-04-26 10:47:34.806486584 -0700
@@ -94,26 +94,27 @@ 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,
 * fall through as if we do not have execute-only
-* support.
+* support in this mm.
 */
if (prot == PROT_EXEC) {
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
_

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

[Dave Hansen]

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.

Signed-off-by: Dave Hansen 
Fixes: 62b5f7d013f ("mm/core, x86/mm/pkeys: Add execute-only protection keys 
support")
Reported-by: Shakeel Butt 
Cc: sta...@vger.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  |   21 +++--
 2 files changed, 22 insertions(+), 11 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-04-26 10:42:18.971487371 -0700
+++ b/arch/x86/include/asm/pkeys.h  2018-04-26 10:42:18.977487371 -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-04-26 10:42:18.973487371 -0700
+++ b/arch/x86/mm/pkeys.c   2018-04-26 10:47:34.806486584 -0700
@@ -94,26 +94,27 @@ 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,
 * fall through as if we do not have execute-only
-* support.
+* support in this mm.
 */
if (prot == PROT_EXEC) {
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

[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

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

[Dave Hansen]

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.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;
-   }
+
/*
 * 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
_

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

[Dave Hansen]

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.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;
-   }
+
/*
 * 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

[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
> _

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

[Dave Hansen]

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 
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
_

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

[Dave Hansen]

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 
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
_