pkeys on POWER: Access rights not reset on execve

pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

This test program:

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <err.h>

/* Return the value of the AMR register.  */
static inline unsigned long int
pkey_read (void)
{
   unsigned long int result;
   __asm__ volatile ("mfspr %0, 13" : "=r" (result));
   return result;
}

/* Overwrite the AMR register with VALUE.  */
static inline void
pkey_write (unsigned long int value)
{
   __asm__ volatile ("mtspr 13, %0" : : "r" (value));
}

int
main (int argc, char **argv)
{
   printf ("AMR (PID %d): 0x%016lx\n", (int) getpid (), pkey_read());
   if (argc > 1)
     {
       int key = syscall (__NR_pkey_alloc, 0, 0);
       if (key < 0)
         err (1, "pkey_alloc");
       printf ("Allocated key (PID %d): %d\n", (int) getpid (), key);
       return 0;
     }

   pid_t pid = fork ();
   if (pid == 0)
     {
       execl ("/proc/self/exe", argv[0], "subprocess", NULL);
       _exit (1);
     }
   if (pid < 0)
     err (1, "fork");
   int status;
   if (waitpid (pid, &status, 0) < 0)
     err (1, "waitpid");

   int key = syscall (__NR_pkey_alloc, 0, 0);
   if (key < 0)
     err (1, "pkey_alloc");
   printf ("Allocated key (PID %d): %d\n", (int) getpid (), key);

   unsigned long int amr = -1;
   printf ("Setting AMR: 0x%016lx\n", amr);
   pkey_write (amr);
   printf ("New AMR value (PID %d, before execl): 0x%016lx\n",
           (int) getpid (), pkey_read());
   execl ("/proc/self/exe", argv[0], "subprocess", NULL);
   err (1, "exec");
   return 1;
}

shows that the AMR register value is not reset on execve:

AMR (PID 112291): 0x0000000000000000
AMR (PID 112292): 0x0000000000000000
Allocated key (PID 112292): 2
Allocated key (PID 112291): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 112291, before execl): 0x0c00000000000000
AMR (PID 112291): 0x0c00000000000000
Allocated key (PID 112291): 2

I think this is a real bug and needs to be fixed even if the defaults 
are kept as-is (see the other thread).

(Seen on 4.17.0-rc5.)

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Fri, May 18, 2018 at 04:27:14PM +0200, Florian Weimer wrote:
> This test program:
> 
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <err.h>
> 
> /* Return the value of the AMR register.  */
> static inline unsigned long int
> pkey_read (void)
> {
>   unsigned long int result;
>   __asm__ volatile ("mfspr %0, 13" : "=r" (result));
>   return result;
> }
> 
> /* Overwrite the AMR register with VALUE.  */
> static inline void
> pkey_write (unsigned long int value)
> {
>   __asm__ volatile ("mtspr 13, %0" : : "r" (value));
> }
> 
> int
> main (int argc, char **argv)
> {
>   printf ("AMR (PID %d): 0x%016lx\n", (int) getpid (), pkey_read());
>   if (argc > 1)
>     {
>       int key = syscall (__NR_pkey_alloc, 0, 0);
>       if (key < 0)
>         err (1, "pkey_alloc");
>       printf ("Allocated key (PID %d): %d\n", (int) getpid (), key);
>       return 0;
>     }
> 
>   pid_t pid = fork ();
>   if (pid == 0)
>     {
>       execl ("/proc/self/exe", argv[0], "subprocess", NULL);
>       _exit (1);
>     }
>   if (pid < 0)
>     err (1, "fork");
>   int status;
>   if (waitpid (pid, &status, 0) < 0)
>     err (1, "waitpid");
> 
>   int key = syscall (__NR_pkey_alloc, 0, 0);
>   if (key < 0)
>     err (1, "pkey_alloc");
>   printf ("Allocated key (PID %d): %d\n", (int) getpid (), key);
> 
>   unsigned long int amr = -1;
>   printf ("Setting AMR: 0x%016lx\n", amr);
>   pkey_write (amr);
>   printf ("New AMR value (PID %d, before execl): 0x%016lx\n",
>           (int) getpid (), pkey_read());
>   execl ("/proc/self/exe", argv[0], "subprocess", NULL);
>   err (1, "exec");
>   return 1;
> }
> 
> shows that the AMR register value is not reset on execve:
> 
> AMR (PID 112291): 0x0000000000000000
> AMR (PID 112292): 0x0000000000000000
> Allocated key (PID 112292): 2
> Allocated key (PID 112291): 2
> Setting AMR: 0xffffffffffffffff
> New AMR value (PID 112291, before execl): 0x0c00000000000000
> AMR (PID 112291): 0x0c00000000000000
> Allocated key (PID 112291): 2
> 
> I think this is a real bug and needs to be fixed even if the
> defaults are kept as-is (see the other thread).

The issue you may be talking about here is that  --

"when you set the AMR register to 0xffffffffffffffff, it 
just sets it to 0x0c00000000000000."

To me it looks like, exec/fork are not related to the issue.
Or are they also somehow connected to the issue?


The reason the AMR register does not get set to 0xffffffffffffffff,
is because none of those keys; except key 2, are active. So it ignores
all other bits and just sets the bits corresponding to key 2.

However the fundamental issue is still the same, as mentioned in the
other thread.

"Should the permissions on a key be allowed to be changed, if the key
is not allocated in the first place?".

my answer is NO. Lets debate :)
RP

Re: pkeys on POWER: Access rights not reset on execve

[Andy Lutomirski]

On Fri, May 18, 2018 at 6:19 PM Ram Pai <linuxram@us.ibm.com> wrote:

> However the fundamental issue is still the same, as mentioned in the
> other thread.

> "Should the permissions on a key be allowed to be changed, if the key
> is not allocated in the first place?".

> my answer is NO. Lets debate :)

As a preface, here's my two-minute attempt to understand POWER's behavior:
there are two registers, AMR and UAMR.  AMR contains both kernel-relevant
state and user-relevant state.  UAMR specifies which bits of AMR are
available for user code to directly access.  AMR bits outside UAMR are read
as zero and are unaffected by writes.  I'm assuming that the kernel
reserves some subset of AMR bits in advance to correspond to allocatable
pkeys.

Here's my question: given that disallowed AMR bits are read-as-zero, there
can always be a thread that is in the middle of a sequence like:

unsigned long old = amr;
amr |= whatever;
...  <- thread is here
amr = old;

Now another thread calls pkey_alloc(), so UAMR is asynchronously changed,
and the thread will write zero to the relevant AMR bits.  If I understand
correctly, this means that the decision to mask off unallocated keys via
UAMR effectively forces the initial value of newly-allocated keys in other
threads in the allocating process to be zero, whatever zero means.  (I
didn't get far enough in the POWER docs to figure out what zero means.)  So
I don't think you're doing anyone any favors by making UAMR dynamic.

IOW both x86 and POWER screwed up the ISA.

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 05/19/2018 03:50 AM, Andy Lutomirski wrote:
> Now another thread calls pkey_alloc(), so UAMR is asynchronously changed,
> and the thread will write zero to the relevant AMR bits.  If I understand
> correctly, this means that the decision to mask off unallocated keys via
> UAMR effectively forces the initial value of newly-allocated keys in other
> threads in the allocating process to be zero, whatever zero means.  (I
> didn't get far enough in the POWER docs to figure out what zero means.)  So

(Note that this belongs on the other thread, here I originally wanted to 
talk about the lack of reset of AMR to the default value on execve.)

I don't think UAMOR is updated asynchronously.  On pkey_alloc, it is 
only changed for the current thread, and future threads launched from 
it.  Existing threads are unaffected.  This still results in a 
programming model which is substantially different from x86.

> I don't think you're doing anyone any favors by making UAMR dynamic.

This is still true, I think.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Fri, May 18, 2018 at 06:50:39PM -0700, Andy Lutomirski wrote:
> On Fri, May 18, 2018 at 6:19 PM Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > However the fundamental issue is still the same, as mentioned in the
> > other thread.
> 
> > "Should the permissions on a key be allowed to be changed, if the key
> > is not allocated in the first place?".
> 
> > my answer is NO. Lets debate :)
> 
> As a preface, here's my two-minute attempt to understand POWER's behavior:
> there are two registers, AMR and UAMR.  AMR contains both kernel-relevant
> state and user-relevant state.  UAMR specifies which bits of AMR are
> available for user code to directly access.  AMR bits outside UAMR are read
> as zero and are unaffected by writes.  I'm assuming that the kernel
> reserves some subset of AMR bits in advance to correspond to allocatable
> pkeys.


You got it mostly right. Filling in some more details below for
completeness.

Yes there is a AMR register which has two bits each corresponding to
each key.  One bit corresponds to read permission, and the other bit
corresponds to write permission.  If set, the corresponding permission
is denied.  Both userspace and kernel can modify the register.

Yes there is a UAMOR register which has a bit corresponding to each key.
When a bit in UAMOR register is set, that key's permission can be
changed by userspace. Only kernel can modify UAMOR register.

for example, if bit 10 is set in UAMOR register, only then key-10's read-write
permissions can be modified by userspace.

NOTE: the UAMR register you mention above, is actually UAMOR register. 

And finally the kernel reserves some subset of keys, in advance, that
it wants for itself. It will never give away those keys to userspace
through sys_pkey_alloc(), and the bits corresponding to those keys will
be 0 in UAMOR register.

> 
> Here's my question: given that disallowed AMR bits are read-as-zero, there
> can always be a thread that is in the middle of a sequence like:
> 
> step1 : unsigned long old = amr;
> step2 : amr |= whatever;
> step3 : ...  <- thread is here
> step4 : amr = old;
> 
> Now another thread calls pkey_alloc(), so UAMR is asynchronously changed,
> and the thread will write zero to the relevant AMR bits. 

> If I understand
> correctly, this means that the decision to mask off unallocated keys via
> UAMR effectively forces the initial value of newly-allocated keys in other
> threads in the allocating process to be zero, whatever zero means.

The initial value of the newly allocated key will be whatever the
init_value is, that is specified in the sys_pkey_alloc().

Remember, the UAMOR and the AMR values are thread specific. If thread T2
allocates a new key, then that thread will enable the bit in its version
of the UAMOR register. It will not have any effect on the UAMOR value of
any other threads's version.

So in the above code snippet, assuming T1 is executing the code, and T2 is
running parallely, and assume that both threads have the same AMR and
UAMOR value till the end of step2.  At step 3, when T2 allocates a new
key,  thread T2's UAMOR will enable the bit corresponding to the
allocated key and set the corresponding bits in AMR to the init_val
specified in sys_pkey_alloc(). It has no effect on thread T1's UAMOR or
AMR register.

> (I
> didn't get far enough in the POWER docs to figure out what zero means.).

ok.

> So
> I don't think you're doing anyone any favors by making UAMR dynamic.

depending on the explaination above, do you continue to hold that opinion?

> 
> IOW both x86 and POWER screwed up the ISA.

-- 
Ram Pai

Re: pkeys on POWER: Access rights not reset on execve

[Andy Lutomirski]

On Sat, May 19, 2018 at 1:28 PM Ram Pai <linuxram@us.ibm.com> wrote:

> You got it mostly right. Filling in some more details below for
> completeness.

> [...]

Okay, so I guess I was correct as to what the functionality was but not as
to the encoding or the name of UAMOR.

Can you also confirm that mprotect_key() affects all threads?


> And finally the kernel reserves some subset of keys, in advance, that
> it wants for itself. It will never give away those keys to userspace
> through sys_pkey_alloc(), and the bits corresponding to those keys will
> be 0 in UAMOR register.

> >
> > Here's my question: given that disallowed AMR bits are read-as-zero,
there
> > can always be a thread that is in the middle of a sequence like:
> >
> > step1 : unsigned long old = amr;
> > step2 : amr |= whatever;
> > step3 : ...  <- thread is here
> > step4 : amr = old;
> >
> > Now another thread calls pkey_alloc(), so UAMR is asynchronously
changed,
> > and the thread will write zero to the relevant AMR bits.

> > If I understand
> > correctly, this means that the decision to mask off unallocated keys via
> > UAMR effectively forces the initial value of newly-allocated keys in
other
> > threads in the allocating process to be zero, whatever zero means.

> The initial value of the newly allocated key will be whatever the
> init_value is, that is specified in the sys_pkey_alloc().

> Remember, the UAMOR and the AMR values are thread specific. If thread T2
> allocates a new key, then that thread will enable the bit in its version
> of the UAMOR register. It will not have any effect on the UAMOR value of
> any other threads's version.

So is it possible for two threads to each call pkey_alloc() and end up with
the same key?  If so, it seems entirely broken.  If not, then how do you
intend for a multithreaded application to usefully allocate a new key?
Regardless, it seems like the current behavior on POWER is very difficult
to work with.  Can you give an example of a use case for which POWER's
behavior makes sense?

For the use cases I've imagined, POWER's behavior does not make sense.
  x86's is not ideal but is still better.  Here are my two example use cases:

1. A crypto library.  Suppose I'm writing a TLS-terminating server, and I
want it to be resistant to Heartbleed-like bugs.  I could store my private
keys protected by mprotect_key() and arrange for all threads and signal
handlers to have PKRU/AMR values that prevent any access to the memory.
When an explicit call is made to sign with the key, I would temporarily
change PKRU/AMR to allow access, compute the signature, and change PKRU/AMR
back.  On x86 right now, this works nicely.  On POWER, it doesn't, because
any thread started before my pkey_alloc() call can access the protected
memory, as can any signal handler.

2. A database using mmap() (with persistent memory or otherwise).  It would
be nice to be resistant to accidental corruption due to stray writes.  I
would do more or less the same thing as (1), except that I would want
threads that are not actively writing to the database to be able the
protected memory.  On x86, I need to manually convince threads that may
have been started before my pkey_alloc() call as well as signal handlers to
update their PKRU values.  On POWER, as in example (1), the error goes the
other direction -- if I fail to propagate the AMR bits to all threads,
writes are not blocked.

--Andy

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Sat, May 19, 2018 at 04:47:23PM -0700, Andy Lutomirski wrote: > On Sat, May 19, 2018 at 1:28 PM Ram Pai <linuxram@us.ibm.com> wrote:

...snip...
> 
> So is it possible for two threads to each call pkey_alloc() and end up with
> the same key?  If so, it seems entirely broken. 

No. Two threads cannot allocate the same key; just like x86. 

> If not, then how do you
> intend for a multithreaded application to usefully allocate a new key?
> Regardless, it seems like the current behavior on POWER is very difficult
> to work with.  Can you give an example of a use case for which POWER's
> behavior makes sense?
> 
> For the use cases I've imagined, POWER's behavior does not make sense.
>   x86's is not ideal but is still better.  Here are my two example use cases:
> 
> 1. A crypto library.  Suppose I'm writing a TLS-terminating server, and I
> want it to be resistant to Heartbleed-like bugs.  I could store my private
> keys protected by mprotect_key() and arrange for all threads and signal
> handlers to have PKRU/AMR values that prevent any access to the memory.
> When an explicit call is made to sign with the key, I would temporarily
> change PKRU/AMR to allow access, compute the signature, and change PKRU/AMR
> back.  On x86 right now, this works nicely.  On POWER, it doesn't, because
> any thread started before my pkey_alloc() call can access the protected
> memory, as can any signal handler.
> 
> 2. A database using mmap() (with persistent memory or otherwise).  It would
> be nice to be resistant to accidental corruption due to stray writes.  I
> would do more or less the same thing as (1), except that I would want
> threads that are not actively writing to the database to be able the
> protected memory.  On x86, I need to manually convince threads that may
> have been started before my pkey_alloc() call as well as signal handlers to
> update their PKRU values.  On POWER, as in example (1), the error goes the
> other direction -- if I fail to propagate the AMR bits to all threads,
> writes are not blocked.

I see the problem from an application's point of view, on powerpc.  If
the key allocated in one thread is not activated on all threads
(existing one and future one), than other threads will not be able
to modify the key's permissions. Hence they will not be able to control
access/write to pages to which the key is associated.

As Florian suggested, I should enable the key's bit in the UAMOR value
corresponding to existing threads, when a new key is allocated.

Now, looking at the implementation for x86, I see that sys_mpkey_alloc()
makes no attempt to modify anything of any other thread. How
does it manage to activate the key on any other thread? Is this
magic done by the hardware?

RP

Re: pkeys on POWER: Access rights not reset on execve

[Andy Lutomirski]

On Sat, May 19, 2018 at 11:04 PM Ram Pai <linuxram@us.ibm.com> wrote:

> On Sat, May 19, 2018 at 04:47:23PM -0700, Andy Lutomirski wrote: > On
Sat, May 19, 2018 at 1:28 PM Ram Pai <linuxram@us.ibm.com> wrote:

> ...snip...
> >
> > So is it possible for two threads to each call pkey_alloc() and end up
with
> > the same key?  If so, it seems entirely broken.

> No. Two threads cannot allocate the same key; just like x86.

> > If not, then how do you
> > intend for a multithreaded application to usefully allocate a new key?
> > Regardless, it seems like the current behavior on POWER is very
difficult
> > to work with.  Can you give an example of a use case for which POWER's
> > behavior makes sense?
> >
> > For the use cases I've imagined, POWER's behavior does not make sense.
> >   x86's is not ideal but is still better.  Here are my two example use
cases:
> >
> > 1. A crypto library.  Suppose I'm writing a TLS-terminating server, and
I
> > want it to be resistant to Heartbleed-like bugs.  I could store my
private
> > keys protected by mprotect_key() and arrange for all threads and signal
> > handlers to have PKRU/AMR values that prevent any access to the memory.
> > When an explicit call is made to sign with the key, I would temporarily
> > change PKRU/AMR to allow access, compute the signature, and change
PKRU/AMR
> > back.  On x86 right now, this works nicely.  On POWER, it doesn't,
because
> > any thread started before my pkey_alloc() call can access the protected
> > memory, as can any signal handler.
> >
> > 2. A database using mmap() (with persistent memory or otherwise).  It
would
> > be nice to be resistant to accidental corruption due to stray writes.  I
> > would do more or less the same thing as (1), except that I would want
> > threads that are not actively writing to the database to be able the
> > protected memory.  On x86, I need to manually convince threads that may
> > have been started before my pkey_alloc() call as well as signal
handlers to
> > update their PKRU values.  On POWER, as in example (1), the error goes
the
> > other direction -- if I fail to propagate the AMR bits to all threads,
> > writes are not blocked.

> I see the problem from an application's point of view, on powerpc.  If
> the key allocated in one thread is not activated on all threads
> (existing one and future one), than other threads will not be able
> to modify the key's permissions. Hence they will not be able to control
> access/write to pages to which the key is associated.

> As Florian suggested, I should enable the key's bit in the UAMOR value
> corresponding to existing threads, when a new key is allocated.

> Now, looking at the implementation for x86, I see that sys_mpkey_alloc()
> makes no attempt to modify anything of any other thread. How
> does it manage to activate the key on any other thread? Is this
> magic done by the hardware?

x86 has no equivalent concept to UAMOR.  There are 16 keys no matter what.

--Andy

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Sat, May 19, 2018 at 11:06:20PM -0700, Andy Lutomirski wrote:
> On Sat, May 19, 2018 at 11:04 PM Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > On Sat, May 19, 2018 at 04:47:23PM -0700, Andy Lutomirski wrote: > On
> Sat, May 19, 2018 at 1:28 PM Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > ...snip...
> > >
> > > So is it possible for two threads to each call pkey_alloc() and end up
> with
> > > the same key?  If so, it seems entirely broken.
> 
> > No. Two threads cannot allocate the same key; just like x86.
> 
> > > If not, then how do you
> > > intend for a multithreaded application to usefully allocate a new key?
> > > Regardless, it seems like the current behavior on POWER is very
> difficult
> > > to work with.  Can you give an example of a use case for which POWER's
> > > behavior makes sense?
> > >
> > > For the use cases I've imagined, POWER's behavior does not make sense.
> > >   x86's is not ideal but is still better.  Here are my two example use
> cases:
> > >
> > > 1. A crypto library.  Suppose I'm writing a TLS-terminating server, and
> I
> > > want it to be resistant to Heartbleed-like bugs.  I could store my
> private
> > > keys protected by mprotect_key() and arrange for all threads and signal
> > > handlers to have PKRU/AMR values that prevent any access to the memory.
> > > When an explicit call is made to sign with the key, I would temporarily
> > > change PKRU/AMR to allow access, compute the signature, and change
> PKRU/AMR
> > > back.  On x86 right now, this works nicely.  On POWER, it doesn't,
> because
> > > any thread started before my pkey_alloc() call can access the protected
> > > memory, as can any signal handler.
> > >
> > > 2. A database using mmap() (with persistent memory or otherwise).  It
> would
> > > be nice to be resistant to accidental corruption due to stray writes.  I
> > > would do more or less the same thing as (1), except that I would want
> > > threads that are not actively writing to the database to be able the
> > > protected memory.  On x86, I need to manually convince threads that may
> > > have been started before my pkey_alloc() call as well as signal
> handlers to
> > > update their PKRU values.  On POWER, as in example (1), the error goes
> the
> > > other direction -- if I fail to propagate the AMR bits to all threads,
> > > writes are not blocked.
> 
> > I see the problem from an application's point of view, on powerpc.  If
> > the key allocated in one thread is not activated on all threads
> > (existing one and future one), than other threads will not be able
> > to modify the key's permissions. Hence they will not be able to control
> > access/write to pages to which the key is associated.
> 
> > As Florian suggested, I should enable the key's bit in the UAMOR value
> > corresponding to existing threads, when a new key is allocated.
> 
> > Now, looking at the implementation for x86, I see that sys_mpkey_alloc()
> > makes no attempt to modify anything of any other thread. How
> > does it manage to activate the key on any other thread? Is this
> > magic done by the hardware?
> 
> x86 has no equivalent concept to UAMOR.  There are 16 keys no matter what.


Florian,

	Does the following patch fix the problem for you?  Just like x86
	I am enabling all keys in the UAMOR register during
	initialization itself. Hence any key created by any thread at
	any time, will get activated on all threads. So any thread
	can change the permission on that key. Smoke tested it
	with your test program.


Signed-off-by: Ram Pai <linuxram@us.ibm.com>
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 0eafdf01..ab4519a 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -15,8 +15,9 @@
 int  pkeys_total;		/* Total pkeys as per device tree */
 bool pkeys_devtree_defined;	/* pkey property exported by device tree */
 u32  initial_allocation_mask;	/* Bits set for reserved keys */
-u64  pkey_amr_uamor_mask;	/* Bits in AMR/UMOR not to be touched */
+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 */
 
 #define AMR_BITS_PER_PKEY 2
 #define AMR_RD_BIT 0x1UL
@@ -24,6 +25,7 @@
 #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 switch_off(bitmask, i) (bitmask &= ~(0x3ul << pkeyshift(i)))
 
 static void scan_pkey_feature(void)
 {
@@ -120,19 +122,31 @@ int pkey_initialize(void)
 	os_reserved = 0;
 #endif
 	initial_allocation_mask = ~0x0;
-	pkey_amr_uamor_mask = ~0x0ul;
+
+	/* register mask is in BE format */
+	pkey_amr_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++) {
-		initial_allocation_mask &= ~(0x1 << i);
-		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
+
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+		if (i != 0 &&  i != 1) /* 0 and 1 are already allocated */
+			initial_allocation_mask &= ~(0x1 << i);
+		pkey_amr_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
 	}
+
+	pkey_uamor_mask = ~0x0ul;
+	switch_off(pkey_uamor_mask, 0);
+	/*
+	 * key 1 is recommended not to be used.
+	 * PowerISA(3.0) page 1015,
+	 * @TODO: Revisit this. This is only applicable on
+	 * pseries kernel running on powerVM.
+	 */
+	switch_off(pkey_uamor_mask, 1);
+	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++)
+		switch_off(pkey_uamor_mask, i);
+
+	printk("pkey_uamor_mask=0x%llx \n", pkey_uamor_mask);
 	return 0;
 }
 
@@ -280,7 +294,6 @@ void thread_pkey_regs_save(struct thread_struct *thread)
 	 */
 	thread->amr = read_amr();
 	thread->iamr = read_iamr();
-	thread->uamor = read_uamor();
 }
 
 void thread_pkey_regs_restore(struct thread_struct *new_thread,
@@ -289,9 +302,6 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
 	if (static_branch_likely(&pkey_disabled))
 		return;
 
-	/*
-	 * TODO: Just set UAMOR to zero if @new_thread hasn't used any keys yet.
-	 */
 	if (old_thread->amr != new_thread->amr)
 		write_amr(new_thread->amr);
 	if (old_thread->iamr != new_thread->iamr)
@@ -305,9 +315,9 @@ void thread_pkey_regs_init(struct thread_struct *thread)
 	if (static_branch_likely(&pkey_disabled))
 		return;
 
-	thread->amr = read_amr() & pkey_amr_uamor_mask;
+	thread->amr = read_amr() & pkey_amr_mask;
 	thread->iamr = read_iamr() & pkey_iamr_mask;
-	thread->uamor = read_uamor() & pkey_amr_uamor_mask;
+	thread->uamor = pkey_uamor_mask;
 }
 
 static inline bool pkey_allows_readwrite(int pkey)


> 
> --Andy

-- 
Ram Pai

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 05/20/2018 09:11 PM, Ram Pai wrote:
> Florian,
> 
> 	Does the following patch fix the problem for you?  Just like x86
> 	I am enabling all keys in the UAMOR register during
> 	initialization itself. Hence any key created by any thread at
> 	any time, will get activated on all threads. So any thread
> 	can change the permission on that key. Smoke tested it
> 	with your test program.

I think this goes in the right direction, but the AMR value after fork 
is still strange:

AMR (PID 34912): 0x0000000000000000
AMR after fork (PID 34913): 0x0000000000000000
AMR (PID 34913): 0x0000000000000000
Allocated key in subprocess (PID 34913): 2
Allocated key (PID 34912): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 34912): 0x0fffffffffffffff
About to call execl (PID 34912) ...
AMR (PID 34912): 0x0fffffffffffffff
AMR after fork (PID 34914): 0x0000000000000003
AMR (PID 34914): 0x0000000000000003
Allocated key in subprocess (PID 34914): 2
Allocated key (PID 34912): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 34912): 0x0fffffffffffffff

I mean this line:

AMR after fork (PID 34914): 0x0000000000000003

Shouldn't it be the same as in the parent process?

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote:
> On 05/20/2018 09:11 PM, Ram Pai wrote:
> >Florian,
> >
> >	Does the following patch fix the problem for you?  Just like x86
> >	I am enabling all keys in the UAMOR register during
> >	initialization itself. Hence any key created by any thread at
> >	any time, will get activated on all threads. So any thread
> >	can change the permission on that key. Smoke tested it
> >	with your test program.
> 
> I think this goes in the right direction, but the AMR value after
> fork is still strange:
> 
> AMR (PID 34912): 0x0000000000000000
> AMR after fork (PID 34913): 0x0000000000000000
> AMR (PID 34913): 0x0000000000000000
> Allocated key in subprocess (PID 34913): 2
> Allocated key (PID 34912): 2
> Setting AMR: 0xffffffffffffffff
> New AMR value (PID 34912): 0x0fffffffffffffff
> About to call execl (PID 34912) ...
> AMR (PID 34912): 0x0fffffffffffffff
> AMR after fork (PID 34914): 0x0000000000000003
> AMR (PID 34914): 0x0000000000000003
> Allocated key in subprocess (PID 34914): 2
> Allocated key (PID 34912): 2
> Setting AMR: 0xffffffffffffffff
> New AMR value (PID 34912): 0x0fffffffffffffff
> 
> I mean this line:
> 
> AMR after fork (PID 34914): 0x0000000000000003
> 
> Shouldn't it be the same as in the parent process?

Fixed it. Please try this patch. If it all works to your satisfaction, I
will clean it up further and send to Michael Ellermen(ppc maintainer).


commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527
Author: Ram Pai <linuxram@us.ibm.com>
Date:   Sun Jun 3 14:44:32 2018 -0500

    Fix for the fork bug.
    
    Signed-off-by: Ram Pai <linuxram@us.ibm.com>

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 1237f13..999dd08 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -582,6 +582,7 @@ static void save_all(struct task_struct *tsk)
 		__giveup_spe(tsk);
 
 	msr_check_and_clear(msr_all_available);
+	thread_pkey_regs_save(&tsk->thread);
 }
 
 void flush_all_to_thread(struct task_struct *tsk)
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index ab4519a..af6aa4a 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -294,6 +294,7 @@ void thread_pkey_regs_save(struct thread_struct *thread)
 	 */
 	thread->amr = read_amr();
 	thread->iamr = read_iamr();
+	thread->uamor = read_uamor();
 }
 
 void thread_pkey_regs_restore(struct thread_struct *new_thread,
@@ -315,9 +316,13 @@ void thread_pkey_regs_init(struct thread_struct *thread)
 	if (static_branch_likely(&pkey_disabled))
 		return;
 
-	thread->amr = read_amr() & pkey_amr_mask;
-	thread->iamr = read_iamr() & pkey_iamr_mask;
+	thread->amr = pkey_amr_mask;
+	thread->iamr = pkey_iamr_mask;
 	thread->uamor = pkey_uamor_mask;
+
+	write_uamor(pkey_uamor_mask);
+	write_amr(pkey_amr_mask);
+	write_iamr(pkey_iamr_mask);
 }
 
 static inline bool pkey_allows_readwrite(int pkey)


> 
> Thanks,
> Florian

-- 
Ram Pai

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/03/2018 10:18 PM, Ram Pai wrote:
> On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote:
>> On 05/20/2018 09:11 PM, Ram Pai wrote:
>>> Florian,
>>>
>>> 	Does the following patch fix the problem for you?  Just like x86
>>> 	I am enabling all keys in the UAMOR register during
>>> 	initialization itself. Hence any key created by any thread at
>>> 	any time, will get activated on all threads. So any thread
>>> 	can change the permission on that key. Smoke tested it
>>> 	with your test program.
>>
>> I think this goes in the right direction, but the AMR value after
>> fork is still strange:
>>
>> AMR (PID 34912): 0x0000000000000000
>> AMR after fork (PID 34913): 0x0000000000000000
>> AMR (PID 34913): 0x0000000000000000
>> Allocated key in subprocess (PID 34913): 2
>> Allocated key (PID 34912): 2
>> Setting AMR: 0xffffffffffffffff
>> New AMR value (PID 34912): 0x0fffffffffffffff
>> About to call execl (PID 34912) ...
>> AMR (PID 34912): 0x0fffffffffffffff
>> AMR after fork (PID 34914): 0x0000000000000003
>> AMR (PID 34914): 0x0000000000000003
>> Allocated key in subprocess (PID 34914): 2
>> Allocated key (PID 34912): 2
>> Setting AMR: 0xffffffffffffffff
>> New AMR value (PID 34912): 0x0fffffffffffffff
>>
>> I mean this line:
>>
>> AMR after fork (PID 34914): 0x0000000000000003
>>
>> Shouldn't it be the same as in the parent process?
> 
> Fixed it. Please try this patch. If it all works to your satisfaction, I
> will clean it up further and send to Michael Ellermen(ppc maintainer).
> 
> 
> commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527
> Author: Ram Pai <linuxram@us.ibm.com>
> Date:   Sun Jun 3 14:44:32 2018 -0500
> 
>      Fix for the fork bug.
>      
>      Signed-off-by: Ram Pai <linuxram@us.ibm.com>

Is this on top of the previous patch, or a separate fix?

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Mon, Jun 04, 2018 at 12:12:07PM +0200, Florian Weimer wrote:
> On 06/03/2018 10:18 PM, Ram Pai wrote:
> >On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote:
> >>On 05/20/2018 09:11 PM, Ram Pai wrote:
> >>>Florian,
> >>>
> >>>	Does the following patch fix the problem for you?  Just like x86
> >>>	I am enabling all keys in the UAMOR register during
> >>>	initialization itself. Hence any key created by any thread at
> >>>	any time, will get activated on all threads. So any thread
> >>>	can change the permission on that key. Smoke tested it
> >>>	with your test program.
> >>
> >>I think this goes in the right direction, but the AMR value after
> >>fork is still strange:
> >>
> >>AMR (PID 34912): 0x0000000000000000
> >>AMR after fork (PID 34913): 0x0000000000000000
> >>AMR (PID 34913): 0x0000000000000000
> >>Allocated key in subprocess (PID 34913): 2
> >>Allocated key (PID 34912): 2
> >>Setting AMR: 0xffffffffffffffff
> >>New AMR value (PID 34912): 0x0fffffffffffffff
> >>About to call execl (PID 34912) ...
> >>AMR (PID 34912): 0x0fffffffffffffff
> >>AMR after fork (PID 34914): 0x0000000000000003
> >>AMR (PID 34914): 0x0000000000000003
> >>Allocated key in subprocess (PID 34914): 2
> >>Allocated key (PID 34912): 2
> >>Setting AMR: 0xffffffffffffffff
> >>New AMR value (PID 34912): 0x0fffffffffffffff
> >>
> >>I mean this line:
> >>
> >>AMR after fork (PID 34914): 0x0000000000000003
> >>
> >>Shouldn't it be the same as in the parent process?
> >
> >Fixed it. Please try this patch. If it all works to your satisfaction, I
> >will clean it up further and send to Michael Ellermen(ppc maintainer).
> >
> >
> >commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527
> >Author: Ram Pai <linuxram@us.ibm.com>
> >Date:   Sun Jun 3 14:44:32 2018 -0500
> >
> >     Fix for the fork bug.
> >     Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> 
> Is this on top of the previous patch, or a separate fix?

top of previous patch.
RP

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/04/2018 04:01 PM, Ram Pai wrote:
> On Mon, Jun 04, 2018 at 12:12:07PM +0200, Florian Weimer wrote:
>> On 06/03/2018 10:18 PM, Ram Pai wrote:
>>> On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote:
>>>> On 05/20/2018 09:11 PM, Ram Pai wrote:
>>>>> Florian,
>>>>>
>>>>> 	Does the following patch fix the problem for you?  Just like x86
>>>>> 	I am enabling all keys in the UAMOR register during
>>>>> 	initialization itself. Hence any key created by any thread at
>>>>> 	any time, will get activated on all threads. So any thread
>>>>> 	can change the permission on that key. Smoke tested it
>>>>> 	with your test program.
>>>>
>>>> I think this goes in the right direction, but the AMR value after
>>>> fork is still strange:
>>>>
>>>> AMR (PID 34912): 0x0000000000000000
>>>> AMR after fork (PID 34913): 0x0000000000000000
>>>> AMR (PID 34913): 0x0000000000000000
>>>> Allocated key in subprocess (PID 34913): 2
>>>> Allocated key (PID 34912): 2
>>>> Setting AMR: 0xffffffffffffffff
>>>> New AMR value (PID 34912): 0x0fffffffffffffff
>>>> About to call execl (PID 34912) ...
>>>> AMR (PID 34912): 0x0fffffffffffffff
>>>> AMR after fork (PID 34914): 0x0000000000000003
>>>> AMR (PID 34914): 0x0000000000000003
>>>> Allocated key in subprocess (PID 34914): 2
>>>> Allocated key (PID 34912): 2
>>>> Setting AMR: 0xffffffffffffffff
>>>> New AMR value (PID 34912): 0x0fffffffffffffff
>>>>
>>>> I mean this line:
>>>>
>>>> AMR after fork (PID 34914): 0x0000000000000003
>>>>
>>>> Shouldn't it be the same as in the parent process?
>>>
>>> Fixed it. Please try this patch. If it all works to your satisfaction, I
>>> will clean it up further and send to Michael Ellermen(ppc maintainer).
>>>
>>>
>>> commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527
>>> Author: Ram Pai <linuxram@us.ibm.com>
>>> Date:   Sun Jun 3 14:44:32 2018 -0500
>>>
>>>      Fix for the fork bug.
>>>      Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>>
>> Is this on top of the previous patch, or a separate fix?
> 
> top of previous patch.

Thanks.  With this patch, I get this on an LPAR:

AMR (PID 1876): 0x0000000000000003
AMR after fork (PID 1877): 0x0000000000000003
AMR (PID 1877): 0x0000000000000003
Allocated key in subprocess (PID 1877): 2
Allocated key (PID 1876): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 1876): 0x0fffffffffffffff
About to call execl (PID 1876) ...
AMR (PID 1876): 0x0000000000000003
AMR after fork (PID 1878): 0x0000000000000003
AMR (PID 1878): 0x0000000000000003
Allocated key in subprocess (PID 1878): 2
Allocated key (PID 1876): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 1876): 0x0fffffffffffffff

Test program is still this one:

<https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/173198.html>

So the process starts out with a different AMR value for some reason. 
That could be a pre-existing bug that was just hidden by the 
reset-to-zero on fork, or it could be intentional.  But the kernel code 
does not indicate that key 63 is reserved (POWER numbers keys from the 
MSB to the LSB).

But it looks like we are finally getting somewhere. 8-)

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Mon, Jun 04, 2018 at 07:57:46PM +0200, Florian Weimer wrote:
> On 06/04/2018 04:01 PM, Ram Pai wrote:
> >On Mon, Jun 04, 2018 at 12:12:07PM +0200, Florian Weimer wrote:
> >>On 06/03/2018 10:18 PM, Ram Pai wrote:
> >>>On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote:
> >>>>On 05/20/2018 09:11 PM, Ram Pai wrote:
> >>>>>Florian,
> >>>>>
> >>>>>	Does the following patch fix the problem for you?  Just like x86
> >>>>>	I am enabling all keys in the UAMOR register during
> >>>>>	initialization itself. Hence any key created by any thread at
> >>>>>	any time, will get activated on all threads. So any thread
> >>>>>	can change the permission on that key. Smoke tested it
> >>>>>	with your test program.
> >>>>
> >>>>I think this goes in the right direction, but the AMR value after
> >>>>fork is still strange:
> >>>>
> >>>>AMR (PID 34912): 0x0000000000000000
> >>>>AMR after fork (PID 34913): 0x0000000000000000
> >>>>AMR (PID 34913): 0x0000000000000000
> >>>>Allocated key in subprocess (PID 34913): 2
> >>>>Allocated key (PID 34912): 2
> >>>>Setting AMR: 0xffffffffffffffff
> >>>>New AMR value (PID 34912): 0x0fffffffffffffff
> >>>>About to call execl (PID 34912) ...
> >>>>AMR (PID 34912): 0x0fffffffffffffff
> >>>>AMR after fork (PID 34914): 0x0000000000000003
> >>>>AMR (PID 34914): 0x0000000000000003
> >>>>Allocated key in subprocess (PID 34914): 2
> >>>>Allocated key (PID 34912): 2
> >>>>Setting AMR: 0xffffffffffffffff
> >>>>New AMR value (PID 34912): 0x0fffffffffffffff
> >>>>
> >>>>I mean this line:
> >>>>
> >>>>AMR after fork (PID 34914): 0x0000000000000003
> >>>>
> >>>>Shouldn't it be the same as in the parent process?
> >>>
> >>>Fixed it. Please try this patch. If it all works to your satisfaction, I
> >>>will clean it up further and send to Michael Ellermen(ppc maintainer).
> >>>
> >>>
> >>>commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527
> >>>Author: Ram Pai <linuxram@us.ibm.com>
> >>>Date:   Sun Jun 3 14:44:32 2018 -0500
> >>>
> >>>     Fix for the fork bug.
> >>>     Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >>
> >>Is this on top of the previous patch, or a separate fix?
> >
> >top of previous patch.
> 
> Thanks.  With this patch, I get this on an LPAR:
> 
> AMR (PID 1876): 0x0000000000000003
> AMR after fork (PID 1877): 0x0000000000000003
> AMR (PID 1877): 0x0000000000000003
> Allocated key in subprocess (PID 1877): 2
> Allocated key (PID 1876): 2
> Setting AMR: 0xffffffffffffffff
> New AMR value (PID 1876): 0x0fffffffffffffff
> About to call execl (PID 1876) ...
> AMR (PID 1876): 0x0000000000000003
> AMR after fork (PID 1878): 0x0000000000000003
> AMR (PID 1878): 0x0000000000000003
> Allocated key in subprocess (PID 1878): 2
> Allocated key (PID 1876): 2
> Setting AMR: 0xffffffffffffffff
> New AMR value (PID 1876): 0x0fffffffffffffff
> 
> Test program is still this one:
> 
> <https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/173198.html>
> 
> So the process starts out with a different AMR value for some
> reason. That could be a pre-existing bug that was just hidden by the
> reset-to-zero on fork, or it could be intentional.  But the kernel

yes it is a bug, a patch for which is lined up for submission..

The fix is


commit eaf5b2ac002ad2f5bca118d7ce075ce28311aa8e
Author: Ram Pai <linuxram@us.ibm.com>
Date:   Mon Jun 4 10:58:44 2018 -0500

    powerpc/pkeys: fix total pkeys calculation
    
    Total number of pkeys calculation is off by 1. Fix it.
    
    Signed-off-by: Ram Pai <linuxram@us.ibm.com>

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 4530cdf..3384c4e 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -93,7 +93,7 @@ int pkey_initialize(void)
 	 * arch-neutral code.
 	 */
 	pkeys_total = min_t(int, pkeys_total,
-			(ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT));
+			((ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT)+1));
 
 	if (!pkey_mmu_enabled() || radix_enabled() || !pkeys_total)
 		static_branch_enable(&pkey_disabled);

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Mon, Jun 04, 2018 at 07:57:46PM +0200, Florian Weimer wrote:
> On 06/04/2018 04:01 PM, Ram Pai wrote:
> >On Mon, Jun 04, 2018 at 12:12:07PM +0200, Florian Weimer wrote:
> >>On 06/03/2018 10:18 PM, Ram Pai wrote:
> >>>On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote:
> >>>>On 05/20/2018 09:11 PM, Ram Pai wrote:
> >>>>>Florian,
> >>>>>
> >>>>>	Does the following patch fix the problem for you?  Just like x86
> >>>>>	I am enabling all keys in the UAMOR register during
> >>>>>	initialization itself. Hence any key created by any thread at
> >>>>>	any time, will get activated on all threads. So any thread
> >>>>>	can change the permission on that key. Smoke tested it
> >>>>>	with your test program.
> >>>>
> >>>>I think this goes in the right direction, but the AMR value after
> >>>>fork is still strange:
> >>>>
> >>>>AMR (PID 34912): 0x0000000000000000
> >>>>AMR after fork (PID 34913): 0x0000000000000000
> >>>>AMR (PID 34913): 0x0000000000000000
> >>>>Allocated key in subprocess (PID 34913): 2
> >>>>Allocated key (PID 34912): 2
> >>>>Setting AMR: 0xffffffffffffffff
> >>>>New AMR value (PID 34912): 0x0fffffffffffffff
> >>>>About to call execl (PID 34912) ...
> >>>>AMR (PID 34912): 0x0fffffffffffffff
> >>>>AMR after fork (PID 34914): 0x0000000000000003
> >>>>AMR (PID 34914): 0x0000000000000003
> >>>>Allocated key in subprocess (PID 34914): 2
> >>>>Allocated key (PID 34912): 2
> >>>>Setting AMR: 0xffffffffffffffff
> >>>>New AMR value (PID 34912): 0x0fffffffffffffff
> >>>>
> >>>>I mean this line:
> >>>>
> >>>>AMR after fork (PID 34914): 0x0000000000000003
> >>>>
> >>>>Shouldn't it be the same as in the parent process?
> >>>
> >>>Fixed it. Please try this patch. If it all works to your satisfaction,=
 I
> >>>will clean it up further and send to Michael Ellermen(ppc maintainer).
> >>>
> >>>
> >>>commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527
> >>>Author: Ram Pai <linuxram@us.ibm.com>
> >>>Date:   Sun Jun 3 14:44:32 2018 -0500
> >>>
> >>>     Fix for the fork bug.
> >>>     Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >>
> >>Is this on top of the previous patch, or a separate fix?
> >
> >top of previous patch.
>=20
> Thanks.  With this patch, I get this on an LPAR:
>=20
> AMR (PID 1876): 0x0000000000000003
> AMR after fork (PID 1877): 0x0000000000000003
> AMR (PID 1877): 0x0000000000000003
> Allocated key in subprocess (PID 1877): 2
> Allocated key (PID 1876): 2
> Setting AMR: 0xffffffffffffffff
> New AMR value (PID 1876): 0x0fffffffffffffff
> About to call execl (PID 1876) ...
> AMR (PID 1876): 0x0000000000000003
> AMR after fork (PID 1878): 0x0000000000000003
> AMR (PID 1878): 0x0000000000000003
> Allocated key in subprocess (PID 1878): 2
> Allocated key (PID 1876): 2
> Setting AMR: 0xffffffffffffffff
> New AMR value (PID 1876): 0x0fffffffffffffff
>=20
> Test program is still this one:
>=20
> <https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/173198.html>
>=20
> So the process starts out with a different AMR value for some
> reason. That could be a pre-existing bug that was just hidden by the
> reset-to-zero on fork, or it could be intentional.  But the kernel

yes it is a bug, a patch for which is lined up for submission..

The fix is


commit eaf5b2ac002ad2f5bca118d7ce075ce28311aa8e
Author: Ram Pai <linuxram@us.ibm.com>
Date:   Mon Jun 4 10:58:44 2018 -0500

    powerpc/pkeys: fix total pkeys calculation
=20=20=20=20
    Total number of pkeys calculation is off by 1. Fix it.
=20=20=20=20
    Signed-off-by: Ram Pai <linuxram@us.ibm.com>

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 4530cdf..3384c4e 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -93,7 +93,7 @@ int pkey_initialize(void)
 	 * arch-neutral code.
 	 */
 	pkeys_total =3D min_t(int, pkeys_total,
-			(ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT));
+			((ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT)+1));
=20
 	if (!pkey_mmu_enabled() || radix_enabled() || !pkeys_total)
 		static_branch_enable(&pkey_disabled);

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/04/2018 09:02 PM, Ram Pai wrote:
> On Mon, Jun 04, 2018 at 07:57:46PM +0200, Florian Weimer wrote:
>> On 06/04/2018 04:01 PM, Ram Pai wrote:
>>> On Mon, Jun 04, 2018 at 12:12:07PM +0200, Florian Weimer wrote:
>>>> On 06/03/2018 10:18 PM, Ram Pai wrote:
>>>>> On Mon, May 21, 2018 at 01:29:11PM +0200, Florian Weimer wrote:
>>>>>> On 05/20/2018 09:11 PM, Ram Pai wrote:
>>>>>>> Florian,
>>>>>>>
>>>>>>> 	Does the following patch fix the problem for you?  Just like x86
>>>>>>> 	I am enabling all keys in the UAMOR register during
>>>>>>> 	initialization itself. Hence any key created by any thread at
>>>>>>> 	any time, will get activated on all threads. So any thread
>>>>>>> 	can change the permission on that key. Smoke tested it
>>>>>>> 	with your test program.
>>>>>>
>>>>>> I think this goes in the right direction, but the AMR value after
>>>>>> fork is still strange:
>>>>>>
>>>>>> AMR (PID 34912): 0x0000000000000000
>>>>>> AMR after fork (PID 34913): 0x0000000000000000
>>>>>> AMR (PID 34913): 0x0000000000000000
>>>>>> Allocated key in subprocess (PID 34913): 2
>>>>>> Allocated key (PID 34912): 2
>>>>>> Setting AMR: 0xffffffffffffffff
>>>>>> New AMR value (PID 34912): 0x0fffffffffffffff
>>>>>> About to call execl (PID 34912) ...
>>>>>> AMR (PID 34912): 0x0fffffffffffffff
>>>>>> AMR after fork (PID 34914): 0x0000000000000003
>>>>>> AMR (PID 34914): 0x0000000000000003
>>>>>> Allocated key in subprocess (PID 34914): 2
>>>>>> Allocated key (PID 34912): 2
>>>>>> Setting AMR: 0xffffffffffffffff
>>>>>> New AMR value (PID 34912): 0x0fffffffffffffff
>>>>>>
>>>>>> I mean this line:
>>>>>>
>>>>>> AMR after fork (PID 34914): 0x0000000000000003
>>>>>>
>>>>>> Shouldn't it be the same as in the parent process?
>>>>>
>>>>> Fixed it. Please try this patch. If it all works to your satisfaction, I
>>>>> will clean it up further and send to Michael Ellermen(ppc maintainer).
>>>>>
>>>>>
>>>>> commit 51f4208ed5baeab1edb9b0f8b68d7144449b3527
>>>>> Author: Ram Pai <linuxram@us.ibm.com>
>>>>> Date:   Sun Jun 3 14:44:32 2018 -0500
>>>>>
>>>>>      Fix for the fork bug.
>>>>>      Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>>>>
>>>> Is this on top of the previous patch, or a separate fix?
>>>
>>> top of previous patch.
>>
>> Thanks.  With this patch, I get this on an LPAR:
>>
>> AMR (PID 1876): 0x0000000000000003
>> AMR after fork (PID 1877): 0x0000000000000003
>> AMR (PID 1877): 0x0000000000000003
>> Allocated key in subprocess (PID 1877): 2
>> Allocated key (PID 1876): 2
>> Setting AMR: 0xffffffffffffffff
>> New AMR value (PID 1876): 0x0fffffffffffffff
>> About to call execl (PID 1876) ...
>> AMR (PID 1876): 0x0000000000000003
>> AMR after fork (PID 1878): 0x0000000000000003
>> AMR (PID 1878): 0x0000000000000003
>> Allocated key in subprocess (PID 1878): 2
>> Allocated key (PID 1876): 2
>> Setting AMR: 0xffffffffffffffff
>> New AMR value (PID 1876): 0x0fffffffffffffff
>>
>> Test program is still this one:
>>
>> <https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/173198.html>
>>
>> So the process starts out with a different AMR value for some
>> reason. That could be a pre-existing bug that was just hidden by the
>> reset-to-zero on fork, or it could be intentional.  But the kernel
> 
> yes it is a bug, a patch for which is lined up for submission..
> 
> The fix is
> 
> 
> commit eaf5b2ac002ad2f5bca118d7ce075ce28311aa8e
> Author: Ram Pai <linuxram@us.ibm.com>
> Date:   Mon Jun 4 10:58:44 2018 -0500
> 
>      powerpc/pkeys: fix total pkeys calculation
>      
>      Total number of pkeys calculation is off by 1. Fix it.
>      
>      Signed-off-by: Ram Pai <linuxram@us.ibm.com>

Looks good to me now.  Initial AMR value is zero, as is currently intended.

So the remaining question at this point is whether the Intel behavior 
(default-deny instead of default-allow) is preferable.

But if you can get the existing fixes into 4.18 and perhaps the relevant 
stable kernels, that would already be a great help for my glibc work.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

> 
> So the remaining question at this point is whether the Intel
> behavior (default-deny instead of default-allow) is preferable.

Florian, remind me what behavior needs to fixed? 

-- 
Ram Pai

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/08/2018 04:34 AM, Ram Pai wrote:
>>
>> So the remaining question at this point is whether the Intel
>> behavior (default-deny instead of default-allow) is preferable.
> 
> Florian, remind me what behavior needs to fixed?

See the other thread.  The Intel register equivalent to the AMR by 
default disallows access to yet-unallocated keys, so that threads which 
are created before key allocation do not magically gain access to a key 
allocated by another thread.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Michal Suchánek]

On Fri, 8 Jun 2018 07:53:51 +0200
Florian Weimer <fweimer@redhat.com> wrote:

> On 06/08/2018 04:34 AM, Ram Pai wrote:
> >>
> >> So the remaining question at this point is whether the Intel
> >> behavior (default-deny instead of default-allow) is preferable.  
> > 
> > Florian, remind me what behavior needs to fixed?  
> 
> See the other thread.  The Intel register equivalent to the AMR by 
> default disallows access to yet-unallocated keys, so that threads
> which are created before key allocation do not magically gain access
> to a key allocated by another thread.
> 

That does not make any sense. The threads share the address space so
they should also share the keys.

Or in other words the keys are supposed to be acceleration of
mprotect() so if mprotect() magically gives access to threads that did
not call it so should pkey functions. If they cannot do that then they
fail the primary purpose.

And in any case what semantic that makes sense will you ever get by
threads not magically getting new keys?

Suppose you will spawn some threads, then allocate a new key, associate
it with a piece of protected data, etc.

Now the old threads do not have access to the protected data at all. So
if they want to access it they have to call into a management thread
that created the access key to give them the data. Which means they
need to call into the kernel to switch to the management thread. Which
completely defeats the purpose of the acceleration of mprotect() which
was to avoid calling into the kernel to access the data. In other words
you can as well spawn a management process and use shared memory.

What's worse, if you wanted to opt out of this feature and give the old
threads the new key that's going to be quite a bit of fiddling.

That said there might be an enhancement that kind of makes sense. For
example if you allocate a key to associate with an area that you want
to read most of the time and update occasionally it might make sense to
tell the kernel: make the read bit on this key process-global, make the
write bit on this key thread-local, and do not allow setting the
execute bit at all. Then a thread calling a function to update the data
will get write access but other threads will continue to have read
access unless they also call a function to update the data.

Thanks

Michal

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/08/2018 12:15 PM, Michal SuchA!nek wrote:
> On Fri, 8 Jun 2018 07:53:51 +0200
> Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 06/08/2018 04:34 AM, Ram Pai wrote:
>>>>
>>>> So the remaining question at this point is whether the Intel
>>>> behavior (default-deny instead of default-allow) is preferable.
>>>
>>> Florian, remind me what behavior needs to fixed?
>>
>> See the other thread.  The Intel register equivalent to the AMR by
>> default disallows access to yet-unallocated keys, so that threads
>> which are created before key allocation do not magically gain access
>> to a key allocated by another thread.
>>
> 
> That does not make any sense. The threads share the address space so
> they should also share the keys.
> 
> Or in other words the keys are supposed to be acceleration of
> mprotect() so if mprotect() magically gives access to threads that did
> not call it so should pkey functions. If they cannot do that then they
> fail the primary purpose.

That's not how protection keys work.  The access rights are 
thread-specific, so that you can change them locally, without 
synchronization and expensive inter-node communication.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/08/2018 12:15 PM, Michal Suchánek wrote:
> On Fri, 8 Jun 2018 07:53:51 +0200
> Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 06/08/2018 04:34 AM, Ram Pai wrote:
>>>>
>>>> So the remaining question at this point is whether the Intel
>>>> behavior (default-deny instead of default-allow) is preferable.
>>>
>>> Florian, remind me what behavior needs to fixed?
>>
>> See the other thread.  The Intel register equivalent to the AMR by
>> default disallows access to yet-unallocated keys, so that threads
>> which are created before key allocation do not magically gain access
>> to a key allocated by another thread.
>>
> 
> That does not make any sense. The threads share the address space so
> they should also share the keys.
> 
> Or in other words the keys are supposed to be acceleration of
> mprotect() so if mprotect() magically gives access to threads that did
> not call it so should pkey functions. If they cannot do that then they
> fail the primary purpose.

That's not how protection keys work.  The access rights are 
thread-specific, so that you can change them locally, without 
synchronization and expensive inter-node communication.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Michal Suchánek]

On Fri, 8 Jun 2018 12:44:53 +0200
Florian Weimer <fweimer@redhat.com> wrote:

> On 06/08/2018 12:15 PM, Michal Suchánek wrote:
> > On Fri, 8 Jun 2018 07:53:51 +0200
> > Florian Weimer <fweimer@redhat.com> wrote:
> >   
> >> On 06/08/2018 04:34 AM, Ram Pai wrote:  
> >>>>
> >>>> So the remaining question at this point is whether the Intel
> >>>> behavior (default-deny instead of default-allow) is preferable.  
> >>>
> >>> Florian, remind me what behavior needs to fixed?  
> >>
> >> See the other thread.  The Intel register equivalent to the AMR by
> >> default disallows access to yet-unallocated keys, so that threads
> >> which are created before key allocation do not magically gain
> >> access to a key allocated by another thread.
> >>  
> > 
> > That does not make any sense. The threads share the address space so
> > they should also share the keys.
> > 
> > Or in other words the keys are supposed to be acceleration of
> > mprotect() so if mprotect() magically gives access to threads that
> > did not call it so should pkey functions. If they cannot do that
> > then they fail the primary purpose.  
> 
> That's not how protection keys work.  The access rights are 
> thread-specific, so that you can change them locally, without 
> synchronization and expensive inter-node communication.
> 

And the association of a key with part of the address space is
thread-local as well?

Thanks

Michal

Re: pkeys on POWER: Access rights not reset on execve

[Michal Suchánek]

On Fri, 8 Jun 2018 12:44:53 +0200
Florian Weimer <fweimer@redhat.com> wrote:

> On 06/08/2018 12:15 PM, Michal Such=C3=A1nek wrote:
> > On Fri, 8 Jun 2018 07:53:51 +0200
> > Florian Weimer <fweimer@redhat.com> wrote:
> >  =20
> >> On 06/08/2018 04:34 AM, Ram Pai wrote: =20
> >>>>
> >>>> So the remaining question at this point is whether the Intel
> >>>> behavior (default-deny instead of default-allow) is preferable. =20
> >>>
> >>> Florian, remind me what behavior needs to fixed? =20
> >>
> >> See the other thread.  The Intel register equivalent to the AMR by
> >> default disallows access to yet-unallocated keys, so that threads
> >> which are created before key allocation do not magically gain
> >> access to a key allocated by another thread.
> >> =20
> >=20
> > That does not make any sense. The threads share the address space so
> > they should also share the keys.
> >=20
> > Or in other words the keys are supposed to be acceleration of
> > mprotect() so if mprotect() magically gives access to threads that
> > did not call it so should pkey functions. If they cannot do that
> > then they fail the primary purpose. =20
>=20
> That's not how protection keys work.  The access rights are=20
> thread-specific, so that you can change them locally, without=20
> synchronization and expensive inter-node communication.
>=20

And the association of a key with part of the address space is
thread-local as well?

Thanks

Michal

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/08/2018 02:54 PM, Michal SuchA!nek wrote:
> On Fri, 8 Jun 2018 12:44:53 +0200
> Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 06/08/2018 12:15 PM, Michal SuchA!nek wrote:
>>> On Fri, 8 Jun 2018 07:53:51 +0200
>>> Florian Weimer <fweimer@redhat.com> wrote:
>>>    
>>>> On 06/08/2018 04:34 AM, Ram Pai wrote:
>>>>>>
>>>>>> So the remaining question at this point is whether the Intel
>>>>>> behavior (default-deny instead of default-allow) is preferable.
>>>>>
>>>>> Florian, remind me what behavior needs to fixed?
>>>>
>>>> See the other thread.  The Intel register equivalent to the AMR by
>>>> default disallows access to yet-unallocated keys, so that threads
>>>> which are created before key allocation do not magically gain
>>>> access to a key allocated by another thread.
>>>>   
>>>
>>> That does not make any sense. The threads share the address space so
>>> they should also share the keys.
>>>
>>> Or in other words the keys are supposed to be acceleration of
>>> mprotect() so if mprotect() magically gives access to threads that
>>> did not call it so should pkey functions. If they cannot do that
>>> then they fail the primary purpose.
>>
>> That's not how protection keys work.  The access rights are
>> thread-specific, so that you can change them locally, without
>> synchronization and expensive inter-node communication.
>>
> 
> And the association of a key with part of the address space is
> thread-local as well?

No, that part is still per-process.

Florian

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/08/2018 02:54 PM, Michal Suchánek wrote:
> On Fri, 8 Jun 2018 12:44:53 +0200
> Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 06/08/2018 12:15 PM, Michal Suchánek wrote:
>>> On Fri, 8 Jun 2018 07:53:51 +0200
>>> Florian Weimer <fweimer@redhat.com> wrote:
>>>    
>>>> On 06/08/2018 04:34 AM, Ram Pai wrote:
>>>>>>
>>>>>> So the remaining question at this point is whether the Intel
>>>>>> behavior (default-deny instead of default-allow) is preferable.
>>>>>
>>>>> Florian, remind me what behavior needs to fixed?
>>>>
>>>> See the other thread.  The Intel register equivalent to the AMR by
>>>> default disallows access to yet-unallocated keys, so that threads
>>>> which are created before key allocation do not magically gain
>>>> access to a key allocated by another thread.
>>>>   
>>>
>>> That does not make any sense. The threads share the address space so
>>> they should also share the keys.
>>>
>>> Or in other words the keys are supposed to be acceleration of
>>> mprotect() so if mprotect() magically gives access to threads that
>>> did not call it so should pkey functions. If they cannot do that
>>> then they fail the primary purpose.
>>
>> That's not how protection keys work.  The access rights are
>> thread-specific, so that you can change them locally, without
>> synchronization and expensive inter-node communication.
>>
> 
> And the association of a key with part of the address space is
> thread-local as well?

No, that part is still per-process.

Florian

Re: pkeys on POWER: Access rights not reset on execve

[Michal Suchánek]

On Fri, 8 Jun 2018 14:57:06 +0200
Florian Weimer <fweimer@redhat.com> wrote:

> On 06/08/2018 02:54 PM, Michal Suchánek wrote:
> > On Fri, 8 Jun 2018 12:44:53 +0200
> > Florian Weimer <fweimer@redhat.com> wrote:
> >   
> >> On 06/08/2018 12:15 PM, Michal Suchánek wrote:  
> >>> On Fri, 8 Jun 2018 07:53:51 +0200
> >>> Florian Weimer <fweimer@redhat.com> wrote:
> >>>      
> >>>> On 06/08/2018 04:34 AM, Ram Pai wrote:  
> >>>>>>
> >>>>>> So the remaining question at this point is whether the Intel
> >>>>>> behavior (default-deny instead of default-allow) is
> >>>>>> preferable.  
> >>>>>
> >>>>> Florian, remind me what behavior needs to fixed?  
> >>>>
> >>>> See the other thread.  The Intel register equivalent to the AMR
> >>>> by default disallows access to yet-unallocated keys, so that
> >>>> threads which are created before key allocation do not magically
> >>>> gain access to a key allocated by another thread.
> >>>>     
> >>>
> >>> That does not make any sense. The threads share the address space
> >>> so they should also share the keys.
> >>>
> >>> Or in other words the keys are supposed to be acceleration of
> >>> mprotect() so if mprotect() magically gives access to threads that
> >>> did not call it so should pkey functions. If they cannot do that
> >>> then they fail the primary purpose.  
> >>
> >> That's not how protection keys work.  The access rights are
> >> thread-specific, so that you can change them locally, without
> >> synchronization and expensive inter-node communication.
> >>  
> > 
> > And the association of a key with part of the address space is
> > thread-local as well?  
> 
> No, that part is still per-process.

So as said above it does not make sense to make keys per-thread.

Thanks

Michal

Re: pkeys on POWER: Access rights not reset on execve

[Michal Suchánek]

On Fri, 8 Jun 2018 14:57:06 +0200
Florian Weimer <fweimer@redhat.com> wrote:

> On 06/08/2018 02:54 PM, Michal Such=C3=A1nek wrote:
> > On Fri, 8 Jun 2018 12:44:53 +0200
> > Florian Weimer <fweimer@redhat.com> wrote:
> >  =20
> >> On 06/08/2018 12:15 PM, Michal Such=C3=A1nek wrote: =20
> >>> On Fri, 8 Jun 2018 07:53:51 +0200
> >>> Florian Weimer <fweimer@redhat.com> wrote:
> >>>     =20
> >>>> On 06/08/2018 04:34 AM, Ram Pai wrote: =20
> >>>>>>
> >>>>>> So the remaining question at this point is whether the Intel
> >>>>>> behavior (default-deny instead of default-allow) is
> >>>>>> preferable. =20
> >>>>>
> >>>>> Florian, remind me what behavior needs to fixed? =20
> >>>>
> >>>> See the other thread.  The Intel register equivalent to the AMR
> >>>> by default disallows access to yet-unallocated keys, so that
> >>>> threads which are created before key allocation do not magically
> >>>> gain access to a key allocated by another thread.
> >>>>    =20
> >>>
> >>> That does not make any sense. The threads share the address space
> >>> so they should also share the keys.
> >>>
> >>> Or in other words the keys are supposed to be acceleration of
> >>> mprotect() so if mprotect() magically gives access to threads that
> >>> did not call it so should pkey functions. If they cannot do that
> >>> then they fail the primary purpose. =20
> >>
> >> That's not how protection keys work.  The access rights are
> >> thread-specific, so that you can change them locally, without
> >> synchronization and expensive inter-node communication.
> >> =20
> >=20
> > And the association of a key with part of the address space is
> > thread-local as well? =20
>=20
> No, that part is still per-process.

So as said above it does not make sense to make keys per-thread.

Thanks

Michal

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/08/2018 03:49 PM, Michal SuchA!nek wrote:
> On Fri, 8 Jun 2018 14:57:06 +0200
> Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 06/08/2018 02:54 PM, Michal SuchA!nek wrote:
>>> On Fri, 8 Jun 2018 12:44:53 +0200
>>> Florian Weimer <fweimer@redhat.com> wrote:
>>>    
>>>> On 06/08/2018 12:15 PM, Michal SuchA!nek wrote:
>>>>> On Fri, 8 Jun 2018 07:53:51 +0200
>>>>> Florian Weimer <fweimer@redhat.com> wrote:
>>>>>       
>>>>>> On 06/08/2018 04:34 AM, Ram Pai wrote:
>>>>>>>>
>>>>>>>> So the remaining question at this point is whether the Intel
>>>>>>>> behavior (default-deny instead of default-allow) is
>>>>>>>> preferable.
>>>>>>>
>>>>>>> Florian, remind me what behavior needs to fixed?
>>>>>>
>>>>>> See the other thread.  The Intel register equivalent to the AMR
>>>>>> by default disallows access to yet-unallocated keys, so that
>>>>>> threads which are created before key allocation do not magically
>>>>>> gain access to a key allocated by another thread.
>>>>>>      
>>>>>
>>>>> That does not make any sense. The threads share the address space
>>>>> so they should also share the keys.
>>>>>
>>>>> Or in other words the keys are supposed to be acceleration of
>>>>> mprotect() so if mprotect() magically gives access to threads that
>>>>> did not call it so should pkey functions. If they cannot do that
>>>>> then they fail the primary purpose.
>>>>
>>>> That's not how protection keys work.  The access rights are
>>>> thread-specific, so that you can change them locally, without
>>>> synchronization and expensive inter-node communication.
>>>>   
>>>
>>> And the association of a key with part of the address space is
>>> thread-local as well?
>>
>> No, that part is still per-process.
> 
> So as said above it does not make sense to make keys per-thread.

The keys are still global, but the access rights are per-thread and have 
to be for reliability reasons.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/08/2018 03:49 PM, Michal Suchánek wrote:
> On Fri, 8 Jun 2018 14:57:06 +0200
> Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 06/08/2018 02:54 PM, Michal Suchánek wrote:
>>> On Fri, 8 Jun 2018 12:44:53 +0200
>>> Florian Weimer <fweimer@redhat.com> wrote:
>>>    
>>>> On 06/08/2018 12:15 PM, Michal Suchánek wrote:
>>>>> On Fri, 8 Jun 2018 07:53:51 +0200
>>>>> Florian Weimer <fweimer@redhat.com> wrote:
>>>>>       
>>>>>> On 06/08/2018 04:34 AM, Ram Pai wrote:
>>>>>>>>
>>>>>>>> So the remaining question at this point is whether the Intel
>>>>>>>> behavior (default-deny instead of default-allow) is
>>>>>>>> preferable.
>>>>>>>
>>>>>>> Florian, remind me what behavior needs to fixed?
>>>>>>
>>>>>> See the other thread.  The Intel register equivalent to the AMR
>>>>>> by default disallows access to yet-unallocated keys, so that
>>>>>> threads which are created before key allocation do not magically
>>>>>> gain access to a key allocated by another thread.
>>>>>>      
>>>>>
>>>>> That does not make any sense. The threads share the address space
>>>>> so they should also share the keys.
>>>>>
>>>>> Or in other words the keys are supposed to be acceleration of
>>>>> mprotect() so if mprotect() magically gives access to threads that
>>>>> did not call it so should pkey functions. If they cannot do that
>>>>> then they fail the primary purpose.
>>>>
>>>> That's not how protection keys work.  The access rights are
>>>> thread-specific, so that you can change them locally, without
>>>> synchronization and expensive inter-node communication.
>>>>   
>>>
>>> And the association of a key with part of the address space is
>>> thread-local as well?
>>
>> No, that part is still per-process.
> 
> So as said above it does not make sense to make keys per-thread.

The keys are still global, but the access rights are per-thread and have 
to be for reliability reasons.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Michal Suchánek]

On Fri, 8 Jun 2018 15:51:03 +0200
Florian Weimer <fweimer@redhat.com> wrote:

> On 06/08/2018 03:49 PM, Michal Suchánek wrote:
> > On Fri, 8 Jun 2018 14:57:06 +0200
> > Florian Weimer <fweimer@redhat.com> wrote:
> >   
> >> On 06/08/2018 02:54 PM, Michal Suchánek wrote:  
> >>> On Fri, 8 Jun 2018 12:44:53 +0200
> >>> Florian Weimer <fweimer@redhat.com> wrote:
> >>>      
> >>>> On 06/08/2018 12:15 PM, Michal Suchánek wrote:  
> >>>>> On Fri, 8 Jun 2018 07:53:51 +0200
> >>>>> Florian Weimer <fweimer@redhat.com> wrote:
> >>>>>         
> >>>>>> On 06/08/2018 04:34 AM, Ram Pai wrote:  
> >>>>>>>>
> >>>>>>>> So the remaining question at this point is whether the Intel
> >>>>>>>> behavior (default-deny instead of default-allow) is
> >>>>>>>> preferable.  
> >>>>>>>
> >>>>>>> Florian, remind me what behavior needs to fixed?  
> >>>>>>
> >>>>>> See the other thread.  The Intel register equivalent to the AMR
> >>>>>> by default disallows access to yet-unallocated keys, so that
> >>>>>> threads which are created before key allocation do not
> >>>>>> magically gain access to a key allocated by another thread.
> >>>>>>        
> >>>>>
> >>>>> That does not make any sense. The threads share the address
> >>>>> space so they should also share the keys.
> >>>>>
> >>>>> Or in other words the keys are supposed to be acceleration of
> >>>>> mprotect() so if mprotect() magically gives access to threads
> >>>>> that did not call it so should pkey functions. If they cannot
> >>>>> do that then they fail the primary purpose.  
> >>>>
> >>>> That's not how protection keys work.  The access rights are
> >>>> thread-specific, so that you can change them locally, without
> >>>> synchronization and expensive inter-node communication.
> >>>>     
> >>>
> >>> And the association of a key with part of the address space is
> >>> thread-local as well?  
> >>
> >> No, that part is still per-process.  
> > 
> > So as said above it does not make sense to make keys per-thread.  
> 
> The keys are still global, but the access rights are per-thread and
> have to be for reliability reasons.
> 

Oh, right. The association of keys to memory is independent of key
allocation. However, to change the key permissions or the memory
association to a key you need to allocate it. And key allocation is
propagated lazily between threads so you do not have to stop the world
to allocate a key. So if default key permissions of an unallocated
key allow access then allocating a key and associating it with memory
makes that memory accessible to threads that are not yet aware of the
fact the key has been allocated which is not desirable.

Sounds sensible.

Thanks

Michal

Re: pkeys on POWER: Access rights not reset on execve

[Michal Suchánek]

On Fri, 8 Jun 2018 15:51:03 +0200
Florian Weimer <fweimer@redhat.com> wrote:

> On 06/08/2018 03:49 PM, Michal Such=C3=A1nek wrote:
> > On Fri, 8 Jun 2018 14:57:06 +0200
> > Florian Weimer <fweimer@redhat.com> wrote:
> >  =20
> >> On 06/08/2018 02:54 PM, Michal Such=C3=A1nek wrote: =20
> >>> On Fri, 8 Jun 2018 12:44:53 +0200
> >>> Florian Weimer <fweimer@redhat.com> wrote:
> >>>     =20
> >>>> On 06/08/2018 12:15 PM, Michal Such=C3=A1nek wrote: =20
> >>>>> On Fri, 8 Jun 2018 07:53:51 +0200
> >>>>> Florian Weimer <fweimer@redhat.com> wrote:
> >>>>>        =20
> >>>>>> On 06/08/2018 04:34 AM, Ram Pai wrote: =20
> >>>>>>>>
> >>>>>>>> So the remaining question at this point is whether the Intel
> >>>>>>>> behavior (default-deny instead of default-allow) is
> >>>>>>>> preferable. =20
> >>>>>>>
> >>>>>>> Florian, remind me what behavior needs to fixed? =20
> >>>>>>
> >>>>>> See the other thread.  The Intel register equivalent to the AMR
> >>>>>> by default disallows access to yet-unallocated keys, so that
> >>>>>> threads which are created before key allocation do not
> >>>>>> magically gain access to a key allocated by another thread.
> >>>>>>       =20
> >>>>>
> >>>>> That does not make any sense. The threads share the address
> >>>>> space so they should also share the keys.
> >>>>>
> >>>>> Or in other words the keys are supposed to be acceleration of
> >>>>> mprotect() so if mprotect() magically gives access to threads
> >>>>> that did not call it so should pkey functions. If they cannot
> >>>>> do that then they fail the primary purpose. =20
> >>>>
> >>>> That's not how protection keys work.  The access rights are
> >>>> thread-specific, so that you can change them locally, without
> >>>> synchronization and expensive inter-node communication.
> >>>>    =20
> >>>
> >>> And the association of a key with part of the address space is
> >>> thread-local as well? =20
> >>
> >> No, that part is still per-process. =20
> >=20
> > So as said above it does not make sense to make keys per-thread. =20
>=20
> The keys are still global, but the access rights are per-thread and
> have to be for reliability reasons.
>=20

Oh, right. The association of keys to memory is independent of key
allocation. However, to change the key permissions or the memory
association to a key you need to allocate it. And key allocation is
propagated lazily between threads so you do not have to stop the world
to allocate a key. So if default key permissions of an unallocated
key allow access then allocating a key and associating it with memory
makes that memory accessible to threads that are not yet aware of the
fact the key has been allocated which is not desirable.

Sounds sensible.

Thanks

Michal

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Fri, Jun 08, 2018 at 07:53:51AM +0200, Florian Weimer wrote:
> On 06/08/2018 04:34 AM, Ram Pai wrote:
> >>
> >>So the remaining question at this point is whether the Intel
> >>behavior (default-deny instead of default-allow) is preferable.
> >
> >Florian, remind me what behavior needs to fixed?
> 
> See the other thread.  The Intel register equivalent to the AMR by
> default disallows access to yet-unallocated keys, so that threads
> which are created before key allocation do not magically gain access
> to a key allocated by another thread.

Are you referring to the thread
'[PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics'

If yes, I will wait for your next version of the patch.

Otherwise please point me to the URL of that thread. Sorry and thankx. :)
RP

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/11/2018 07:23 PM, Ram Pai wrote:
> On Fri, Jun 08, 2018 at 07:53:51AM +0200, Florian Weimer wrote:
>> On 06/08/2018 04:34 AM, Ram Pai wrote:
>>>>
>>>> So the remaining question at this point is whether the Intel
>>>> behavior (default-deny instead of default-allow) is preferable.
>>>
>>> Florian, remind me what behavior needs to fixed?
>>
>> See the other thread.  The Intel register equivalent to the AMR by
>> default disallows access to yet-unallocated keys, so that threads
>> which are created before key allocation do not magically gain access
>> to a key allocated by another thread.
> 
> Are you referring to the thread
> '[PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics'

> Otherwise please point me to the URL of that thread. Sorry and thankx. :)

No, it's this issue:

   <https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-May/173157.html>

The UAMOR part has been fixed (thanks), but I think processes still 
start out with default-allow AMR.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Ram Pai]

On Mon, Jun 11, 2018 at 07:29:33PM +0200, Florian Weimer wrote:
> On 06/11/2018 07:23 PM, Ram Pai wrote:
> >On Fri, Jun 08, 2018 at 07:53:51AM +0200, Florian Weimer wrote:
> >>On 06/08/2018 04:34 AM, Ram Pai wrote:
> >>>>
> >>>>So the remaining question at this point is whether the Intel
> >>>>behavior (default-deny instead of default-allow) is preferable.
> >>>
> >>>Florian, remind me what behavior needs to fixed?
> >>
> >>See the other thread.  The Intel register equivalent to the AMR by
> >>default disallows access to yet-unallocated keys, so that threads
> >>which are created before key allocation do not magically gain access
> >>to a key allocated by another thread.
> >
> >Are you referring to the thread
> >'[PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics'
> 
> >Otherwise please point me to the URL of that thread. Sorry and thankx. :)
> 
> No, it's this issue:
> 
>   ...

Ok. try this patch. This patch is on top of the 5 patches that I had
sent last week i.e  "[PATCH  0/5] powerpc/pkeys: fixes to pkeys"

The following is a draft patch though to check if it meets your
expectations.

commit fe53b5fe2dcb3139ea27ade3ae7cbbe43c4af3be
Author: Ram Pai <linuxram@us.ibm.com>
Date:   Mon Jun 11 14:57:34 2018 -0500

    powerpc/pkeys: Deny read/write/execute by default
    
    Deny everything for all keys; with some exceptions. Do not do this for
    pkey-0, or else everything will come to a screaching halt.  Also by
    default, do not deny execute for execute-only key.
    
    This is a draft-patch for now.
    
    Signed-off-by: Ram Pai <linuxram@us.ibm.com>

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 8225263..289aafd 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -128,13 +128,13 @@ int pkey_initialize(void)
 
 	/* register mask is in BE format */
 	pkey_amr_mask = ~0x0ul;
-	pkey_iamr_mask = ~0x0ul;
+	pkey_amr_mask &= ~(0x3ul << pkeyshift(PKEY_0));
+	pkey_amr_mask &= ~(0x3ul << pkeyshift(1));
 
-	for (i = 0; i < (pkeys_total - os_reserved); i++) {
-		pkey_amr_mask &= ~(0x3ul << pkeyshift(i));
-		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
-	}
-	pkey_amr_mask |= (AMR_RD_BIT|AMR_WR_BIT) << pkeyshift(EXECUTE_ONLY_KEY);
+	pkey_iamr_mask = ~0x0ul;
+	pkey_iamr_mask &= ~(0x3ul << pkeyshift(PKEY_0));
+	pkey_iamr_mask &= ~(0x3ul << pkeyshift(1));
+	pkey_iamr_mask &= ~(0x3ul << pkeyshift(EXECUTE_ONLY_KEY));
 
 	pkey_uamor_mask = ~0x0ul;
 	pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0));

-- 
Ram Pai

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 06/11/2018 10:08 PM, Ram Pai wrote:
> Ok. try this patch. This patch is on top of the 5 patches that I had
> sent last week i.e  "[PATCH  0/5] powerpc/pkeys: fixes to pkeys"
> 
> The following is a draft patch though to check if it meets your
> expectations.
> 
> commit fe53b5fe2dcb3139ea27ade3ae7cbbe43c4af3be
> Author: Ram Pai<linuxram@us.ibm.com>
> Date:   Mon Jun 11 14:57:34 2018 -0500
> 
>      powerpc/pkeys: Deny read/write/execute by default

With this patch, my existing misc/tst-pkey test in glibc passes.  The 
in-tree version still has some incorrect assumptions on implementation 
behavior, but those are test bugs.  The kernel behavior with your patch 
look good to me.  Thanks.

Florian

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 05/19/2018 03:19 AM, Ram Pai wrote:

>> New AMR value (PID 112291, before execl): 0x0c00000000000000
>> AMR (PID 112291): 0x0c00000000000000

The issue is here.  The second line is after the execl (printed from the 
start of main), and the AMR value is not reset to zero.

>> Allocated key (PID 112291): 2
>>
>> I think this is a real bug and needs to be fixed even if the
>> defaults are kept as-is (see the other thread).
> 
> The issue you may be talking about here is that  --
> 
> "when you set the AMR register to 0xffffffffffffffff, it
> just sets it to 0x0c00000000000000."
> 
> To me it looks like, exec/fork are not related to the issue.
> Or are they also somehow connected to the issue?

Yes, this is the other issue.  It is not really important here.

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 05/19/2018 03:19 AM, Ram Pai wrote:
> The issue you may be talking about here is that  --
> 
> "when you set the AMR register to 0xffffffffffffffff, it
> just sets it to 0x0c00000000000000."
> 
> To me it looks like, exec/fork are not related to the issue.
> Or are they also somehow connected to the issue?
> 
> 
> The reason the AMR register does not get set to 0xffffffffffffffff,
> is because none of those keys; except key 2, are active. So it ignores
> all other bits and just sets the bits corresponding to key 2.

Here's a slightly different test:

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <err.h>

/* Return the value of the AMR register.  */
static inline unsigned long int
pkey_read (void)
{
   unsigned long int result;
   __asm__ volatile ("mfspr %0, 13" : "=r" (result));
   return result;
}

/* Overwrite the AMR register with VALUE.  */
static inline void
pkey_write (unsigned long int value)
{
   __asm__ volatile ("mtspr 13, %0" : : "r" (value));
}

int
main (int argc, char **argv)
{
   printf ("AMR (PID %d): 0x%016lx\n", (int) getpid (), pkey_read());
   if (argc > 1 && strcmp (argv[1], "alloc") == 0)
     {
       int key = syscall (__NR_pkey_alloc, 0, 0);
       if (key < 0)
         err (1, "pkey_alloc");
       printf ("Allocated key in subprocess (PID %d): %d\n",
               (int) getpid (), key);
       return 0;
     }

   pid_t pid = fork ();
   if (pid == 0)
     {
       printf ("AMR after fork (PID %d): 0x%016lx\n",
               (int) getpid (), pkey_read());
       execl ("/proc/self/exe", argv[0], "alloc", NULL);
       _exit (1);
     }
   if (pid < 0)
     err (1, "fork");
   int status;
   if (waitpid (pid, &status, 0) < 0)
     err (1, "waitpid");

   int key = syscall (__NR_pkey_alloc, 0, 0);
   if (key < 0)
     err (1, "pkey_alloc");
   printf ("Allocated key (PID %d): %d\n", (int) getpid (), key);

   unsigned long int amr = -1;
   printf ("Setting AMR: 0x%016lx\n", amr);
   pkey_write (amr);
   printf ("New AMR value (PID %d): 0x%016lx\n",
           (int) getpid (), pkey_read());
   if (argc == 1)
     {
       printf ("About to call execl (PID %d) ...\n", (int) getpid ());
       execl ("/proc/self/exe", argv[0], "execl", NULL);
       err (1, "exec");
       return 1;
     }
   else
     return 0;
}

It produces:

AMR (PID 110163): 0x0000000000000000
AMR after fork (PID 110164): 0x0000000000000000
AMR (PID 110164): 0x0000000000000000
Allocated key in subprocess (PID 110164): 2
Allocated key (PID 110163): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 110163): 0x0c00000000000000
About to call execl (PID 110163) ...
AMR (PID 110163): 0x0c00000000000000
AMR after fork (PID 110165): 0x0000000000000000
AMR (PID 110165): 0x0000000000000000
Allocated key in subprocess (PID 110165): 2
Allocated key (PID 110163): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 110163): 0x0c00000000000000

A few things which are odd stand out (apart the wrong default for AMR 
and the AMR update restriction covered in the other thread):

* execve does not reset AMR (see after a??About to call execla??)
* fork resets AMR (see lines with PID 110165))
* After execve, a key with non-default access rights is allocated
   (see a??Allocated key (PID 110163): 2a??, second time, after execl)

No matter what you think about the AMR default, I posit that each of 
those are bugs (although the last one should be fixed by resetting AMR 
on execve).

Thanks,
Florian

Re: pkeys on POWER: Access rights not reset on execve

[Florian Weimer]

On 05/19/2018 03:19 AM, Ram Pai wrote:
> The issue you may be talking about here is that  --
> 
> "when you set the AMR register to 0xffffffffffffffff, it
> just sets it to 0x0c00000000000000."
> 
> To me it looks like, exec/fork are not related to the issue.
> Or are they also somehow connected to the issue?
> 
> 
> The reason the AMR register does not get set to 0xffffffffffffffff,
> is because none of those keys; except key 2, are active. So it ignores
> all other bits and just sets the bits corresponding to key 2.

Here's a slightly different test:

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <err.h>

/* Return the value of the AMR register.  */
static inline unsigned long int
pkey_read (void)
{
   unsigned long int result;
   __asm__ volatile ("mfspr %0, 13" : "=r" (result));
   return result;
}

/* Overwrite the AMR register with VALUE.  */
static inline void
pkey_write (unsigned long int value)
{
   __asm__ volatile ("mtspr 13, %0" : : "r" (value));
}

int
main (int argc, char **argv)
{
   printf ("AMR (PID %d): 0x%016lx\n", (int) getpid (), pkey_read());
   if (argc > 1 && strcmp (argv[1], "alloc") == 0)
     {
       int key = syscall (__NR_pkey_alloc, 0, 0);
       if (key < 0)
         err (1, "pkey_alloc");
       printf ("Allocated key in subprocess (PID %d): %d\n",
               (int) getpid (), key);
       return 0;
     }

   pid_t pid = fork ();
   if (pid == 0)
     {
       printf ("AMR after fork (PID %d): 0x%016lx\n",
               (int) getpid (), pkey_read());
       execl ("/proc/self/exe", argv[0], "alloc", NULL);
       _exit (1);
     }
   if (pid < 0)
     err (1, "fork");
   int status;
   if (waitpid (pid, &status, 0) < 0)
     err (1, "waitpid");

   int key = syscall (__NR_pkey_alloc, 0, 0);
   if (key < 0)
     err (1, "pkey_alloc");
   printf ("Allocated key (PID %d): %d\n", (int) getpid (), key);

   unsigned long int amr = -1;
   printf ("Setting AMR: 0x%016lx\n", amr);
   pkey_write (amr);
   printf ("New AMR value (PID %d): 0x%016lx\n",
           (int) getpid (), pkey_read());
   if (argc == 1)
     {
       printf ("About to call execl (PID %d) ...\n", (int) getpid ());
       execl ("/proc/self/exe", argv[0], "execl", NULL);
       err (1, "exec");
       return 1;
     }
   else
     return 0;
}

It produces:

AMR (PID 110163): 0x0000000000000000
AMR after fork (PID 110164): 0x0000000000000000
AMR (PID 110164): 0x0000000000000000
Allocated key in subprocess (PID 110164): 2
Allocated key (PID 110163): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 110163): 0x0c00000000000000
About to call execl (PID 110163) ...
AMR (PID 110163): 0x0c00000000000000
AMR after fork (PID 110165): 0x0000000000000000
AMR (PID 110165): 0x0000000000000000
Allocated key in subprocess (PID 110165): 2
Allocated key (PID 110163): 2
Setting AMR: 0xffffffffffffffff
New AMR value (PID 110163): 0x0c00000000000000

A few things which are odd stand out (apart the wrong default for AMR 
and the AMR update restriction covered in the other thread):

* execve does not reset AMR (see after “About to call execl”)
* fork resets AMR (see lines with PID 110165))
* After execve, a key with non-default access rights is allocated
   (see “Allocated key (PID 110163): 2”, second time, after execl)

No matter what you think about the AMR default, I posit that each of 
those are bugs (although the last one should be fixed by resetting AMR 
on execve).

Thanks,
Florian