powerpc, pkey: make protection key 0 less special

PATCH Superseded Ram Pai 2 participants 4 messages Mar 27, 2018 – Apr 6, 2018 PowerPC

Share & tools

Ram Pai

author

2018-03-27 01:05 UTC #0 lore raw

PATCH v2 1/1 powerpc, pkey: make protection key 0 less special

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

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

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

Tested on powerpc.

cc Thomas Gleixner <[email protected]>

cc Dave Hansen <[email protected]>

cc Michael Ellermen <[email protected]>

cc Ingo Molnar <[email protected]>

cc Andrew Morton <[email protected]>

Signed-off-by Ram Pai <[email protected]>

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

 arch/powerpc/include/asm/pkeys.h | 20 ++++++++++++++++----
 arch/powerpc/mm/pkeys.c          | 20 ++++++++++++--------
 2 files changed, 28 insertions(+), 12 deletions(-)

arch/powerpc/include/asm/pkeys.h +16 -4

index 0409c80..b598fa9 100644
@@ -101,10 +101,14 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
 
 static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
-	/* A reserved key is never considered as 'explicitly allocated' */
-	return ((pkey < arch_max_pkey()) &&
-		!__mm_pkey_is_reserved(pkey) &&
-		__mm_pkey_is_allocated(mm, pkey));
+	if (pkey < 0 || pkey >= arch_max_pkey())
+		return false;
+
+	/* Reserved keys are never allocated. */
+	if (__mm_pkey_is_reserved(pkey))
+		return false;
+
+	return __mm_pkey_is_allocated(mm, pkey);
 }
 
 extern void __arch_activate_pkey(int pkey);

@@ -200,6 +204,14 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 {
 	if (static_branch_likely(&pkey_disabled))
 		return -EINVAL;
+
+	/*
+	 * userspace is discouraged from changing permissions of
+	 * pkey-0. powerpc hardware does not support it anyway.
+	 */
+	if (!pkey)
+		return init_val ? -EINVAL : 0;
+
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }

arch/powerpc/mm/pkeys.c +12 -8

index ba71c54..e7a9e34 100644
@@ -119,19 +119,21 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	/*
-	 * Bits are in LE format. NOTE: 1, 0 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.
-	 */
+	/* Bits are in LE format. */
 	initial_allocation_mask = ~0x0;
 
 	/* register mask is in BE format */
 	pkey_amr_uamor_mask = ~0x0ul;
 	pkey_iamr_mask = ~0x0ul;
 
-	for (i = 2; i < (pkeys_total - os_reserved); i++) {
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+	 	/*
+		 * key 1 is recommended not to be used.
+		 * PowerISA(3.0) page 1015,
+		 */
+		if (i == 1)
+			continue;
+
 		initial_allocation_mask &= ~(0x1 << i);
 		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));

@@ -145,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
 {
 	if (static_branch_likely(&pkey_disabled))
 		return;
-	mm_pkey_allocation_map(mm) = initial_allocation_mask;
+
+	/* allocate key-0 by default */
+	mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
 	/* -1 means unallocated or invalid */
 	mm->context.execute_only_pkey = -1;
 }

Signature · 1 line

1.8.3.1

Thiago Jung Bauermann

2018-04-04 21:41 UTC #1 lore raw

Hello Ram,

Quoted 12 lines · Ram Pai

Applications need the ability to associate an address-range with some key and latter revert to its initial default key. Pkey-0 comes close to providing this function but falls short, because the current implementation disallows applications to explicitly associate pkey-0 to the address range.
Lets make pkey-0 less special and treat it almost like any other key. Thus it can be explicitly associated with any address range, and can be freed. This gives the application more flexibility and power. The ability to free pkey-0 must be used responsibily, since pkey-0 is associated with almost all address-range by default.

Quoted 4 lines

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

It's also special in more ways (and if intentional, these should be part of the commit message as well):

This has two causes: this patch explicitly forbids it in arch_set_user_pkey_access(), and also because even if it's allocated, the bits for key 0 in AMOR and UAMOR aren't set.

(d) it can be freed, but can't be allocated again later.

This is because mm_pkey_alloc() only calls __arch_activate_pkey(ret) if ret > 0.

It looks like (d) is a bug. Either mm_pkey_free() should fail with key 0, or mm_pkey_alloc() should work with it.

(c) could be a measure to prevent users from shooting themselves in their feet. But if that is the case, then mm_pkey_free() should forbid freeing key 0 too.

Quoted 41 lines

Tested on powerpc.

cc Thomas Gleixner <[email protected]>

cc Dave Hansen <[email protected]>

cc Michael Ellermen <[email protected]>

cc Ingo Molnar <[email protected]>

cc Andrew Morton <[email protected]>

Signed-off-by Ram Pai <[email protected]>

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

 arch/powerpc/include/asm/pkeys.h | 20 ++++++++++++++++----
 arch/powerpc/mm/pkeys.c          | 20 ++++++++++++--------
 2 files changed, 28 insertions(+), 12 deletions(-)

arch/powerpc/include/asm/pkeys.h +8 -4

index 0409c80..b598fa9 100644
@@ -101,10 +101,14 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
 
 static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
-	/* A reserved key is never considered as 'explicitly allocated' */
-	return ((pkey < arch_max_pkey()) &&
-		!__mm_pkey_is_reserved(pkey) &&
-		__mm_pkey_is_allocated(mm, pkey));
+	if (pkey < 0 || pkey >= arch_max_pkey())
+		return false;
+
+	/* Reserved keys are never allocated. */
+	if (__mm_pkey_is_reserved(pkey))
+		return false;
+
+	return __mm_pkey_is_allocated(mm, pkey);
 }
 
 extern void __arch_activate_pkey(int pkey);

arch/powerpc/include/asm/pkeys.h static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 210

Quoted 7 lines

arch/powerpc/include/asm/pkeys.h +4 -0

@@ -200,6 +204,14 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 {
 	if (static_branch_likely(&pkey_disabled))
 		return -EINVAL;
+
+	/*
+	 * userspace is discouraged from changing permissions of
+	 * pkey-0.

They're not discouraged. They're not allowed to. :-)

arch/powerpc/include/asm/pkeys.h static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 211

Quoted 1 line

arch/powerpc/include/asm/pkeys.h +1 -0
+	 * powerpc hardware does not support it anyway.

It doesn't? I don't get that impression from reading the ISA, but perhaps I'm missing something.

Quoted 21 lines

arch/powerpc/include/asm/pkeys.h +4 -0

+	 */
+	if (!pkey)
+		return init_val ? -EINVAL : 0;
+
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }

arch/powerpc/mm/pkeys.c +0 -6

index ba71c54..e7a9e34 100644
@@ -119,19 +119,21 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	/*
-	 * Bits are in LE format. NOTE: 1, 0 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.
-	 */

arch/powerpc/mm/pkeys.c int pkey_initialize(void) · line 130

Quoted 10 lines

arch/powerpc/mm/pkeys.c +3 -1

+	/* Bits are in LE format. */
 	initial_allocation_mask = ~0x0;
 
 	/* register mask is in BE format */
 	pkey_amr_uamor_mask = ~0x0ul;
 	pkey_iamr_mask = ~0x0ul;
 
-	for (i = 2; i < (pkeys_total - os_reserved); i++) {
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+	 	/*

There's a space between the tabs here.

Quoted 10 lines

arch/powerpc/mm/pkeys.c +6 -0

+		 * key 1 is recommended not to be used.
+		 * PowerISA(3.0) page 1015,
+		 */
+		if (i == 1)
+			continue;
+
 		initial_allocation_mask &= ~(0x1 << i);
 		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));

arch/powerpc/mm/pkeys.c void pkey_mm_init(struct mm_struct *mm) · line 155

Quoted 10 lines

arch/powerpc/mm/pkeys.c +3 -1

@@ -145,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
 {
 	if (static_branch_likely(&pkey_disabled))
 		return;
-	mm_pkey_allocation_map(mm) = initial_allocation_mask;
+
+	/* allocate key-0 by default */
+	mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
 	/* -1 means unallocated or invalid */
 	mm->context.execute_only_pkey = -1;
 }

I think we should also set the AMOR and UAMOR bits for key 0. Otherwise, key 0 will be in allocated-but-not-enabled state which is yet another subtle way in which it will be special.

Also, pkey_access_permitted() has a special case for key 0. Should it?

--
Thiago Jung Bauermann
IBM Linux Technology Center

Ram Pai

author

2018-04-06 18:01 UTC #2 lore raw

Quoted 22 lines · Thiago Jung Bauermann

Hello Ram,
Quoted 12 lines · Ram Pai
Applications need the ability to associate an address-range with some key and latter revert to its initial default key. Pkey-0 comes close to providing this function but falls short, because the current implementation disallows applications to explicitly associate pkey-0 to the address range.
Lets make pkey-0 less special and treat it almost like any other key. Thus it can be explicitly associated with any address range, and can be freed. This gives the application more flexibility and power. The ability to free pkey-0 must be used responsibily, since pkey-0 is associated with almost all address-range by default.
Quoted 4 lines
Even with this change pkey-0 continues to be slightly more special from the following point of view.
(a) it is implicitly allocated.
(b) it is the default key assigned to any address-range.

Quoted 8 lines

It's also special in more ways (and if intentional, these should be part of the commit message as well):
(c) it's not possible to change permissions for key 0
This has two causes: this patch explicitly forbids it in arch_set_user_pkey_access(), and also because even if it's allocated, the bits for key 0 in AMOR and UAMOR aren't set.

Yes. will have to capture that one aswell.

we cannot let userspace change permissions on key 0 because doing so will hurt the kernel too. Unlike x86 where keys are effective only in userspace, powerpc keys are effective even in the kernel. So if the kernel tries to access something, it will get stuck forever. Almost everything in the kernel is associated with key-0.

I ran a small test program which disabled access on key 0 from userspace, and as expected ran into softlockups. It certainly can lead to denial-of-service-attack. We can let apps shoot-itself-in-its-foot but if the shot hurts someone else, we will have to stop it.

The key-semantics discussed with the x86 folks did not explicitly say anything about changing permissions on key-0. We will have to keep that part of the semantics open-ended.

Quoted 8 lines

(d) it can be freed, but can't be allocated again later.
This is because mm_pkey_alloc() only calls __arch_activate_pkey(ret) if ret > 0.
It looks like (d) is a bug. Either mm_pkey_free() should fail with key 0, or mm_pkey_alloc() should work with it.

Well, it can be allocated, just that we do not let userspace change the permissions on the key. __arch_activate_pkey(ret) does not get called for pkey-0.

Quoted 44 lines

(c) could be a measure to prevent users from shooting themselves in their feet. But if that is the case, then mm_pkey_free() should forbid freeing key 0 too.
Quoted 39 lines
Tested on powerpc.
cc Thomas Gleixner <[email protected]>
cc Dave Hansen <[email protected]>
cc Michael Ellermen <[email protected]>
cc Ingo Molnar <[email protected]>
cc Andrew Morton <[email protected]>
Signed-off-by Ram Pai <[email protected]>
---
History:
v2: mm_pkey_is_allocated() continued to treat pkey-0 special.
fixed it.
 arch/powerpc/include/asm/pkeys.h | 20 ++++++++++++++++----
 arch/powerpc/mm/pkeys.c          | 20 ++++++++++++--------
 2 files changed, 28 insertions(+), 12 deletions(-)
arch/powerpc/include/asm/pkeys.h +8 -4
index 0409c80..b598fa9 100644
@@ -101,10 +101,14 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
 
 static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
-	/* A reserved key is never considered as 'explicitly allocated' */
-	return ((pkey < arch_max_pkey()) &&
-		!__mm_pkey_is_reserved(pkey) &&
-		__mm_pkey_is_allocated(mm, pkey));
+	if (pkey < 0 || pkey >= arch_max_pkey())
+		return false;
+
+	/* Reserved keys are never allocated. */
+	if (__mm_pkey_is_reserved(pkey))
+		return false;
+
+	return __mm_pkey_is_allocated(mm, pkey);
 }
 

Quoted 11 lines

arch/powerpc/include/asm/pkeys.h static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 210
Quoted 9 lines
arch/powerpc/include/asm/pkeys.h +4 -0
 extern void __arch_activate_pkey(int pkey);
@@ -200,6 +204,14 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 {
 	if (static_branch_likely(&pkey_disabled))
 		return -EINVAL;
+
+	/*
+	 * userspace is discouraged from changing permissions of
+	 * pkey-0.
They're not discouraged. They're not allowed to. :-)

ok :-)

Quoted 5 lines

arch/powerpc/include/asm/pkeys.h static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 211
Quoted 2 lines
arch/powerpc/include/asm/pkeys.h +1 -0
+	 * powerpc hardware does not support it anyway.
It doesn't? I don't get that impression from reading the ISA, but perhaps I'm missing something.

Good Catch. I am wrongly blaming it on powerpc hardware. Its a semantics enforced by our pkey code to block DOS attacks.

Quoted 29 lines

arch/powerpc/include/asm/pkeys.h +4 -0

+	 */
+	if (!pkey)
+		return init_val ? -EINVAL : 0;
+
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }

arch/powerpc/mm/pkeys.c +1 -6

index ba71c54..e7a9e34 100644
@@ -119,19 +119,21 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	/*
-	 * Bits are in LE format. NOTE: 1, 0 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.
-	 */
+	/* Bits are in LE format. */
 	initial_allocation_mask = ~0x0;
 
 	/* register mask is in BE format */
 	pkey_amr_uamor_mask = ~0x0ul;
 	pkey_iamr_mask = ~0x0ul;

Quoted 5 lines

arch/powerpc/mm/pkeys.c int pkey_initialize(void) · line 130
Quoted 3 lines
arch/powerpc/mm/pkeys.c +2 -1
-	for (i = 2; i < (pkeys_total - os_reserved); i++) {
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+	 	/*
There's a space between the tabs here.

ok. will fix.

Quoted 17 lines

arch/powerpc/mm/pkeys.c +8 -1

+		 * key 1 is recommended not to be used.
+		 * PowerISA(3.0) page 1015,
+		 */
+		if (i == 1)
+			continue;
+
 		initial_allocation_mask &= ~(0x1 << i);
 		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));

@@ -145,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
 {
 	if (static_branch_likely(&pkey_disabled))
 		return;
-	mm_pkey_allocation_map(mm) = initial_allocation_mask;
+
+	/* allocate key-0 by default */

Quoted 8 lines

arch/powerpc/mm/pkeys.c void pkey_mm_init(struct mm_struct *mm) · line 155
Quoted 4 lines
arch/powerpc/mm/pkeys.c +1 -0
+	mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
 	/* -1 means unallocated or invalid */
 	mm->context.execute_only_pkey = -1;
 }
I think we should also set the AMOR and UAMOR bits for key 0. Otherwise, key 0 will be in allocated-but-not-enabled state which is yet another subtle way in which it will be special.

No. as explained above, it will hurt to let userspace modify permissions on key-0.

Quoted 2 lines

Also, pkey_access_permitted() has a special case for key 0. Should it?

we can delete that check. though it does not hurt to leave it in place. Access/Write/Execute on pkey-0 is always permitted.

Thiago Jung Bauermann

2018-04-06 20:45 UTC #3 lore raw

Quoted 40 lines · Ram Pai

Quoted 22 lines · Thiago Jung Bauermann
Hello Ram,
Quoted 12 lines · Ram Pai
Applications need the ability to associate an address-range with some key and latter revert to its initial default key. Pkey-0 comes close to providing this function but falls short, because the current implementation disallows applications to explicitly associate pkey-0 to the address range.
Lets make pkey-0 less special and treat it almost like any other key. Thus it can be explicitly associated with any address range, and can be freed. This gives the application more flexibility and power. The ability to free pkey-0 must be used responsibily, since pkey-0 is associated with almost all address-range by default.
Quoted 4 lines
Even with this change pkey-0 continues to be slightly more special from the following point of view.
(a) it is implicitly allocated.
(b) it is the default key assigned to any address-range.
Quoted 8 lines
It's also special in more ways (and if intentional, these should be part of the commit message as well):
(c) it's not possible to change permissions for key 0
This has two causes: this patch explicitly forbids it in arch_set_user_pkey_access(), and also because even if it's allocated, the bits for key 0 in AMOR and UAMOR aren't set.
Yes. will have to capture that one aswell.
we cannot let userspace change permissions on key 0 because doing so will hurt the kernel too. Unlike x86 where keys are effective only in userspace, powerpc keys are effective even in the kernel. So if the kernel tries to access something, it will get stuck forever. Almost everything in the kernel is associated with key-0.

Quoted 5 lines

I ran a small test program which disabled access on key 0 from userspace, and as expected ran into softlockups. It certainly can lead to denial-of-service-attack. We can let apps shoot-itself-in-its-foot but if the shot hurts someone else, we will have to stop it.

Ah, I wasn't aware of that. We definitely shouldn't let userspace change key 0 permissions then. :-) It would be good to have this information in a comment in the code somewhere, IMHO.

Quoted 10 lines

The key-semantics discussed with the x86 folks did not explicitly say anything about changing permissions on key-0. We will have to keep that part of the semantics open-ended.
Quoted 6 lines
(d) it can be freed, but can't be allocated again later.
This is because mm_pkey_alloc() only calls __arch_activate_pkey(ret) if ret > 0.

Quoted 6 lines

Quoted 2 lines
It looks like (d) is a bug. Either mm_pkey_free() should fail with key 0, or mm_pkey_alloc() should work with it.
Well, it can be allocated, just that we do not let userspace change the permissions on the key. __arch_activate_pkey(ret) does not get called for pkey-0.

Yes, now I see how the allocated-but-not-enabled state makes sense. Considering that the kernel always uses key 0, it's unworkable on powerpc for an app to free pkey 0. In that case I think we should reject it in mm_pkey_free().

Quoted 58 lines

Quoted 43 lines
(c) could be a measure to prevent users from shooting themselves in their feet. But if that is the case, then mm_pkey_free() should forbid freeing key 0 too.
Quoted 39 lines
Tested on powerpc.
cc Thomas Gleixner <[email protected]>
cc Dave Hansen <[email protected]>
cc Michael Ellermen <[email protected]>
cc Ingo Molnar <[email protected]>
cc Andrew Morton <[email protected]>
Signed-off-by Ram Pai <[email protected]>
---
History:
v2: mm_pkey_is_allocated() continued to treat pkey-0 special.
fixed it.
 arch/powerpc/include/asm/pkeys.h | 20 ++++++++++++++++----
 arch/powerpc/mm/pkeys.c          | 20 ++++++++++++--------
 2 files changed, 28 insertions(+), 12 deletions(-)
arch/powerpc/include/asm/pkeys.h +8 -4
index 0409c80..b598fa9 100644
@@ -101,10 +101,14 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
 
 static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 {
-	/* A reserved key is never considered as 'explicitly allocated' */
-	return ((pkey < arch_max_pkey()) &&
-		!__mm_pkey_is_reserved(pkey) &&
-		__mm_pkey_is_allocated(mm, pkey));
+	if (pkey < 0 || pkey >= arch_max_pkey())
+		return false;
+
+	/* Reserved keys are never allocated. */
+	if (__mm_pkey_is_reserved(pkey))
+		return false;
+
+	return __mm_pkey_is_allocated(mm, pkey);
 }
 
Quoted 11 lines
arch/powerpc/include/asm/pkeys.h static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 210
Quoted 9 lines
arch/powerpc/include/asm/pkeys.h +4 -0
 extern void __arch_activate_pkey(int pkey);
@@ -200,6 +204,14 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 {
 	if (static_branch_likely(&pkey_disabled))
 		return -EINVAL;
+
+	/*
+	 * userspace is discouraged from changing permissions of
+	 * pkey-0.
They're not discouraged. They're not allowed to. :-)
ok :-)
Quoted 1 line

Quoted 7 lines

Quoted 4 lines
arch/powerpc/include/asm/pkeys.h static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 211
Quoted 1 line
arch/powerpc/include/asm/pkeys.h +1 -0
+	 * powerpc hardware does not support it anyway.
It doesn't? I don't get that impression from reading the ISA, but perhaps I'm missing something.
Good Catch. I am wrongly blaming it on powerpc hardware. Its a semantics enforced by our pkey code to block DOS attacks.

I think this would be a good place to explain why we can't allow userspace to change permissions on key 0.

Quoted 64 lines

Quoted 30 lines

arch/powerpc/include/asm/pkeys.h +4 -0

+	 */
+	if (!pkey)
+		return init_val ? -EINVAL : 0;
+
 	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }

arch/powerpc/mm/pkeys.c +1 -6

index ba71c54..e7a9e34 100644
@@ -119,19 +119,21 @@ int pkey_initialize(void)
 #else
 	os_reserved = 0;
 #endif
-	/*
-	 * Bits are in LE format. NOTE: 1, 0 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.
-	 */
+	/* Bits are in LE format. */
 	initial_allocation_mask = ~0x0;
 
 	/* register mask is in BE format */
 	pkey_amr_uamor_mask = ~0x0ul;
 	pkey_iamr_mask = ~0x0ul;

Quoted 5 lines

arch/powerpc/mm/pkeys.c int pkey_initialize(void) · line 130
Quoted 3 lines
arch/powerpc/mm/pkeys.c +2 -1
-	for (i = 2; i < (pkeys_total - os_reserved); i++) {
+	for (i = 0; i < (pkeys_total - os_reserved); i++) {
+	 	/*
There's a space between the tabs here.

ok. will fix.

Quoted 17 lines

arch/powerpc/mm/pkeys.c +8 -1

+		 * key 1 is recommended not to be used.
+		 * PowerISA(3.0) page 1015,
+		 */
+		if (i == 1)
+			continue;
+
 		initial_allocation_mask &= ~(0x1 << i);
 		pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
 		pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));

@@ -145,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
 {
 	if (static_branch_likely(&pkey_disabled))
 		return;
-	mm_pkey_allocation_map(mm) = initial_allocation_mask;
+
+	/* allocate key-0 by default */

Quoted 8 lines

arch/powerpc/mm/pkeys.c void pkey_mm_init(struct mm_struct *mm) · line 155
Quoted 4 lines
arch/powerpc/mm/pkeys.c +1 -0
+	mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
 	/* -1 means unallocated or invalid */
 	mm->context.execute_only_pkey = -1;
 }
I think we should also set the AMOR and UAMOR bits for key 0. Otherwise, key 0 will be in allocated-but-not-enabled state which is yet another subtle way in which it will be special.

Quoted 2 lines

No. as explained above, it will hurt to let userspace modify permissions on key-0.

Indeed. Thanks for explaining and even double-checking that it's indeed the case.

Quoted 5 lines

Quoted 2 lines
Also, pkey_access_permitted() has a special case for key 0. Should it?
we can delete that check. though it does not hurt to leave it in place. Access/Write/Execute on pkey-0 is always permitted.

It doesn't hurt, but it's redundant since the is_pkey_enabled() call right below will have the same effect.

Actually I just changed my mind because I see now that the early exit prevents the need to read the UAMOR (which is done by is_pkey_enabled()). Since this function is called by page table code it's probably worth avoiding the additional SPR access.

--
Thiago Jung Bauermann
IBM Linux Technology Center