[v2,6/6] powerpc/pkeys: Deny read/write/execute by default

Message ID 1528936144-6696-7-git-send-email-linuxram@us.ibm.com
State Superseded
Headers show
Series
  • powerpc/pkeys: fixes to pkeys
Related show

Commit Message

Ram Pai June 14, 2018, 12:29 a.m.
Deny all permissions on all keys, with some exceptions.  pkey-0 must
allow all permissions, or else everything comes to a screaching halt.
Execute-only key must allow execute permission.

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

Comments

Michael Ellerman June 19, 2018, 12:39 p.m. | #1
Ram Pai <linuxram@us.ibm.com> writes:
> Deny all permissions on all keys, with some exceptions.  pkey-0 must
> allow all permissions, or else everything comes to a screaching halt.
> Execute-only key must allow execute permission.

Another ABI change.

Are we calling this a bug fix?

cheers
Florian Weimer June 19, 2018, 1:19 p.m. | #2
On 06/19/2018 02:39 PM, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
>> Deny all permissions on all keys, with some exceptions.  pkey-0 must
>> allow all permissions, or else everything comes to a screaching halt.
>> Execute-only key must allow execute permission.
> 
> Another ABI change.
> 
> Are we calling this a bug fix?

Yes.  Note that the default is configurable on x86 (default is deny), so 
it's not really an ABI change as such IMHO.

Thanks,
Florian
Ram Pai June 19, 2018, 4:31 p.m. | #3
On Tue, Jun 19, 2018 at 10:39:59PM +1000, Michael Ellerman wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> > Deny all permissions on all keys, with some exceptions.  pkey-0 must
> > allow all permissions, or else everything comes to a screaching halt.
> > Execute-only key must allow execute permission.
> 
> Another ABI change.
> 
> Are we calling this a bug fix?

It is a ABI change. There are two cases where this could break an
existing application.

a) single threaded application, depending on the AMR bits of
	unallocated keys to do something.
       
	Not sure what one can achieve doing so.


b) Multithreaded application could see the difference. The scenarios is 
	i) Thread T2 allocates a key and associates with Memory M1
	ii) Thread T1 accesses the memory M1.

	Without the patch step (ii) will be successful.
	With the patch step (ii) will fail.

	I doubt any multithreaded applications are out there depending
	on this particular behavior. And if it does, than it is
	depending on a buggy behavior.

RP

Patch

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 9098605..cec990c 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -128,13 +128,11 @@  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));
 
-	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(EXECUTE_ONLY_KEY));
 
 	pkey_uamor_mask = ~0x0ul;
 	pkey_uamor_mask &= ~(0x3ul << pkeyshift(PKEY_0));