[PATCH v3] powerpc, pkey: make protection key 0 less special

[PATCH v3] powerpc, pkey: make protection key 0 less special

[Ram Pai]

Applications need the ability to associate an address-range with some
key and latter revert to its initial default key. Pkey-0 comes close to
providing this function but falls short, because the current
implementation disallows applications to explicitly associate pkey-0 to
the address range.

Lets make pkey-0 less special and treat it almost like any other key.
Thus it can be explicitly associated with any address range, and can be
freed. This gives the application more flexibility and power.  The
ability to free pkey-0 must be used responsibily, since pkey-0 is
associated with almost all address-range by default.

Even with this change pkey-0 continues to be slightly more special
from the following point of view.
(a) it is implicitly allocated.
(b) it is the default key assigned to any address-range.
(c) its permissions cannot be modified by userspace.

NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
with all pages including kernel pages, and pkeys are also active in
kernel mode. If any permission is denied on pkey-0, the kernel running
in the context of the application will be unable to operate.

Tested on powerpc.

cc: Thomas Gleixner <tglx@linutronix.de>
cc: Dave Hansen <dave.hansen@intel.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Ingo Molnar <mingo@kernel.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
History:

	v3: . Corrected a comment in arch_set_user_pkey_access().
	    . Clarified the header, to capture the notion that
	      pkey-0 permissions cannot be modified by userspace on powerpc.
		-- comment from Thiago

	v2: . mm_pkey_is_allocated() continued to treat pkey-0 special.
	            fixed it.

 arch/powerpc/include/asm/pkeys.h |   22 ++++++++++++++++++----
 arch/powerpc/mm/pkeys.c          |   26 +++++++++++++++-----------
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 0409c80..31a6976 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -101,10 +101,14 @@ 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));
+	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);
@@ -200,6 +204,16 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 {
 	if (static_branch_likely(&pkey_disabled))
 		return -EINVAL;
+
+	/*
+	 * userspace should not change pkey-0 permissions.
+	 * pkey-0 is associated with every page in the kernel.
+	 * If userspace denies any permission on pkey-0, the
+	 * kernel cannot operate.
+	 */
+	if (!pkey)
+		return init_val ? -EINVAL : 0;
+
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }
 
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 0eafdf0..d6873b4 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -119,16 +119,21 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
+	/* Bits are in LE format. */
 	initial_allocation_mask = ~0x0;
+
+	/* register mask is in BE format */
 	pkey_amr_uamor_mask = ~0x0ul;
 	pkey_iamr_mask = ~0x0ul;
-	/*
-	 * key 0, 1 are reserved.
-	 * key 0 is the default key, which allows read/write/execute.
-	 * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
-	 * programming note.
-	 */
-	for (i = 2; i < (pkeys_total - os_reserved); i++) {
+
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+		/*
+		 * key 1 is recommended not to be used.
+		 * PowerISA(3.0) page 1015,
+		 */
+		if (i == 1)
+			continue;
+
 		initial_allocation_mask &= ~(0x1 << i);
 		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
@@ -142,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
 {
 	if (static_branch_likely(&pkey_disabled))
 		return;
-	mm_pkey_allocation_map(mm) = initial_allocation_mask;
+
+	/* allocate key-0 by default */
+	mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
 	/* -1 means unallocated or invalid */
 	mm->context.execute_only_pkey = -1;
 }
@@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
 	int pkey_shift;
 	u64 amr;
 
-	if (!pkey)
-		return true;
-
 	if (!is_pkey_enabled(pkey))
 		return true;
 
-- 
1.7.1

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Dave Hansen]

On 05/04/2018 12:22 PM, Ram Pai wrote:
> @@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
>  	int pkey_shift;
>  	u64 amr;
>  
> -	if (!pkey)
> -		return true;
> -
>  	if (!is_pkey_enabled(pkey))
>  		return true;

Looks fine to me.  Obviously doesn't have any impact on x86 or the
generic code.

One question, though.  Which other check makes up for this removed !pkey
check?

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Ram Pai]

On Fri, May 04, 2018 at 12:59:27PM -0700, Dave Hansen wrote:
> On 05/04/2018 12:22 PM, Ram Pai wrote:
> > @@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
> >  	int pkey_shift;
> >  	u64 amr;
> >  
> > -	if (!pkey)
> > -		return true;
> > -
> >  	if (!is_pkey_enabled(pkey))
> >  		return true;
> 
> Looks fine to me.  Obviously doesn't have any impact on x86 or the
> generic code.
> 
> One question, though.  Which other check makes up for this removed !pkey
> check?

is_pkey_enabled() does take care of it.  we do not enable userspace to
change permissions on pkey-0. This information is tracked in
UAMOR register.  is_pkey_enabled() refers to UAMOR to determine
if the given key is modifiable by userspace. since UAMOR has the bit
corresponding to key-0 set to 0, is_pkey_enabled(key-0) will return
false. 

The deleted code above, would have done the same job without
referring UAMOR. However having special checks on pkey-0 makes
pkey-0 special. It defeats the purpose of this patch; which is to make
pkey-0 less special :).


-- 
Ram Pai

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Michal Suchánek]

On Fri,  4 May 2018 12:22:58 -0700
"Ram Pai" <linuxram@us.ibm.com> wrote:

> Applications need the ability to associate an address-range with some
> key and latter revert to its initial default key. Pkey-0 comes close
> to providing this function but falls short, because the current
> implementation disallows applications to explicitly associate pkey-0
> to the address range.
> 
> Lets make pkey-0 less special and treat it almost like any other key.
> Thus it can be explicitly associated with any address range, and can
> be freed. This gives the application more flexibility and power.  The
> ability to free pkey-0 must be used responsibily, since pkey-0 is
> associated with almost all address-range by default.
> 
> Even with this change pkey-0 continues to be slightly more special
> from the following point of view.
> (a) it is implicitly allocated.
> (b) it is the default key assigned to any address-range.
> (c) its permissions cannot be modified by userspace.
> 
> NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
> with all pages including kernel pages, and pkeys are also active in
> kernel mode. If any permission is denied on pkey-0, the kernel running
> in the context of the application will be unable to operate.

If it is not ok to change permissions of pkey 0 is it ok to free it?

Thanks

Michal
> 
> Tested on powerpc.
> 
> cc: Thomas Gleixner <tglx@linutronix.de>
> cc: Dave Hansen <dave.hansen@intel.com>
> cc: Michael Ellermen <mpe@ellerman.id.au>
> cc: Ingo Molnar <mingo@kernel.org>
> cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> History:
> 
> 	v3: . Corrected a comment in arch_set_user_pkey_access().
> 	    . Clarified the header, to capture the notion that
> 	      pkey-0 permissions cannot be modified by userspace on
> powerpc. -- comment from Thiago
> 
> 	v2: . mm_pkey_is_allocated() continued to treat pkey-0
> special. fixed it.
> 
>  arch/powerpc/include/asm/pkeys.h |   22 ++++++++++++++++++----
>  arch/powerpc/mm/pkeys.c          |   26 +++++++++++++++-----------
>  2 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h
> b/arch/powerpc/include/asm/pkeys.h index 0409c80..31a6976 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -101,10 +101,14 @@ 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));
> +	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);
> @@ -200,6 +204,16 @@ static inline int
> arch_set_user_pkey_access(struct task_struct *tsk, int pkey, {
>  	if (static_branch_likely(&pkey_disabled))
>  		return -EINVAL;
> +
> +	/*
> +	 * userspace should not change pkey-0 permissions.
> +	 * pkey-0 is associated with every page in the kernel.
> +	 * If userspace denies any permission on pkey-0, the
> +	 * kernel cannot operate.
> +	 */
> +	if (!pkey)
> +		return init_val ? -EINVAL : 0;
> +
>  	return __arch_set_user_pkey_access(tsk, pkey, init_val);
>  }
>  
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 0eafdf0..d6873b4 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -119,16 +119,21 @@ int pkey_initialize(void)
>  #else
>  	os_reserved = 0;
>  #endif
> +	/* Bits are in LE format. */
>  	initial_allocation_mask = ~0x0;
> +
> +	/* register mask is in BE format */
>  	pkey_amr_uamor_mask = ~0x0ul;
>  	pkey_iamr_mask = ~0x0ul;
> -	/*
> -	 * key 0, 1 are reserved.
> -	 * key 0 is the default key, which allows read/write/execute.
> -	 * key 1 is recommended not to be used. PowerISA(3.0) page
> 1015,
> -	 * programming note.
> -	 */
> -	for (i = 2; i < (pkeys_total - os_reserved); i++) {
> +
> +	for (i = 0; i < (pkeys_total - os_reserved); i++) {
> +		/*
> +		 * key 1 is recommended not to be used.
> +		 * PowerISA(3.0) page 1015,
> +		 */
> +		if (i == 1)
> +			continue;
> +
>  		initial_allocation_mask &= ~(0x1 << i);
>  		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
>  		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
> @@ -142,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
>  {
>  	if (static_branch_likely(&pkey_disabled))
>  		return;
> -	mm_pkey_allocation_map(mm) = initial_allocation_mask;
> +
> +	/* allocate key-0 by default */
> +	mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
>  	/* -1 means unallocated or invalid */
>  	mm->context.execute_only_pkey = -1;
>  }
> @@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool
> write, bool execute) int pkey_shift;
>  	u64 amr;
>  
> -	if (!pkey)
> -		return true;
> -
>  	if (!is_pkey_enabled(pkey))
>  		return true;
>  

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Dave Hansen]

On 05/04/2018 02:26 PM, Michal Suchánek wrote:
> If it is not ok to change permissions of pkey 0 is it ok to free it?

It's pretty much never OK to free it on x86 or ppc.  But, we're not
going to put code in to keep userspace from shooting itself in the foot,
at least on x86.

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Ram Pai]

On Fri, May 04, 2018 at 02:31:10PM -0700, Dave Hansen wrote:
> On 05/04/2018 02:26 PM, Michal Suchánek wrote:
> > If it is not ok to change permissions of pkey 0 is it ok to free it?
> 
> It's pretty much never OK to free it on x86 or ppc.  But, we're not
> going to put code in to keep userspace from shooting itself in the foot,
> at least on x86.

and on powerpc aswell.


-- 
Ram Pai

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Michal Suchánek]

On Fri, 4 May 2018 14:45:07 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> On Fri, May 04, 2018 at 02:31:10PM -0700, Dave Hansen wrote:
> > On 05/04/2018 02:26 PM, Michal Such=C3=A1nek wrote: =20
> > > If it is not ok to change permissions of pkey 0 is it ok to free
> > > it? =20
> >=20
> > It's pretty much never OK to free it on x86 or ppc.  But, we're not
> > going to put code in to keep userspace from shooting itself in the
> > foot, at least on x86. =20
>=20
> and on powerpc aswell.

But once it's free it can be re-allocated. So you are moving the
special-casing from free code to code dealing with allocation.

If you want something like allocate_exec_only_pkey then the function
(either in kernel or in userspace) needs to make sure it is not
getting/requesting key 0 on powerpc.

Thanks

Michal

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Ram Pai]

On Sat, May 05, 2018 at 02:39:56PM +0200, Michal Suchánek wrote:
> On Fri, 4 May 2018 14:45:07 -0700
> Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > On Fri, May 04, 2018 at 02:31:10PM -0700, Dave Hansen wrote:
> > > On 05/04/2018 02:26 PM, Michal Suchánek wrote:  
> > > > If it is not ok to change permissions of pkey 0 is it ok to free
> > > > it?  
> > > 
> > > It's pretty much never OK to free it on x86 or ppc.  But, we're not
> > > going to put code in to keep userspace from shooting itself in the
> > > foot, at least on x86.  
> > 
> > and on powerpc aswell.
> 
> But once it's free it can be re-allocated. So you are moving the
> special-casing from free code to code dealing with allocation.

Actually if an application frees key-0, it has potentially opened up a
can-of-worms. It could step on anything that explodes. 

Its choice between imposing policies on an application v/s freeing it up
to choose its own policy. I think the kernel should impose some form of
mild-policy. But others think there should be none.

> 
> If you want something like allocate_exec_only_pkey then the function
> (either in kernel or in userspace) needs to make sure it is not
> getting/requesting key 0 on powerpc.

Yes. makes sense. I will put in some checks towards that.


Thanks,
RP

> 
> Thanks
> 
> Michal

-- 
Ram Pai

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Michal Suchánek]

On Sun, 6 May 2018 13:10:43 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> On Sat, May 05, 2018 at 02:39:56PM +0200, Michal Such=C3=A1nek wrote:
> > On Fri, 4 May 2018 14:45:07 -0700
> > Ram Pai <linuxram@us.ibm.com> wrote:
> >  =20
> > > On Fri, May 04, 2018 at 02:31:10PM -0700, Dave Hansen wrote: =20
> > > > On 05/04/2018 02:26 PM, Michal Such=C3=A1nek wrote:   =20
> > > > > If it is not ok to change permissions of pkey 0 is it ok to
> > > > > free it?   =20
> > > >=20
> > > > It's pretty much never OK to free it on x86 or ppc.  But, we're
> > > > not going to put code in to keep userspace from shooting itself
> > > > in the foot, at least on x86.   =20
> > >=20
> > > and on powerpc aswell. =20
> >=20
> > But once it's free it can be re-allocated. So you are moving the
> > special-casing from free code to code dealing with allocation. =20
>=20
> Actually if an application frees key-0, it has potentially opened up a
> can-of-worms. It could step on anything that explodes.=20
>=20
> Its choice between imposing policies on an application v/s freeing it
> up to choose its own policy. I think the kernel should impose some
> form of mild-policy. But others think there should be none.

It is a problem of the application when it frees a key it still needs.
I am not for or against allowing that. The problem is with the
interface change once the key is freed.

>=20
> >=20
> > If you want something like allocate_exec_only_pkey then the function
> > (either in kernel or in userspace) needs to make sure it is not
> > getting/requesting key 0 on powerpc. =20
>=20
> Yes. makes sense. I will put in some checks towards that.

Suppose an application is adapted to take advantage of freeing key
0, perhaps to revoke access to any code and data used at runtime
initialization which is not longer needed.=20

As I understand it with the proposed change any address range not
associated with non-default key becomes inaccessible and key 0 becomes
available for allocation again. This is fine on x86 where key 0 is
fully functional.=20

However, on powerpc the application now has key 0 available and it is
not fully a fully functional key. So the application now needs to check
that the key it is allocating is not key 0. Otherwise further key
operations may unexpectedly fail.

To prevent this I would suggest

a) do not allow allocating key 0. If it is freed it becomes reserved

b) never expose key 0 to applications. Allocate key 2 as the default
key and present the key numbers with an offset to userspace so key 2
appears as key 0 to user applications.

Thanks

Michal

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Ram Pai]

On Mon, May 07, 2018 at 01:21:49PM +0200, Michal Suchánek wrote:
> On Sun, 6 May 2018 13:10:43 -0700
> Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > On Sat, May 05, 2018 at 02:39:56PM +0200, Michal Suchánek wrote:
> > > On Fri, 4 May 2018 14:45:07 -0700
> > > Ram Pai <linuxram@us.ibm.com> wrote:
> > >   
> > > > On Fri, May 04, 2018 at 02:31:10PM -0700, Dave Hansen wrote:  
> > > > > On 05/04/2018 02:26 PM, Michal Suchánek wrote:    
> > > > > > If it is not ok to change permissions of pkey 0 is it ok to
> > > > > > free it?    
> > > > > 
> > > > > It's pretty much never OK to free it on x86 or ppc.  But, we're
> > > > > not going to put code in to keep userspace from shooting itself
> > > > > in the foot, at least on x86.    
> > > > 
> > > > and on powerpc aswell.  
> > > 
> > > But once it's free it can be re-allocated. So you are moving the
> > > special-casing from free code to code dealing with allocation.  
> > 
> > Actually if an application frees key-0, it has potentially opened up a
> > can-of-worms. It could step on anything that explodes. 
> > 
> > Its choice between imposing policies on an application v/s freeing it
> > up to choose its own policy. I think the kernel should impose some
> > form of mild-policy. But others think there should be none.
> 
> It is a problem of the application when it frees a key it still needs.
> I am not for or against allowing that. The problem is with the
> interface change once the key is freed.
> 
> > 
> > > 
> > > If you want something like allocate_exec_only_pkey then the function
> > > (either in kernel or in userspace) needs to make sure it is not
> > > getting/requesting key 0 on powerpc.  
> > 
> > Yes. makes sense. I will put in some checks towards that.
> 
> Suppose an application is adapted to take advantage of freeing key
> 0, perhaps to revoke access to any code and data used at runtime
> initialization which is not longer needed. 
> 
> As I understand it with the proposed change any address range not
> associated with non-default key becomes inaccessible and key 0 becomes
> available for allocation again.

The above sentence is difficult to parse. I think what you are saying is --
	Any address range associated with key-0 become inaccessible if key-0
	is freed.  Is that a correct simplification of the above
	statement?

Assuming I got it right --  

	Yes. key-0 when freed becomes available for allocation again.
	No. When key-0 is freed any address range
associated with key-0 will continue to be accessible.  It was accessible
to begin with and will continue to be accessible.  It was accessible,
   because kernel; during task-creation, never disables any permissions on
   key-0, and also never lets userspace disable any permissions on
   key-0.

The problem arises, when key-0 gets reallocated at a later time. If the
key-0 at that point(during reallocation) is treated like any other key;
allowing userspace to change its permissions, it can explode on
access to any page associated with key-0. (BTW: almost everything is
associated with key-0 by default).

> This is fine on x86 where key 0 is fully functional. 

> 
> However, on powerpc the application now has key 0 available and it is
> not fully a fully functional key. So the application now needs to check
> that the key it is allocating is not key 0. Otherwise further key
> operations may unexpectedly fail.
> 
> To prevent this I would suggest
> 
> a) do not allow allocating key 0. If it is freed it becomes reserved

Certainly yes. We never promised any particular key to the user space. So
not returning key-0 during allocation should break no semantics.

> 
> b) never expose key 0 to applications. Allocate key 2 as the default
> key and present the key numbers with an offset to userspace so key 2
> appears as key 0 to user applications.

this is complicated. offseting the number is not easy. if kernel
has allocated key-2 and tells the user space to use key-0, userspace
will use bit-0 and bit-1 of the AMR register to program the permissions
on the key, and cpu will start applying those changes on key-0.
Basically kernel, userspace and cpu will be out-of-sync. This is
possible if the userspace uses a shift-aware library function to program
the AMR. BTW: A library function integrated into glibc is yet to be
defined AFAICK.


> 
> Thanks
> 
> Michal

-- 
Ram Pai

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Michal Suchánek]

On Tue, 8 May 2018 09:38:01 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> On Mon, May 07, 2018 at 01:21:49PM +0200, Michal Such=C3=A1nek wrote:
> > On Sun, 6 May 2018 13:10:43 -0700
> > Ram Pai <linuxram@us.ibm.com> wrote:
> >  =20
> > > On Sat, May 05, 2018 at 02:39:56PM +0200, Michal Such=C3=A1nek wrote:=
 =20
> > > > On Fri, 4 May 2018 14:45:07 -0700
> > > > Ram Pai <linuxram@us.ibm.com> wrote:
...
> > Suppose an application is adapted to take advantage of freeing key
> > 0, perhaps to revoke access to any code and data used at runtime
> > initialization which is not longer needed.=20
> >=20
> > As I understand it with the proposed change any address range not
> > associated with non-default key becomes inaccessible and key 0
> > becomes available for allocation again. =20
>=20
> The above sentence is difficult to parse. I think what you are saying
> is -- Any address range associated with key-0 become inaccessible if
> key-0 is freed.  Is that a correct simplification of the above
> 	statement?
>=20
> Assuming I got it right -- =20
>=20
> 	Yes. key-0 when freed becomes available for allocation again.
> 	No. When key-0 is freed any address range
> associated with key-0 will continue to be accessible.  It was
> accessible to begin with and will continue to be accessible.  It was
> accessible, because kernel; during task-creation, never disables any
> permissions on key-0, and also never lets userspace disable any
> permissions on key-0.

Oh, right. The result of freeing a used key is not defined. When it is
not defined expecting a sane result is too much.

>=20
> The problem arises, when key-0 gets reallocated at a later time. If
> the key-0 at that point(during reallocation) is treated like any
> other key; allowing userspace to change its permissions, it can
> explode on access to any page associated with key-0. (BTW: almost
> everything is associated with key-0 by default).
>=20
> > This is fine on x86 where key 0 is fully functional.  =20
>=20
> >=20
> > However, on powerpc the application now has key 0 available and it
> > is not fully a fully functional key. So the application now needs
> > to check that the key it is allocating is not key 0. Otherwise
> > further key operations may unexpectedly fail.
> >=20
> > To prevent this I would suggest
> >=20
> > a) do not allow allocating key 0. If it is freed it becomes
> > reserved =20
>=20
> Certainly yes. We never promised any particular key to the user
> space. So not returning key-0 during allocation should break no
> semantics.
>=20
> >=20
> > b) never expose key 0 to applications. Allocate key 2 as the default
> > key and present the key numbers with an offset to userspace so key 2
> > appears as key 0 to user applications. =20
>=20
> this is complicated. offseting the number is not easy. if kernel
> has allocated key-2 and tells the user space to use key-0, userspace
> will use bit-0 and bit-1 of the AMR register to program the
> permissions on the key, and cpu will start applying those changes on
> key-0. Basically kernel, userspace and cpu will be out-of-sync. This
> is possible if the userspace uses a shift-aware library function to
> program the AMR. BTW: A library function integrated into glibc is yet
> to be defined AFAICK.

If userspace can program the hardware directly then it is not possible
to lie about the key numbers, unfortunately. An alternative is to
reserve a different key for the kernel so applications get default key
as 0 and can set permissions on it on any platform.

How is the application denied setting the permissions on key 0 if it
can program the register directly?

Thanks

Michal

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Ram Pai]

On Tue, May 08, 2018 at 07:03:36PM +0200, Michal Suchánek wrote:
> 
> How is the application denied setting the permissions on key 0 if it
> can program the register directly?

There is a UAMOR register. The userspace can change the permissions of a
given key; by modifying the bits in AMOR register, only if the kernel
has set the corresponding bit in the UAMOR register.  For key-0 the
kernel does not set the corresponding UAMOR bit.

RP

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Michal Suchánek]

On Fri,  4 May 2018 12:22:58 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> Applications need the ability to associate an address-range with some
> key and latter revert to its initial default key. Pkey-0 comes close
> to providing this function but falls short, because the current
> implementation disallows applications to explicitly associate pkey-0
> to the address range.

Is there any way for the application to determine which key is the
default one associated with all address space until the application
installs its own key associations?

It seems it is somehow assumed this is key 0 but I did not find it
documented anywhere nor did I notice an interface for determining the
default key.

Thanks

Michal

> 
> Lets make pkey-0 less special and treat it almost like any other key.
> Thus it can be explicitly associated with any address range, and can
> be freed. This gives the application more flexibility and power.  The
> ability to free pkey-0 must be used responsibily, since pkey-0 is
> associated with almost all address-range by default.
> 
> Even with this change pkey-0 continues to be slightly more special
> from the following point of view.
> (a) it is implicitly allocated.
> (b) it is the default key assigned to any address-range.
> (c) its permissions cannot be modified by userspace.
> 
> NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
> with all pages including kernel pages, and pkeys are also active in
> kernel mode. If any permission is denied on pkey-0, the kernel running
> in the context of the application will be unable to operate.
> 
> Tested on powerpc.
> 
> cc: Thomas Gleixner <tglx@linutronix.de>
> cc: Dave Hansen <dave.hansen@intel.com>
> cc: Michael Ellermen <mpe@ellerman.id.au>
> cc: Ingo Molnar <mingo@kernel.org>
> cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> History:
> 
> 	v3: . Corrected a comment in arch_set_user_pkey_access().
> 	    . Clarified the header, to capture the notion that
> 	      pkey-0 permissions cannot be modified by userspace on
> powerpc. -- comment from Thiago
> 
> 	v2: . mm_pkey_is_allocated() continued to treat pkey-0
> special. fixed it.
> 
>  arch/powerpc/include/asm/pkeys.h |   22 ++++++++++++++++++----
>  arch/powerpc/mm/pkeys.c          |   26 +++++++++++++++-----------
>  2 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pkeys.h
> b/arch/powerpc/include/asm/pkeys.h index 0409c80..31a6976 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -101,10 +101,14 @@ 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));
> +	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);
> @@ -200,6 +204,16 @@ static inline int
> arch_set_user_pkey_access(struct task_struct *tsk, int pkey, {
>  	if (static_branch_likely(&pkey_disabled))
>  		return -EINVAL;
> +
> +	/*
> +	 * userspace should not change pkey-0 permissions.
> +	 * pkey-0 is associated with every page in the kernel.
> +	 * If userspace denies any permission on pkey-0, the
> +	 * kernel cannot operate.
> +	 */
> +	if (!pkey)
> +		return init_val ? -EINVAL : 0;
> +
>  	return __arch_set_user_pkey_access(tsk, pkey, init_val);
>  }
>  
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 0eafdf0..d6873b4 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -119,16 +119,21 @@ int pkey_initialize(void)
>  #else
>  	os_reserved = 0;
>  #endif
> +	/* Bits are in LE format. */
>  	initial_allocation_mask = ~0x0;
> +
> +	/* register mask is in BE format */
>  	pkey_amr_uamor_mask = ~0x0ul;
>  	pkey_iamr_mask = ~0x0ul;
> -	/*
> -	 * key 0, 1 are reserved.
> -	 * key 0 is the default key, which allows read/write/execute.
> -	 * key 1 is recommended not to be used. PowerISA(3.0) page
> 1015,
> -	 * programming note.
> -	 */
> -	for (i = 2; i < (pkeys_total - os_reserved); i++) {
> +
> +	for (i = 0; i < (pkeys_total - os_reserved); i++) {
> +		/*
> +		 * key 1 is recommended not to be used.
> +		 * PowerISA(3.0) page 1015,
> +		 */
> +		if (i == 1)
> +			continue;
> +
>  		initial_allocation_mask &= ~(0x1 << i);
>  		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
>  		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
> @@ -142,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
>  {
>  	if (static_branch_likely(&pkey_disabled))
>  		return;
> -	mm_pkey_allocation_map(mm) = initial_allocation_mask;
> +
> +	/* allocate key-0 by default */
> +	mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
>  	/* -1 means unallocated or invalid */
>  	mm->context.execute_only_pkey = -1;
>  }
> @@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool
> write, bool execute) int pkey_shift;
>  	u64 amr;
>  
> -	if (!pkey)
> -		return true;
> -
>  	if (!is_pkey_enabled(pkey))
>  		return true;
>  

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Dave Hansen]

On 05/09/2018 08:43 AM, Michal Suchánek wrote:
> It seems it is somehow assumed this is key 0 but I did not find it
> documented anywhere nor did I notice an interface for determining the
> default key.

Does the manpage not count as documentation?  :)

"pkey 0 is used as the default key"

https://manpages.debian.org/stretch/manpages/pkeys.7.en.html

Re: [PATCH v3] powerpc, pkey: make protection key 0 less special

[Michal Suchánek]

On Wed, 9 May 2018 08:46:08 -0700
Dave Hansen <dave.hansen@intel.com> wrote:

> On 05/09/2018 08:43 AM, Michal Such=C3=A1nek wrote:
> > It seems it is somehow assumed this is key 0 but I did not find it
> > documented anywhere nor did I notice an interface for determining
> > the default key. =20
>=20
> Does the manpage not count as documentation?  :)
>=20
> "pkey 0 is used as the default key"
>=20
> https://manpages.debian.org/stretch/manpages/pkeys.7.en.html

I happened to be reading Documentation/x86/protection-keys.txt which is
much more terse. The man page is certainly clearer.

Thanks

Michal