[PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

[PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

[Ram Pai]

Key 2 is preallocated and reserved for execute-only key. In rare
cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave
incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key.

Ensure key 2 is available for preallocation before reserving it for
execute_only purpose.  Problem noticed by Michael Ellermen.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/mm/pkeys.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index cec990c..0b03914 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -19,6 +19,7 @@
 u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
 u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
 u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
+int  execute_only_key = 2;
 
 #define AMR_BITS_PER_PKEY 2
 #define AMR_RD_BIT 0x1UL
@@ -26,7 +27,6 @@
 #define IAMR_EX_BIT 0x1UL
 #define PKEY_REG_BITS (sizeof(u64)*8)
 #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
-#define EXECUTE_ONLY_KEY 2
 
 static void scan_pkey_feature(void)
 {
@@ -122,8 +122,12 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
+
+	if ((pkeys_total - os_reserved) <= execute_only_key)
+		execute_only_key = -1;
+
 	/* Bits are in LE format. */
-	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
+	reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
 	initial_allocation_mask  = reserved_allocation_mask | (0x1 << PKEY_0);
 
 	/* register mask is in BE format */
@@ -132,11 +136,11 @@ int pkey_initialize(void)
 
 	pkey_iamr_mask = ~0x0ul;
 	pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0));
-	pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
+	pkey_iamr_mask &= ~(0x3ul << pkeyshift(execute_only_key));
 
 	pkey_uamor_mask = ~0x0ul;
 	pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0));
-	pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
+	pkey_uamor_mask &= ~(0x3ul << pkeyshift(execute_only_key));
 
 	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++)
 		pkey_uamor_mask &= ~(0x3ul << pkeyshift(i));
@@ -151,7 +155,7 @@ void pkey_mm_init(struct mm_struct *mm)
 	if (static_branch_likely(&pkey_disabled))
 		return;
 	mm_pkey_allocation_map(mm) = initial_allocation_mask;
-	mm->context.execute_only_pkey = EXECUTE_ONLY_KEY;
+	mm->context.execute_only_pkey = execute_only_key;
 }
 
 static inline u64 read_amr(void)
-- 
1.7.1

[PATCH 2/2] powerpc/pkeys: key allocation/deallocation must not change pkey registers

[Ram Pai]

Key allocation and deallocation has the side effect of programming the
UAMOR/AMR/IAMR registers. This is wrong, since its the responsibility of
the application and not that of the kernel, to modify the permission on
the key.

Do not modify the pkey registers at key allocation/deallocation.

This patch also fixes a bug where a sys_pkey_free() resets the UAMOR
bits of the key, thus making its permissions unmodifiable from user
space.  Latter if the same key gets reallocated from a different thread
this thread will no longer be able to change the permissions on the key.

Problem noticed/reported by Michael Ellermen while running
selftests/core-pkeys

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/include/asm/pkeys.h |   11 -----------
 arch/powerpc/mm/pkeys.c          |   27 ---------------------------
 2 files changed, 0 insertions(+), 38 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index c824528..92a9962 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -101,8 +101,6 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 	return __mm_pkey_is_allocated(mm, pkey);
 }
 
-extern void __arch_activate_pkey(int pkey);
-extern void __arch_deactivate_pkey(int pkey);
 /*
  * Returns a positive, 5-bit key on success, or -1 on failure.
  * Relies on the mmap_sem to protect against concurrency in mm_pkey_alloc() and
@@ -131,11 +129,6 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
 	ret = ffz((u32)mm_pkey_allocation_map(mm));
 	__mm_pkey_allocated(mm, ret);
 
-	/*
-	 * Enable the key in the hardware
-	 */
-	if (ret > 0)
-		__arch_activate_pkey(ret);
 	return ret;
 }
 
@@ -147,10 +140,6 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
 	if (!mm_pkey_is_allocated(mm, pkey))
 		return -EINVAL;
 
-	/*
-	 * Disable the key in the hardware
-	 */
-	__arch_deactivate_pkey(pkey);
 	__mm_pkey_free(mm, pkey);
 
 	return 0;
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 0b03914..27ac7f0 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -224,33 +224,6 @@ static inline void init_iamr(int pkey, u8 init_bits)
 	write_iamr(old_iamr | new_iamr_bits);
 }
 
-static void pkey_status_change(int pkey, bool enable)
-{
-	u64 old_uamor;
-
-	/* Reset the AMR and IAMR bits for this key */
-	init_amr(pkey, 0x0);
-	init_iamr(pkey, 0x0);
-
-	/* Enable/disable key */
-	old_uamor = read_uamor();
-	if (enable)
-		old_uamor |= (0x3ul << pkeyshift(pkey));
-	else
-		old_uamor &= ~(0x3ul << pkeyshift(pkey));
-	write_uamor(old_uamor);
-}
-
-void __arch_activate_pkey(int pkey)
-{
-	pkey_status_change(pkey, true);
-}
-
-void __arch_deactivate_pkey(int pkey)
-{
-	pkey_status_change(pkey, false);
-}
-
 /*
  * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
  * specified in @init_val.
-- 
1.7.1

[PATCH 1/2] powerpc/core-pkeys: execute-permission on keys are disabled by default

[Ram Pai]

Only when the key is allocated, its permission are enabled.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 tools/testing/selftests/powerpc/ptrace/core-pkey.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
index 36bc312..b353d86 100644
--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
@@ -140,6 +140,10 @@ static int child(struct shared_info *info)
 
 	if (disable_execute)
 		info->iamr |= 1ul << pkeyshift(pkey1);
+	else
+		info->iamr &= ~(1ul << pkeyshift(pkey1));
+	info->iamr &= ~(1ul << pkeyshift(pkey2) | 1ul << pkeyshift(pkey3));
+
 
 	info->uamor |= 3ul << pkeyshift(pkey1) | 3ul << pkeyshift(pkey2);
 
-- 
1.7.1

[PATCH 2/2] powerpc/ptrace-pkeys: execute-permission on keys are disabled by default

[Ram Pai]

The test case assumes execute-permissions of unallocated keys are
enabled by default.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 .../testing/selftests/powerpc/ptrace/ptrace-pkey.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
index 5cf631f..559c6cb 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
@@ -104,6 +104,11 @@ static int child(struct shared_info *info)
 
 	if (disable_execute)
 		info->expected_iamr |= 1ul << pkeyshift(pkey1);
+	else
+		info->expected_iamr &= ~(1ul << pkeyshift(pkey1));
+	info->expected_iamr &= ~(1ul << pkeyshift(pkey2) | 1ul << pkeyshift(pkey3));
+
+
 
 	info->expected_uamor |= 3ul << pkeyshift(pkey1) |
 				3ul << pkeyshift(pkey2);
-- 
1.7.1

Re: [PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

[Thiago Jung Bauermann]

Hello,

Ram Pai <linuxram@us.ibm.com> writes:

> Key 2 is preallocated and reserved for execute-only key. In rare
> cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave
> incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key.
>
> Ensure key 2 is available for preallocation before reserving it for
> execute_only purpose.  Problem noticed by Michael Ellermen.

Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet,
this patch could be squashed into it.

> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/mm/pkeys.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index cec990c..0b03914 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -19,6 +19,7 @@
>  u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
>  u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
>  u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
> +int  execute_only_key = 2;
>
>  #define AMR_BITS_PER_PKEY 2
>  #define AMR_RD_BIT 0x1UL
> @@ -26,7 +27,6 @@
>  #define IAMR_EX_BIT 0x1UL
>  #define PKEY_REG_BITS (sizeof(u64)*8)
>  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> -#define EXECUTE_ONLY_KEY 2
>
>  static void scan_pkey_feature(void)
>  {
> @@ -122,8 +122,12 @@ int pkey_initialize(void)
>  #else
>  	os_reserved = 0;
>  #endif
> +
> +	if ((pkeys_total - os_reserved) <= execute_only_key)
> +		execute_only_key = -1;
> +
>  	/* Bits are in LE format. */
> -	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
> +	reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);

My understanding is that left-shifting by a negative amount is undefined
behavior in C. A quick test tells me that at least on the couple of
machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior? If so,
a comment pointing this out would make this less confusing.

>  	initial_allocation_mask  = reserved_allocation_mask | (0x1 << PKEY_0);
>
>  	/* register mask is in BE format */
> @@ -132,11 +136,11 @@ int pkey_initialize(void)
>
>  	pkey_iamr_mask = ~0x0ul;
>  	pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0));
> -	pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
> +	pkey_iamr_mask &= ~(0x3ul << pkeyshift(execute_only_key));
>
>  	pkey_uamor_mask = ~0x0ul;
>  	pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0));
> -	pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
> +	pkey_uamor_mask &= ~(0x3ul << pkeyshift(execute_only_key));

Here the behaviour is undefined in C as well, given that pkeyshift(-1) =
64, which is the total number of bits in the left operand. Does GCC
guarantee that the result will be 0 here as well?

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

[Gabriel Paubert]

On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote:
> 
> Hello,
> 
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > Key 2 is preallocated and reserved for execute-only key. In rare
> > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave
> > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key.
> >
> > Ensure key 2 is available for preallocation before reserving it for
> > execute_only purpose.  Problem noticed by Michael Ellermen.
> 
> Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet,
> this patch could be squashed into it.
> 
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/mm/pkeys.c |   14 +++++++++-----
> >  1 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> > index cec990c..0b03914 100644
> > --- a/arch/powerpc/mm/pkeys.c
> > +++ b/arch/powerpc/mm/pkeys.c
> > @@ -19,6 +19,7 @@
> >  u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
> >  u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
> >  u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
> > +int  execute_only_key = 2;
> >
> >  #define AMR_BITS_PER_PKEY 2
> >  #define AMR_RD_BIT 0x1UL
> > @@ -26,7 +27,6 @@
> >  #define IAMR_EX_BIT 0x1UL
> >  #define PKEY_REG_BITS (sizeof(u64)*8)
> >  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> > -#define EXECUTE_ONLY_KEY 2
> >
> >  static void scan_pkey_feature(void)
> >  {
> > @@ -122,8 +122,12 @@ int pkey_initialize(void)
> >  #else
> >  	os_reserved = 0;
> >  #endif
> > +
> > +	if ((pkeys_total - os_reserved) <= execute_only_key)
> > +		execute_only_key = -1;
> > +
> >  	/* Bits are in LE format. */
> > -	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
> > +	reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
> 
> My understanding is that left-shifting by a negative amount is undefined
> behavior in C. A quick test tells me that at least on the couple of
> machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior? 

Not in general. It probably always works on Power because of the definition 
of the machine instruction for shifts with variable amount (consider the 
shift amount unsigned and take it modulo twice the width of the operand), 
but for example it fails on x86 (1<<-1 gives 0x80000000).

> If so, a comment pointing this out would make this less confusing.

Unless I miss something, this code is run once at boot, so its
performance is irrelevant.

In this case simply rewrite it as:

	reserved_allocation_mask = 0x1 << 1;
	if ( (pkeys_total - os_reserved) <= execute_only_key) {
		execute_only_key = -1;
	} else {
		reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
	}

Caveat, I have assumed that the code will either:
- only run once,
- pkeys_total and os_reserved are int, not unsigned

> 
> >  	initial_allocation_mask  = reserved_allocation_mask | (0x1 << PKEY_0);
> >
> >  	/* register mask is in BE format */
> > @@ -132,11 +136,11 @@ int pkey_initialize(void)
> >
> >  	pkey_iamr_mask = ~0x0ul;
> >  	pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0));
> > -	pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
> > +	pkey_iamr_mask &= ~(0x3ul << pkeyshift(execute_only_key));
> >
> >  	pkey_uamor_mask = ~0x0ul;
> >  	pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0));
> > -	pkey_uamor_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
> > +	pkey_uamor_mask &= ~(0x3ul << pkeyshift(execute_only_key));
> 
> Here the behaviour is undefined in C as well, given that pkeyshift(-1) =
> 64, which is the total number of bits in the left operand. Does GCC
> guarantee that the result will be 0 here as well?

Same answer: very likely on Power, not portable.

	Gabriel

Re: [PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

[Thiago Jung Bauermann]

Gabriel Paubert <paubert@iram.es> writes:

> On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote:
>>
>> Hello,
>>
>> Ram Pai <linuxram@us.ibm.com> writes:
>>
>> > Key 2 is preallocated and reserved for execute-only key. In rare
>> > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave
>> > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key.
>> >
>> > Ensure key 2 is available for preallocation before reserving it for
>> > execute_only purpose.  Problem noticed by Michael Ellermen.
>>
>> Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet,
>> this patch could be squashed into it.
>>
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> > ---
>> >  arch/powerpc/mm/pkeys.c |   14 +++++++++-----
>> >  1 files changed, 9 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
>> > index cec990c..0b03914 100644
>> > --- a/arch/powerpc/mm/pkeys.c
>> > +++ b/arch/powerpc/mm/pkeys.c
>> > @@ -19,6 +19,7 @@
>> >  u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
>> >  u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
>> >  u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
>> > +int  execute_only_key = 2;
>> >
>> >  #define AMR_BITS_PER_PKEY 2
>> >  #define AMR_RD_BIT 0x1UL
>> > @@ -26,7 +27,6 @@
>> >  #define IAMR_EX_BIT 0x1UL
>> >  #define PKEY_REG_BITS (sizeof(u64)*8)
>> >  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
>> > -#define EXECUTE_ONLY_KEY 2
>> >
>> >  static void scan_pkey_feature(void)
>> >  {
>> > @@ -122,8 +122,12 @@ int pkey_initialize(void)
>> >  #else
>> >  	os_reserved = 0;
>> >  #endif
>> > +
>> > +	if ((pkeys_total - os_reserved) <= execute_only_key)
>> > +		execute_only_key = -1;
>> > +
>> >  	/* Bits are in LE format. */
>> > -	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
>> > +	reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
>>
>> My understanding is that left-shifting by a negative amount is undefined
>> behavior in C. A quick test tells me that at least on the couple of
>> machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior?
>
> Not in general. It probably always works on Power because of the definition
> of the machine instruction for shifts with variable amount (consider the
> shift amount unsigned and take it modulo twice the width of the operand),

Ok, thanks for confirming.

> but for example it fails on x86 (1<<-1 gives 0x80000000).

Strange, this works on my laptop with an Intel(R) Core(TM) i5-7300U CPU:

$ cat blah.c
#include <stdio.h>

int main(int argc, char *argv[])
{
        printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1);
        return 0;
}
$ make blah
cc     blah.c   -o blah
blah.c: In function 'main':
blah.c:5:52: warning: left shift count is negative [-Wshift-count-negative]
  printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1);
                                                    ^~
$ ./blah
1 << -1 = 0

Even if I change the cast and printf format to int, the result is the
same. Or am I doing it wrong?

>> If so, a comment pointing this out would make this less confusing.
>
> Unless I miss something, this code is run once at boot, so its
> performance is irrelevant.
>
> In this case simply rewrite it as:
>
> 	reserved_allocation_mask = 0x1 << 1;
> 	if ( (pkeys_total - os_reserved) <= execute_only_key) {
> 		execute_only_key = -1;
> 	} else {
> 		reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
> 	}

I agree it's clearer and more robust code (except that the first
line should be inside the if block).

>
> Caveat, I have assumed that the code will either:
> - only run once,
> - pkeys_total and os_reserved are int, not unsigned

Both of the above are true.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

[Ram Pai]

On Fri, Jun 29, 2018 at 09:58:37PM -0300, Thiago Jung Bauermann wrote:
> 
> Gabriel Paubert <paubert@iram.es> writes:
> 
> > On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote:
> >>
> >> Hello,
> >>
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >>
> >> > Key 2 is preallocated and reserved for execute-only key. In rare
> >> > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave
> >> > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key.
> >> >
> >> > Ensure key 2 is available for preallocation before reserving it for
> >> > execute_only purpose.  Problem noticed by Michael Ellermen.
> >>
> >> Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet,
> >> this patch could be squashed into it.
> >>
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> > ---
> >> >  arch/powerpc/mm/pkeys.c |   14 +++++++++-----
> >> >  1 files changed, 9 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> >> > index cec990c..0b03914 100644
> >> > --- a/arch/powerpc/mm/pkeys.c
> >> > +++ b/arch/powerpc/mm/pkeys.c
> >> > @@ -19,6 +19,7 @@
> >> >  u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
> >> >  u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
> >> >  u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
> >> > +int  execute_only_key = 2;
> >> >
> >> >  #define AMR_BITS_PER_PKEY 2
> >> >  #define AMR_RD_BIT 0x1UL
> >> > @@ -26,7 +27,6 @@
> >> >  #define IAMR_EX_BIT 0x1UL
> >> >  #define PKEY_REG_BITS (sizeof(u64)*8)
> >> >  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> >> > -#define EXECUTE_ONLY_KEY 2
> >> >
> >> >  static void scan_pkey_feature(void)
> >> >  {
> >> > @@ -122,8 +122,12 @@ int pkey_initialize(void)
> >> >  #else
> >> >  	os_reserved = 0;
> >> >  #endif
> >> > +
> >> > +	if ((pkeys_total - os_reserved) <= execute_only_key)
> >> > +		execute_only_key = -1;
> >> > +
> >> >  	/* Bits are in LE format. */
> >> > -	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
> >> > +	reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
> >>
> >> My understanding is that left-shifting by a negative amount is undefined
> >> behavior in C. A quick test tells me that at least on the couple of
> >> machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior?
> >
> > Not in general. It probably always works on Power because of the definition
> > of the machine instruction for shifts with variable amount (consider the
> > shift amount unsigned and take it modulo twice the width of the operand),
> 
> Ok, thanks for confirming.
> 
> > but for example it fails on x86 (1<<-1 gives 0x80000000).
> 
> Strange, this works on my laptop with an Intel(R) Core(TM) i5-7300U CPU:
> 
> $ cat blah.c
> #include <stdio.h>
> 
> int main(int argc, char *argv[])
> {

>         return 0;
> }
> $ make blah
> cc     blah.c   -o blah
> blah.c: In function 'main':
> blah.c:5:52: warning: left shift count is negative [-Wshift-count-negative]
>   printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1);
>                                                     ^~
> $ ./blah
> 1 << -1 = 0

My intel box does the same. It makes it zero. So does my
powerpc box.  Mathematically, (1 << -1) is nothing but 2^-1,
which is 1/2, which is 0.5, and when rounded it has to be 0.

However, yes, GCC defines it to be 'undefined'.  gcc compiler does
warn 'left shift count is negative'. Will have to fix it.

Thanks for catching this.

> 
> Even if I change the cast and printf format to int, the result is the
> same. Or am I doing it wrong?
> 
> >> If so, a comment pointing this out would make this less confusing.
> >
> > Unless I miss something, this code is run once at boot, so its
> > performance is irrelevant.
> >
> > In this case simply rewrite it as:
> >
> > 	reserved_allocation_mask = 0x1 << 1;
> > 	if ( (pkeys_total - os_reserved) <= execute_only_key) {
> > 		execute_only_key = -1;
> > 	} else {
> > 		reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
> > 	}

I tried hard not to sprikle if-then-else in the code. Makes it less
	elegant.  But then, correctness is  more important than
	elegance.

> 
> I agree it's clearer and more robust code (except that the first
> line should be inside the if block).
> 
> >
> > Caveat, I have assumed that the code will either:
> > - only run once,
> > - pkeys_total and os_reserved are int, not unsigned
> 
> Both of the above are true.

yes.

Thanks,
RP

Re: [PATCH 1/2] powerpc/pkeys: preallocate execute_only key only if the key is available.

[Gabriel Paubert]

On Fri, Jun 29, 2018 at 09:58:37PM -0300, Thiago Jung Bauermann wrote:
> 
> Gabriel Paubert <paubert@iram.es> writes:
> 
> > On Thu, Jun 28, 2018 at 11:56:34PM -0300, Thiago Jung Bauermann wrote:
> >>
> >> Hello,
> >>
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >>
> >> > Key 2 is preallocated and reserved for execute-only key. In rare
> >> > cases if key-2 is unavailable, mprotect(PROT_EXEC) will behave
> >> > incorrectly. NOTE: mprotect(PROT_EXEC) uses execute-only key.
> >> >
> >> > Ensure key 2 is available for preallocation before reserving it for
> >> > execute_only purpose.  Problem noticed by Michael Ellermen.
> >>
> >> Since "powerpc/pkeys: Preallocate execute-only key" isn't upstream yet,
> >> this patch could be squashed into it.
> >>
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> > ---
> >> >  arch/powerpc/mm/pkeys.c |   14 +++++++++-----
> >> >  1 files changed, 9 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> >> > index cec990c..0b03914 100644
> >> > --- a/arch/powerpc/mm/pkeys.c
> >> > +++ b/arch/powerpc/mm/pkeys.c
> >> > @@ -19,6 +19,7 @@
> >> >  u64  pkey_amr_mask;		/* Bits in AMR not to be touched */
> >> >  u64  pkey_iamr_mask;		/* Bits in AMR not to be touched */
> >> >  u64  pkey_uamor_mask;		/* Bits in UMOR not to be touched */
> >> > +int  execute_only_key = 2;
> >> >
> >> >  #define AMR_BITS_PER_PKEY 2
> >> >  #define AMR_RD_BIT 0x1UL
> >> > @@ -26,7 +27,6 @@
> >> >  #define IAMR_EX_BIT 0x1UL
> >> >  #define PKEY_REG_BITS (sizeof(u64)*8)
> >> >  #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
> >> > -#define EXECUTE_ONLY_KEY 2
> >> >
> >> >  static void scan_pkey_feature(void)
> >> >  {
> >> > @@ -122,8 +122,12 @@ int pkey_initialize(void)
> >> >  #else
> >> >  	os_reserved = 0;
> >> >  #endif
> >> > +
> >> > +	if ((pkeys_total - os_reserved) <= execute_only_key)
> >> > +		execute_only_key = -1;
> >> > +
> >> >  	/* Bits are in LE format. */
> >> > -	reserved_allocation_mask = (0x1 << 1) | (0x1 << EXECUTE_ONLY_KEY);
> >> > +	reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
> >>
> >> My understanding is that left-shifting by a negative amount is undefined
> >> behavior in C. A quick test tells me that at least on the couple of
> >> machines I tested, 1 < -1 = 0. Does GCC guarantee that behavior?
> >
> > Not in general. It probably always works on Power because of the definition
> > of the machine instruction for shifts with variable amount (consider the
> > shift amount unsigned and take it modulo twice the width of the operand),
> 
> Ok, thanks for confirming.
> 
> > but for example it fails on x86 (1<<-1 gives 0x80000000).
> 
> Strange, this works on my laptop with an Intel(R) Core(TM) i5-7300U CPU:
> 
> $ cat blah.c
> #include <stdio.h>
> 
> int main(int argc, char *argv[])
> {
>         printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1);
>         return 0;
> }
> $ make blah
> cc     blah.c   -o blah
> blah.c: In function 'main':
> blah.c:5:52: warning: left shift count is negative [-Wshift-count-negative]
>   printf("1 << -1 = %llx\n", (unsigned long long) 1 << -1);
>                                                     ^~
> $ ./blah
> 1 << -1 = 0
> 
> Even if I change the cast and printf format to int, the result is the
> same. Or am I doing it wrong?

Try something more dynamic, 1 << -1 is evaluated at compile time by gcc,
and when you get a warning, the compile time expression evaluation does
not always give the same result as the run time machine instructions.

To test this I wrote (yes, the error checking is approximate, it would
be better to use strtol):

/***************************************************************************/
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
	int val, amount, valid;
	if (argc != 3) {
		printf("Needs 2 arguments!\n");
		return EXIT_FAILURE;
	}
	valid = sscanf(argv[1], "%d", &val);
	if (valid == 1) {
		valid = sscanf(argv[2], "%d", &amount);
	}
	if (valid == 1) {
		printf("%d shifted by %d is %d\n", val, amount, val<<amount);
		return EXIT_SUCCESS;
	} else {
		printf("Both arguments must be integers!\n");
		return EXIT_FAILURE;
	}
}
/***************************************************************************/

Compile it, and then run it with 1 and -1 as parameters. The result is
not the same on PPC and x86.


> 
> >> If so, a comment pointing this out would make this less confusing.
> >
> > Unless I miss something, this code is run once at boot, so its
> > performance is irrelevant.
> >
> > In this case simply rewrite it as:
> >
> > 	reserved_allocation_mask = 0x1 << 1;
> > 	if ( (pkeys_total - os_reserved) <= execute_only_key) {
> > 		execute_only_key = -1;
> > 	} else {
> > 		reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
> > 	}
> 
> I agree it's clearer and more robust code (except that the first
> line should be inside the if block).

Indeed, sorry for this.

	Gabriel

> 
> >
> > Caveat, I have assumed that the code will either:
> > - only run once,
> > - pkeys_total and os_reserved are int, not unsigned
> 
> Both of the above are true.
> 
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 

Re: [PATCH 2/2] powerpc/pkeys: key allocation/deallocation must not change pkey registers

[Thiago Jung Bauermann]

Ram Pai <linuxram@us.ibm.com> writes:

> Key allocation and deallocation has the side effect of programming the
> UAMOR/AMR/IAMR registers. This is wrong, since its the responsibility of
> the application and not that of the kernel, to modify the permission on
> the key.
>
> Do not modify the pkey registers at key allocation/deallocation.
>
> This patch also fixes a bug where a sys_pkey_free() resets the UAMOR
> bits of the key, thus making its permissions unmodifiable from user
> space.  Latter if the same key gets reallocated from a different thread
> this thread will no longer be able to change the permissions on the key.
>
> Problem noticed/reported by Michael Ellermen while running
> selftests/core-pkeys
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/pkeys.h |   11 -----------
>  arch/powerpc/mm/pkeys.c          |   27 ---------------------------
>  2 files changed, 0 insertions(+), 38 deletions(-)

LGTM.

Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 1/2] powerpc/core-pkeys: execute-permission on keys are disabled by default

[Thiago Jung Bauermann]

Ram Pai <linuxram@us.ibm.com> writes:

> Only when the key is allocated, its permission are enabled.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  tools/testing/selftests/powerpc/ptrace/core-pkey.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> index 36bc312..b353d86 100644
> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> @@ -140,6 +140,10 @@ static int child(struct shared_info *info)
>
>  	if (disable_execute)
>  		info->iamr |= 1ul << pkeyshift(pkey1);
> +	else
> +		info->iamr &= ~(1ul << pkeyshift(pkey1));
> +	info->iamr &= ~(1ul << pkeyshift(pkey2) | 1ul << pkeyshift(pkey3));
> +
>
>  	info->uamor |= 3ul << pkeyshift(pkey1) | 3ul << pkeyshift(pkey2);

Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 2/2] powerpc/ptrace-pkeys: execute-permission on keys are disabled by default

[Thiago Jung Bauermann]

Ram Pai <linuxram@us.ibm.com> writes:

> The test case assumes execute-permissions of unallocated keys are
> enabled by default.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  .../testing/selftests/powerpc/ptrace/ptrace-pkey.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
> index 5cf631f..559c6cb 100644
> --- a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
> +++ b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
> @@ -104,6 +104,11 @@ static int child(struct shared_info *info)
>
>  	if (disable_execute)
>  		info->expected_iamr |= 1ul << pkeyshift(pkey1);
> +	else
> +		info->expected_iamr &= ~(1ul << pkeyshift(pkey1));
> +	info->expected_iamr &= ~(1ul << pkeyshift(pkey2) | 1ul << pkeyshift(pkey3));
> +
> +
>
>  	info->expected_uamor |= 3ul << pkeyshift(pkey1) |
>  				3ul << pkeyshift(pkey2);

Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

-- 
Thiago Jung Bauermann
IBM Linux Technology Center