Re: [PATCH 09/25] powerpc: ability to create execute-disabled pkeys

On Tue, Oct 24, 2017 at 12:28:35PM +0530, Aneesh Kumar K.V wrote:

Ram Pai [off-list ref] writes:

quoted

On Wed, Oct 18, 2017 at 02:42:56PM +1100, Balbir Singh wrote:

quoted

On Fri,  8 Sep 2017 15:44:57 -0700
Ram Pai [off-list ref] wrote:

quoted

powerpc has hardware support to disable execute on a pkey.
This patch enables the ability to create execute-disabled
keys.

Signed-off-by: Ram Pai <redacted>
---
 arch/powerpc/include/uapi/asm/mman.h |    6 ++++++
 arch/powerpc/mm/pkeys.c              |   16 ++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index ab45cc2..f272b09 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h

@@ -45,4 +45,10 @@
 #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
 #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
 
+/* override any generic PKEY Permission defines */
+#define PKEY_DISABLE_EXECUTE   0x4
+#undef PKEY_ACCESS_MASK
+#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
+				PKEY_DISABLE_WRITE  |\
+				PKEY_DISABLE_EXECUTE)
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index cc5be6a..2282864 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c

@@ -24,6 +24,14 @@ void __init pkey_initialize(void)
 {
 	int os_reserved, i;
 
+	/*
+	 * we define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
+	 * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
+	 * Ensure that the bits a distinct.
+	 */
+	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
+		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));

Will these values every change? It's good to have I guess.

quoted

+
 	/* disable the pkey system till everything
 	 * is in place. A patch further down the
 	 * line will enable it.

@@ -120,10 +128,18 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val)
 {
 	u64 new_amr_bits = 0x0ul;
+	u64 new_iamr_bits = 0x0ul;
 
 	if (!is_pkey_enabled(pkey))
 		return -EINVAL;
 
+	if ((init_val & PKEY_DISABLE_EXECUTE)) {
+		if (!pkey_execute_disable_support)
+			return -EINVAL;
+		new_iamr_bits |= IAMR_EX_BIT;
+	}
+	init_iamr(pkey, new_iamr_bits);
+

Where do we check the reserved keys?

The main gate keeper against spurious keys are the system calls.
sys_pkey_mprotect(), sys_pkey_free() and sys_pkey_modify() are the one
that will check against reserved and unallocated keys.  Once it has
passed the check, all other internal functions trust the key values
provided to them. I can put in additional checks but that will
unnecessarily chew a few cpu cycles.

Agree?

BTW: you raise a good point though, I may have missed guarding against
unallocated or reserved keys in sys_pkey_modify(). That was a power
specific system call that I have introduced to change the permissions on
a key.

Why do you need a power specific syscall? We should ideally not require
anything powerpc specific in the application to use memory keys. If it
is for exectue only key, the programming model should remain same as the
other keys.

The programming model has not changed. It continues to be the
same. i.e 

a) allocate a key  through sys_pkey_alloc()
b) associate the key to a addressspace through sys_pkey_mprotect()
c) change the permissions on the key by programming the AMR register as
	and when needed.
d) free the key through sys_pkey_free() when done.


the problem is with the programming of execute-permission on the key. x86
does not support the execute-permission and does not have the issue.

powerpc supports execute-permission but unfortunately has not exposed
that capability to userspace, because IAMR cannot be programmed from
userspace. I have filled in that gap, by providing a power-specific
system call called sys_pkey_modify().  It is a way to enable the exact 
same programming model on keys for execute-permissions.

NOTE: I am not able to find patch that add sys_pkey_modify()

Yes that patch was added only recently to my tree after consulting
Michael Ellermen. I am yet to send out that patch. Will be doing so
in my next version.

RP

-aneesh

-- 
Ram Pai