[PATCH 1/1 v2] x86: pkey-mprotect must allow pkey-0

[PATCH 1/1 v2] x86: 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 x86_64.

cc: Dave Hansen <dave.hansen@intel.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/x86/include/asm/pkeys.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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 1/1 v2] x86: pkey-mprotect must allow pkey-0

[Dave Hansen]

On 03/14/2018 12:46 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.

I'm trying to remember why we cared in the first place. :)

Could you add that to the changelog, please?

> @@ -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;

If an app was being really careful, couldn't it free up all of the
implicitly-pkey-0-assigned memory so that it is not in use at all?  In
that case, we might want to allow this.

On the other hand, nobody is likely to _ever_ actually do this so this
is good shoot-yourself-in-the-foot protection.

Re: [PATCH 1/1 v2] x86: pkey-mprotect must allow pkey-0

[Ram Pai]

On Wed, Mar 14, 2018 at 07:19:23AM -0700, Dave Hansen wrote:
> On 03/14/2018 12:46 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.
> 
> I'm trying to remember why we cared in the first place. :)
> 
> Could you add that to the changelog, please?
> 
> > @@ -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;
> 
> If an app was being really careful, couldn't it free up all of the
> implicitly-pkey-0-assigned memory so that it is not in use at all?  In
> that case, we might want to allow this.
> 
> On the other hand, nobody is likely to _ever_ actually do this so this
> is good shoot-yourself-in-the-foot protection.

I look at key-0 as 'the key'. It has special status. 
(a) It always exist.
(b) it cannot be freed.
(c) it is assigned by default.
(d) its permissions cannot be modified.
(e) it bypasses key-permission checks when assigned.

An arch need not necessarily map 'the key-0' to its key-0.  It could
internally map it to any of its internal key of its choice, transparent
to the application.

Do you see a problem to this view point?

RP

Re: [PATCH 1/1 v2] x86: pkey-mprotect must allow pkey-0

[Dave Hansen]

On 03/14/2018 10:14 AM, Ram Pai wrote:
> I look at key-0 as 'the key'. It has special status. 
> (a) It always exist.

Do you mean "is always allocated"?

> (b) it cannot be freed.

This is the one I'm questioning.

> (c) it is assigned by default.

I agree on this totally. :)

> (d) its permissions cannot be modified.

Why not?  You could pretty easily get a thread going that had its stack
covered with another pkey and that was being very careful what it
accesses.  It could pretty easily set pkey-0's access or write-disable bits.

> (e) it bypasses key-permission checks when assigned.

I don't think this is necessary.  I think the only rule we *need* is:

	pkey-0 is allocated implicitly at execve() time.  You do not
	need to call pkey_alloc() to allocate it.

> An arch need not necessarily map 'the key-0' to its key-0.  It could
> internally map it to any of its internal key of its choice, transparent
> to the application.

I don't understand what you are saying here.

Re: [PATCH 1/1 v2] x86: pkey-mprotect must allow pkey-0

[Ram Pai]

On Wed, Mar 14, 2018 at 10:51:26AM -0700, Dave Hansen wrote:
> On 03/14/2018 10:14 AM, Ram Pai wrote:
> > I look at key-0 as 'the key'. It has special status. 
> > (a) It always exist.
> 
> Do you mean "is always allocated"?

always allocated and cannot be freed. Hence always exists.

If we let it freed, than yes 'it is always allocated', but
may not 'always exist'.

> 
> > (b) it cannot be freed.
> 
> This is the one I'm questioning.

this is a philosophical question. Should we allow the application 
shoot-its-own-feet or help it from doing so. I tend to
gravitate towards the later.

> 
> > (c) it is assigned by default.
> 
> I agree on this totally. :)

good. we have some common ground :)

> 
> > (d) its permissions cannot be modified.
> 
> Why not?  You could pretty easily get a thread going that had its stack
> covered with another pkey and that was being very careful what it
> accesses.  It could pretty easily set pkey-0's access or write-disable bits.

ok. I see your point. Will not argue against it.

> 
> > (e) it bypasses key-permission checks when assigned.
> 
> I don't think this is necessary.  I think the only rule we *need* is:
> 
> 	pkey-0 is allocated implicitly at execve() time.  You do not
> 	need to call pkey_alloc() to allocate it.

And can be explicitly associated with any address range ?

> 
> > An arch need not necessarily map 'the key-0' to its key-0.  It could
> > internally map it to any of its internal key of its choice, transparent
> > to the application.
> 
> I don't understand what you are saying here.

I was trying to disassociate the notion that "application's key-0 
means hardware's key-0". Nevermind. its not important for this
discussion.

-- 
Ram Pai

Re: [PATCH 1/1 v2] x86: pkey-mprotect must allow pkey-0

[Dave Hansen]

On 03/14/2018 11:54 AM, Ram Pai wrote:
>>> (e) it bypasses key-permission checks when assigned.
>> I don't think this is necessary.  I think the only rule we *need* is:
>>
>> 	pkey-0 is allocated implicitly at execve() time.  You do not
>> 	need to call pkey_alloc() to allocate it.
> And can be explicitly associated with any address range ?

Yes, it should ideally be available for use just like any other key when
allocated.