On Mon, Nov 12, 2018 at 01:00:19PM +0100, Florian Weimer wrote:
> * Ram Pai:
>
> > On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
> >> * Ram Pai:
> >>
> >> > Florian,
> >> >
> >> > I can. But I am struggling to understand the requirement. Why is
> >> > this needed? Are we proposing a enhancement to the sys_pkey_alloc(),
> >> > to be able to allocate keys that are initialied to disable-read
> >> > only?
> >>
> >> Yes, I think that would be a natural consequence.
> >>
> >> However, my immediate need comes from the fact that the AMR register can
> >> contain a flag combination that is not possible to represent with the
> >> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags. User code
> >> could write to AMR directly, so I cannot rule out that certain flag
> >> combinations exist there.
> >>
> >> So I came up with this:
> >>
> >> int
> >> pkey_get (int key)
> >> {
> >> if (key < 0 || key > PKEY_MAX)
> >> {
> >> __set_errno (EINVAL);
> >> return -1;
> >> }
> >> unsigned int index = pkey_index (key);
> >> unsigned long int amr = pkey_read ();
> >> unsigned int bits = (amr >> index) & 3;
> >>
> >> /* Translate from AMR values. PKEY_AMR_READ standing alone is not
> >> currently representable. */
> >> if (bits & PKEY_AMR_READ)
> >
> > this should be
> > if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))
>
> This would return zero for PKEY_AMR_READ alone.
>
> >> return PKEY_DISABLE_ACCESS;
> >
> >
> >> else if (bits == PKEY_AMR_WRITE)
> >> return PKEY_DISABLE_WRITE;
> >> return 0;
> >> }
>
> It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case.
> Which is why I want PKEY_DISABLE_READ.
>
> >> And this is not ideal. I would prefer something like this instead:
> >>
> >> switch (bits)
> >> {
> >> case PKEY_AMR_READ | PKEY_AMR_WRITE:
> >> return PKEY_DISABLE_ACCESS;
> >> case PKEY_AMR_READ:
> >> return PKEY_DISABLE_READ;
> >> case PKEY_AMR_WRITE:
> >> return PKEY_DISABLE_WRITE;
> >> case 0:
> >> return 0;
> >> }
> >
> > yes.
> > and on x86 it will be something like:
> > switch (bits)
> > {
> > case PKEY_PKRU_ACCESS :
> > return PKEY_DISABLE_ACCESS;
> > case PKEY_AMR_WRITE:
> > return PKEY_DISABLE_WRITE;
> > case 0:
> > return 0;
> > }
>
> x86 returns the PKRU bits directly, including the nonsensical case
> (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE).
>
> > But for this to work, why do you need to enhance the sys_pkey_alloc()
> > interface? Not that I am against it. Trying to understand if the
> > enhancement is really needed.
>
> sys_pkey_alloc performs an implicit pkey_set for the newly allocated key
> (that is, it updates the PKRU/AMR register). It makes sense to match
> the behavior of the userspace implementation.
Here is a untested patch. Does this meet your needs?
It defines the new flags. Each architecture will than define the set of flags
it supports through PKEY_ACCESS_MASK.
Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 92a9962..724ef43 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -21,11 +21,6 @@
#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
VM_PKEY_BIT3 | VM_PKEY_BIT4)
-/* Override any generic PKEY permission defines */
-#define PKEY_DISABLE_EXECUTE 0x4
-#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS | \
- PKEY_DISABLE_WRITE | \
- PKEY_DISABLE_EXECUTE)
static inline u64 pkey_to_vmflag_bits(u16 pkey)
{
diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index 65065ce..76237b3 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -31,9 +31,9 @@
#define MAP_HUGETLB 0x40000 /* create a huge page mapping */
/* 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_READ |\
PKEY_DISABLE_EXECUTE)
#endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 4860acd..c8b2540 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -62,14 +62,6 @@ int 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));
-
- /*
* pkey_to_vmflag_bits() assumes that the pkey bits are contiguous
* in the vmaflag. Make sure that is really the case.
*/
@@ -259,6 +251,8 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
new_amr_bits |= AMR_RD_BIT | AMR_WR_BIT;
else if (init_val & PKEY_DISABLE_WRITE)
new_amr_bits |= AMR_WR_BIT;
+ else if (init_val & PKEY_DISABLE_READ)
+ new_amr_bits |= AMR_RD_BIT;
init_amr(pkey, new_amr_bits);
return 0;
diff --git a/arch/x86/include/uapi/asm/mman.h b/arch/x86/include/uapi/asm/mman.h
index d4a8d04..e9b121b 100644
--- a/arch/x86/include/uapi/asm/mman.h
+++ b/arch/x86/include/uapi/asm/mman.h
@@ -24,6 +24,11 @@
((key) & 0x2 ? VM_PKEY_BIT1 : 0) | \
((key) & 0x4 ? VM_PKEY_BIT2 : 0) | \
((key) & 0x8 ? VM_PKEY_BIT3 : 0))
+
+/* Override any generic PKEY permission defines */
+#undef PKEY_ACCESS_MASK
+#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
+ PKEY_DISABLE_WRITE)
#endif
#include <asm-generic/mman.h>
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index e7ee328..61168e4 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -71,7 +71,8 @@
#define PKEY_DISABLE_ACCESS 0x1
#define PKEY_DISABLE_WRITE 0x2
-#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
- PKEY_DISABLE_WRITE)
-
+#define PKEY_DISABLE_EXECUTE 0x4
+#define PKEY_DISABLE_READ 0x8
+#define PKEY_ACCESS_MASK 0x0 /* arch can override and define its own
+ mask bits */
#endif /* __ASM_GENERIC_MMAN_COMMON_H */
