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

On Sat, May 19, 2018 at 11:06:20PM -0700, Andy Lutomirski wrote:

On Sat, May 19, 2018 at 11:04 PM Ram Pai [off-list ref] wrote:

quoted

On Sat, May 19, 2018 at 04:47:23PM -0700, Andy Lutomirski wrote: > On

Sat, May 19, 2018 at 1:28 PM Ram Pai [off-list ref] wrote:

quoted

...snip...

quoted

So is it possible for two threads to each call pkey_alloc() and end up

with

quoted

quoted

the same key?  If so, it seems entirely broken.

quoted

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

quoted

quoted

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

quoted

quoted

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:

quoted

quoted

1. A crypto library.  Suppose I'm writing a TLS-terminating server, and

I

quoted

quoted

want it to be resistant to Heartbleed-like bugs.  I could store my

private

quoted

quoted

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

quoted

quoted

back.  On x86 right now, this works nicely.  On POWER, it doesn't,

because

quoted

quoted

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

quoted

quoted

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

quoted

quoted

update their PKRU values.  On POWER, as in example (1), the error goes

the

quoted

quoted

other direction -- if I fail to propagate the AMR bits to all threads,
writes are not blocked.

quoted

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.

quoted

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

quoted

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 <redacted>

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