lkml.org 
[lkml]   [2017]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys
Date

Ram Pai <linuxram@us.ibm.com> writes:

> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -2,6 +2,18 @@
> #define _ASM_PPC64_PKEYS_H
>
> extern bool pkey_inited;
> +/* override any generic PKEY Permission defines */
> +#undef PKEY_DISABLE_ACCESS
> +#define PKEY_DISABLE_ACCESS 0x1
> +#undef PKEY_DISABLE_WRITE
> +#define PKEY_DISABLE_WRITE 0x2
> +#undef PKEY_DISABLE_EXECUTE
> +#define PKEY_DISABLE_EXECUTE 0x4
> +#undef PKEY_ACCESS_MASK
> +#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
> + PKEY_DISABLE_WRITE |\
> + PKEY_DISABLE_EXECUTE)
> +

Is it ok to #undef macros from another header? Especially since said
header is in uapi (include/uapi/asm-generic/mman-common.h).

Also, it's unnecessary to undef the _ACCESS and _WRITE macros since they
are identical to the original definition. And since these macros are
originally defined in an uapi header, the powerpc-specific ones should
be in an uapi header as well, if I understand it correctly.

An alternative solution is to define only PKEY_DISABLE_EXECUTE in
arch/powerpc/include/uapi/asm/mman.h and then test for its existence to
properly define PKEY_ACCESS_MASK in
include/uapi/asm-generic/mman-common.h. What do you think of the code
below?

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index e31f5ee8e81f..67e6a3a343ae 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -4,17 +4,6 @@
#include <asm/firmware.h>

extern bool pkey_inited;
-/* override any generic PKEY Permission defines */
-#undef PKEY_DISABLE_ACCESS
-#define PKEY_DISABLE_ACCESS 0x1
-#undef PKEY_DISABLE_WRITE
-#define PKEY_DISABLE_WRITE 0x2
-#undef PKEY_DISABLE_EXECUTE
-#define PKEY_DISABLE_EXECUTE 0x4
-#undef PKEY_ACCESS_MASK
-#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
- PKEY_DISABLE_WRITE |\
- PKEY_DISABLE_EXECUTE)

#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
VM_PKEY_BIT3 | VM_PKEY_BIT4)
diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index ab45cc2f3101..dee43feb7c53 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -45,4 +45,6 @@
#define MAP_HUGE_1GB (30 << MAP_HUGE_SHIFT) /* 1GB HugeTLB Page */
#define MAP_HUGE_16GB (34 << MAP_HUGE_SHIFT) /* 16GB HugeTLB Page */

+#define PKEY_DISABLE_EXECUTE 0x4
+
#endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 72eb9a1bde79..777f8f8dff47 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -12,7 +12,7 @@
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
* more details.
*/
-#include <uapi/asm-generic/mman-common.h>
+#include <asm/mman.h>
#include <linux/pkeys.h> /* PKEY_* */

bool pkey_inited;
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 8c27db0c5c08..93e3841d9ada 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -74,7 +74,15 @@

#define PKEY_DISABLE_ACCESS 0x1
#define PKEY_DISABLE_WRITE 0x2
+
+/* The arch-specific code may define PKEY_DISABLE_EXECUTE */
+#ifdef PKEY_DISABLE_EXECUTE
+#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS | \
+ PKEY_DISABLE_WRITE | \
+ PKEY_DISABLE_EXECUTE)
+#else
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#endif

#endif /* __ASM_GENERIC_MMAN_COMMON_H */

> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 98d0391..b9ad98d 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -73,6 +73,7 @@ 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 -1;
> @@ -85,5 +86,14 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>
> init_amr(pkey, new_amr_bits);
>
> + /*
> + * By default execute is disabled.
> + * To enable execute, PKEY_ENABLE_EXECUTE
> + * needs to be specified.
> + */
> + if ((init_val & PKEY_DISABLE_EXECUTE))
> + new_iamr_bits |= IAMR_EX_BIT;
> +
> + init_iamr(pkey, new_iamr_bits);
> return 0;
> }

The comment seems to be from an earlier version which has the logic
inverted, and there is no PKEY_ENABLE_EXECUTE. Should the comment be
updated to the following?

By default execute is enabled.
To disable execute, PKEY_DISABLE_EXECUTE needs to be specified.

--
Thiago Jung Bauermann
IBM Linux Technology Center

\
 
 \ /
  Last update: 2017-07-27 16:55    [W:0.414 / U:18.284 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site