Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Dave Hansen]

On 03/14/2018 01:00 AM, Florian Weimer wrote:
> ... but not the key which is used for PROT_EXEC emulation, which is still
> reserved

The PROT_EXEC key is dynamically allocated.  There is no "the key".

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Florian Weimer]

On 03/14/2018 09:00 AM, Florian Weimer wrote:

On 03/09/2018 09:00 PM, Ram Pai wrote:

On Fri, Mar 09, 2018 at 12:04:49PM +0100, Florian Weimer wrote:

On 03/09/2018 09:12 AM, Ram Pai wrote:
Once an address range is associated with an allocated pkey, it 
cannot be
reverted back to key-0. There is no valid reason for the above 
behavior.


mprotect without a key does not necessarily use key 0, e.g. if
protection keys are used to emulate page protection flag combination
which is not directly supported by the hardware.

Therefore, it seems to me that filtering out non-allocated keys is
the right thing to do.


I am not sure, what you mean. Do you agree with the patch or otherwise?


I think it's inconsistent to make key 0 allocated, but not the key which 
is used for PROT_EXEC emulation, which is still reserved.  Even if you 
change the key 0 behavior, it is still not possible to emulate mprotect 
behavior faithfully with an allocated key.


Ugh.  Should have read the code first before replying:

/* Do we need to assign a pkey for mm's execute-only maps? */
if (execute_only_pkey == -1) {
/* Go allocate one to use, which might fail */
execute_only_pkey = mm_pkey_alloc(mm);
if (execute_only_pkey < 0)
return -1;
need_to_set_mm_pkey = true;
}

So we do allocate the PROT_EXEC-only key, and I assume it means that the 
key can be restored using pkey_mprotect.  So the key 0 behavior is a 
true exception after all, and it makes sense to realign the behavior 
with the other keys.


Thanks,
Florian

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Florian Weimer]

On 03/09/2018 09:00 PM, Ram Pai wrote:

On Fri, Mar 09, 2018 at 12:04:49PM +0100, Florian Weimer wrote:

On 03/09/2018 09:12 AM, Ram Pai wrote:

Once an address range is associated with an allocated pkey, it cannot be
reverted back to key-0. There is no valid reason for the above behavior.


mprotect without a key does not necessarily use key 0, e.g. if
protection keys are used to emulate page protection flag combination
which is not directly supported by the hardware.

Therefore, it seems to me that filtering out non-allocated keys is
the right thing to do.


I am not sure, what you mean. Do you agree with the patch or otherwise?


I think it's inconsistent to make key 0 allocated, but not the key which 
is used for PROT_EXEC emulation, which is still reserved.  Even if you 
change the key 0 behavior, it is still not possible to emulate mprotect 
behavior faithfully with an allocated key.


Thanks,
Florian

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Dave Hansen]

On 03/09/2018 12:06 PM, Ram Pai wrote:
> On Fri, Mar 09, 2018 at 09:19:53PM +1100, Michael Ellerman wrote:
>> Ram Pai  writes:
>>
>>> Once an address range is associated with an allocated pkey, it cannot be
>>> reverted back to key-0. There is no valid reason for the above behavior.  On
>>> the contrary applications need the ability to do so.
>> Please explain this in much more detail. Is it an ABI change?
> Not necessarily an ABI change. older binary applications  will continue
> to work. It can be considered as a bug-fix.

Yeah, agreed.  I do not think this is an ABI change.

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Dave Hansen]

On 03/09/2018 09:55 PM, Ram Pai wrote:
> On Fri, Mar 09, 2018 at 02:40:32PM -0800, Dave Hansen wrote:
>> On 03/09/2018 12:12 AM, Ram Pai wrote:
>>> Once an address range is associated with an allocated pkey, it cannot be
>>> reverted back to key-0. There is no valid reason for the above behavior.  On
>>> the contrary applications need the ability to do so.
>> Why don't we just set pkey 0 to be allocated in the allocation bitmap by
>> default?
> ok. that will make it allocatable. But it will not be associatable,
> given the bug in the current code. And what will be the
> default key associated with a pte? zero? or something else?

I'm just saying that I think we should try to keep from making it
special as much as possible.

Let's fix the bug that keeps it from being associatable.

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Ram Pai]

On Fri, Mar 09, 2018 at 02:40:32PM -0800, Dave Hansen wrote:
> On 03/09/2018 12:12 AM, Ram Pai wrote:
> > Once an address range is associated with an allocated pkey, it cannot be
> > reverted back to key-0. There is no valid reason for the above behavior.  On
> > the contrary applications need the ability to do so.
> 
> Why don't we just set pkey 0 to be allocated in the allocation bitmap by
> default?

ok. that will make it allocatable. But it will not be associatable,
given the bug in the current code. And what will be the
default key associated with a pte? zero? or something else?

> 
> We *could* also just not let it be special and let it be freed.  An app
> could theoretically be careful and make sure nothing is using it.

unable to see how this solves the problem. Need some more explaination.


RP

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Dave Hansen]

On 03/09/2018 12:12 AM, Ram Pai wrote:
> Once an address range is associated with an allocated pkey, it cannot be
> reverted back to key-0. There is no valid reason for the above behavior.  On
> the contrary applications need the ability to do so.

Why don't we just set pkey 0 to be allocated in the allocation bitmap by
default?

We *could* also just not let it be special and let it be freed.  An app
could theoretically be careful and make sure nothing is using it.

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Ram Pai]

On Fri, Mar 09, 2018 at 09:43:32AM +0100, Ingo Molnar wrote:
> 
> * Ram Pai  wrote:
> 
> > Once an address range is associated with an allocated pkey, it cannot be
> > reverted back to key-0. There is no valid reason for the above behavior.  On
> > the contrary applications need the ability to do so.
> > 
> > The patch relaxes the restriction.
> > 
> > Tested on powerpc and x86_64.
> > 
> > cc: Dave Hansen 
> > cc: Michael Ellermen 
> > cc: Ingo Molnar 
> > Signed-off-by: Ram Pai 
> > ---
> >  arch/powerpc/include/asm/pkeys.h | 19 ++-
> >  arch/x86/include/asm/pkeys.h |  5 +++--
> >  2 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/pkeys.h 
> > b/arch/powerpc/include/asm/pkeys.h
> > index 0409c80..3e8abe4 100644
> > --- a/arch/powerpc/include/asm/pkeys.h
> > +++ b/arch/powerpc/include/asm/pkeys.h
> > @@ -101,10 +101,18 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
> >  
> >  static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
> >  {
> > -   /* A reserved key is never considered as 'explicitly allocated' */
> > -   return ((pkey < arch_max_pkey()) &&
> > -   !__mm_pkey_is_reserved(pkey) &&
> > -   __mm_pkey_is_allocated(mm, pkey));
> > +   /* pkey 0 is allocated by default. */
> > +   if (!pkey)
> > +  return true;
> > +
> > +   if (pkey < 0 || pkey >= arch_max_pkey())
> > +  return false;
> > +
> > +   /* reserved keys are never allocated. */
> > +   if (__mm_pkey_is_reserved(pkey))
> > +  return false;
> 
> Please capitalize in comments consistently, i.e.:

ok.

> 
>   /* Reserved keys are never allocated: */
> 
> > +
> > +   return(__mm_pkey_is_allocated(mm, pkey));
> 
> 'return' is not a function.

right. will fix.

Thanks,
RP

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Ram Pai]

On Fri, Mar 09, 2018 at 09:19:53PM +1100, Michael Ellerman wrote:
> Ram Pai  writes:
> 
> > Once an address range is associated with an allocated pkey, it cannot be
> > reverted back to key-0. There is no valid reason for the above behavior.  On
> > the contrary applications need the ability to do so.
> 
> Please explain this in much more detail. Is it an ABI change?

Not necessarily an ABI change. older binary applications  will continue
to work. It can be considered as a bug-fix.

> 
> And why did we just notice this?

Yes. this was noticed by an application vendor.

> 
> > The patch relaxes the restriction.
> >
> > Tested on powerpc and x86_64.
> 
> Thanks, but please split the patch, one for each arch.

Will do.
RP

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Ram Pai]

On Fri, Mar 09, 2018 at 12:04:49PM +0100, Florian Weimer wrote:
> On 03/09/2018 09:12 AM, Ram Pai wrote:
> >Once an address range is associated with an allocated pkey, it cannot be
> >reverted back to key-0. There is no valid reason for the above behavior.
> 
> mprotect without a key does not necessarily use key 0, e.g. if
> protection keys are used to emulate page protection flag combination
> which is not directly supported by the hardware.
> 
> Therefore, it seems to me that filtering out non-allocated keys is
> the right thing to do.

I am not sure, what you mean. Do you agree with the patch or otherwise?
RP

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Ram Pai]

On Fri, Mar 09, 2018 at 07:37:04PM +1100, Balbir Singh wrote:
> On Fri, Mar 9, 2018 at 7:12 PM, Ram Pai  wrote:
> > Once an address range is associated with an allocated pkey, it cannot be
> > reverted back to key-0. There is no valid reason for the above behavior.  On
> > the contrary applications need the ability to do so.
> >
> > The patch relaxes the restriction.
> 
> I looked at the code and my observation was going to be that we need
> to change mm_pkey_is_allocated. I still fail to understand what
> happens if pkey 0 is reserved? What is the default key is it the first
> available key? Assuming 0 is the default key may work and seems to
> work, but I am sure its mostly by accident. It would be nice, if we
> could have  a notion of the default key. I don't like the special
> meaning given to key 0 here. Remember on powerpc if 0 is reserved and
> UAMOR/AMOR does not allow modification because it's reserved, setting
> 0 will still fail

The linux pkey API, assumes pkey-0 is the default key. If no key is
explicitly associated with a page, the default key gets associated.
When a default key gets associated with a page, the permissions on the
page are not dictated by the permissions of the default key, but by the
permission of other bits in the pte; i.e _PAGE_RWX.

On powerpc, and AFAICT on x86, neither the hardware nor the hypervisor
reserves key-0. Hence the OS is free to use the key value, the
way it chooses. On Linux we choose to associate key-0 the special status
called default-key.

However I see your point. If some cpu architecture takes away key-0 from
Linux, than implementing the special status for key-0 on that
architecture can become challenging, though not impossible. That
architecture implementation can internally map key-0 value to some other
available key, and associate that key to the page. And offcourse make
sure that the hardware/MMU uses the pte's RWX bits to enforce
permissions, for that key.


-- 
Ram Pai

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Florian Weimer]

On 03/09/2018 09:12 AM, Ram Pai wrote:

Once an address range is associated with an allocated pkey, it cannot be
reverted back to key-0. There is no valid reason for the above behavior.


mprotect without a key does not necessarily use key 0, e.g. if 
protection keys are used to emulate page protection flag combination 
which is not directly supported by the hardware.


Therefore, it seems to me that filtering out non-allocated keys is the 
right thing to do.


Thanks,
Florian

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Michael Ellerman]

Ram Pai  writes:

> Once an address range is associated with an allocated pkey, it cannot be
> reverted back to key-0. There is no valid reason for the above behavior.  On
> the contrary applications need the ability to do so.

Please explain this in much more detail. Is it an ABI change?

And why did we just notice this?

> The patch relaxes the restriction.
>
> Tested on powerpc and x86_64.

Thanks, but please split the patch, one for each arch.

cheers

> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 0409c80..3e8abe4 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -101,10 +101,18 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
>  
>  static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>  {
> - /* A reserved key is never considered as 'explicitly allocated' */
> - return ((pkey < arch_max_pkey()) &&
> - !__mm_pkey_is_reserved(pkey) &&
> - __mm_pkey_is_allocated(mm, pkey));
> + /* pkey 0 is allocated by default. */
> + if (!pkey)
> +return true;
> +
> + if (pkey < 0 || pkey >= arch_max_pkey())
> +return false;
> +
> + /* reserved keys are never allocated. */
> + if (__mm_pkey_is_reserved(pkey))
> +return false;
> +
> + return(__mm_pkey_is_allocated(mm, pkey));
>  }
>  
>  extern void __arch_activate_pkey(int pkey);
> @@ -150,7 +158,8 @@ static inline int mm_pkey_free(struct mm_struct *mm, int 
> pkey)
>   if (static_branch_likely(_disabled))
>   return -1;
>  
> - if (!mm_pkey_is_allocated(mm, pkey))
> + /* pkey 0 cannot be freed */
> + if (!pkey || !mm_pkey_is_allocated(mm, pkey))
>   return -EINVAL;
>  
>   /*
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index a0ba1ff..6ea7486 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -52,7 +52,7 @@ bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>* from pkey_alloc().  pkey 0 is special, and never
>* returned from pkey_alloc().
>*/
> - if (pkey <= 0)
> + if (pkey < 0)
>   return false;
>   if (pkey >= arch_max_pkey())
>   return false;
> @@ -92,7 +92,8 @@ int mm_pkey_alloc(struct mm_struct *mm)
>  static inline
>  int mm_pkey_free(struct mm_struct *mm, int pkey)
>  {
> - if (!mm_pkey_is_allocated(mm, pkey))
> + /* pkey 0 is special and can never be freed */
> + if (!pkey || !mm_pkey_is_allocated(mm, pkey))
>   return -EINVAL;
>  
>   mm_set_pkey_free(mm, pkey);
> -- 
> 1.8.3.1

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Ingo Molnar]

* Ram Pai  wrote:

> Once an address range is associated with an allocated pkey, it cannot be
> reverted back to key-0. There is no valid reason for the above behavior.  On
> the contrary applications need the ability to do so.
> 
> The patch relaxes the restriction.
> 
> Tested on powerpc and x86_64.
> 
> cc: Dave Hansen 
> cc: Michael Ellermen 
> cc: Ingo Molnar 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/pkeys.h | 19 ++-
>  arch/x86/include/asm/pkeys.h |  5 +++--
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 0409c80..3e8abe4 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -101,10 +101,18 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
>  
>  static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>  {
> - /* A reserved key is never considered as 'explicitly allocated' */
> - return ((pkey < arch_max_pkey()) &&
> - !__mm_pkey_is_reserved(pkey) &&
> - __mm_pkey_is_allocated(mm, pkey));
> + /* pkey 0 is allocated by default. */
> + if (!pkey)
> +return true;
> +
> + if (pkey < 0 || pkey >= arch_max_pkey())
> +return false;
> +
> + /* reserved keys are never allocated. */
> + if (__mm_pkey_is_reserved(pkey))
> +return false;

Please capitalize in comments consistently, i.e.:

/* Reserved keys are never allocated: */

> +
> + return(__mm_pkey_is_allocated(mm, pkey));

'return' is not a function.

Thanks,

Ingo

Re: [PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Balbir Singh]

On Fri, Mar 9, 2018 at 7:12 PM, Ram Pai  wrote:
> Once an address range is associated with an allocated pkey, it cannot be
> reverted back to key-0. There is no valid reason for the above behavior.  On
> the contrary applications need the ability to do so.
>
> The patch relaxes the restriction.

I looked at the code and my observation was going to be that we need
to change mm_pkey_is_allocated. I still fail to understand what
happens if pkey 0 is reserved? What is the default key is it the first
available key? Assuming 0 is the default key may work and seems to
work, but I am sure its mostly by accident. It would be nice, if we
could have  a notion of the default key. I don't like the special
meaning given to key 0 here. Remember on powerpc if 0 is reserved and
UAMOR/AMOR does not allow modification because it's reserved, setting
0 will still fail

Balbir

[PATCH] x86, powerpc : pkey-mprotect must allow pkey-0

[Ram Pai]

Once an address range is associated with an allocated pkey, it cannot be
reverted back to key-0. There is no valid reason for the above behavior.  On
the contrary applications need the ability to do so.

The patch relaxes the restriction.

Tested on powerpc and x86_64.

cc: Dave Hansen 
cc: Michael Ellermen 
cc: Ingo Molnar 
Signed-off-by: Ram Pai 
---
 arch/powerpc/include/asm/pkeys.h | 19 ++-
 arch/x86/include/asm/pkeys.h |  5 +++--
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 0409c80..3e8abe4 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -101,10 +101,18 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
 
 static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
-   /* A reserved key is never considered as 'explicitly allocated' */
-   return ((pkey < arch_max_pkey()) &&
-   !__mm_pkey_is_reserved(pkey) &&
-   __mm_pkey_is_allocated(mm, pkey));
+   /* pkey 0 is allocated by default. */
+   if (!pkey)
+  return true;
+
+   if (pkey < 0 || pkey >= arch_max_pkey())
+  return false;
+
+   /* reserved keys are never allocated. */
+   if (__mm_pkey_is_reserved(pkey))
+  return false;
+
+   return(__mm_pkey_is_allocated(mm, pkey));
 }
 
 extern void __arch_activate_pkey(int pkey);
@@ -150,7 +158,8 @@ static inline int mm_pkey_free(struct mm_struct *mm, int 
pkey)
if (static_branch_likely(_disabled))
return -1;
 
-   if (!mm_pkey_is_allocated(mm, pkey))
+   /* pkey 0 cannot be freed */
+   if (!pkey || !mm_pkey_is_allocated(mm, pkey))
return -EINVAL;
 
/*
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index a0ba1ff..6ea7486 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -52,7 +52,7 @@ bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 * from pkey_alloc().  pkey 0 is special, and never
 * returned from pkey_alloc().
 */
-   if (pkey <= 0)
+   if (pkey < 0)
return false;
if (pkey >= arch_max_pkey())
return false;
@@ -92,7 +92,8 @@ int mm_pkey_alloc(struct mm_struct *mm)
 static inline
 int mm_pkey_free(struct mm_struct *mm, int pkey)
 {
-   if (!mm_pkey_is_allocated(mm, pkey))
+   /* pkey 0 is special and can never be freed */
+   if (!pkey || !mm_pkey_is_allocated(mm, pkey))
return -EINVAL;
 
mm_set_pkey_free(mm, pkey);
-- 
1.8.3.1