On Mon, Dec 03, 2018 at 04:52:02PM +0100, Florian Weimer wrote:
> * Ram Pai:
>
> > So the problem is as follows:
> >
> > Currently the kernel supports 'disable-write' and 'disable-access'.
> >
> > On x86, cpu supports 'disable-write' and 'disable-access'. This
> > matches with what the kernel supports. All good.
> >
> > However on power, cpu supports 'disable-read' too. Since userspace can
> > program the cpu directly, userspace has the ability to set
> > 'disable-read' too. This can lead to inconsistency between the kernel
> > and the userspace.
> >
> > We want the kernel to match userspace on all architectures.
>
> Correct.
>
> > Proposed Solution:
> >
> > Enhance the kernel to understand 'disable-read', and facilitate architectures
> > that understand 'disable-read' to allow it.
> >
> > Also explicitly define the semantics of disable-access as
> > 'disable-read and disable-write'
> >
> > Did I get this right? Assuming I did, the implementation has to do
> > the following --
> >
> > On power, sys_pkey_alloc() should succeed if the init_val
> > is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
> > or any combination of the three.
>
> Agreed.
>
> > On x86, sys_pkey_alloc() should succeed if the init_val is
> > PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
> > or any combination of the three, except PKEY_DISABLE_READ
> > specified all by itself.
>
> Again agreed. That's a clever way of phrasing it actually.
>
> > On all other arches, none of the flags are supported.
> >
> >
> > Are we on the same plate?
>
> I think so, thanks.
>
> Florian
Ok. here is a patch, compiled but not tested. See if this meets the
specifications.
-----------------------------------------------------------------------------------
commit 3dc06e73f3795921265d5d1d935e428deab01616
Author: Ram Pai <linuxram@xxxxxxxxxx>
Date: Tue Dec 4 00:04:11 2018 -0500
pkeys: add support of PKEY_DISABLE_READ
Kernel supports 'disable-write' and 'disable-access'.
x86 cpu supports 'disable-write' and 'disable-access'. This
matches with the kernel support.
However POWER cpu supports 'disable-read' too. Since userspace can
program the cpu directly, userspace has the ability to set
'disable-read' too. This can lead to inconsistency between the kernel
and the userspace.
Make kernel match userspace on all architectures.
Enhance the kernel to understand 'disable-read', and facilitate architectures
that understand 'disable-read' to allow it.
Define the semantics of disable-access as 'disable-read and disable-write'
Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 20ebf15..4bd09d0 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -19,11 +19,7 @@
#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)
{
@@ -199,6 +195,16 @@ static inline bool arch_pkeys_enabled(void)
return !static_branch_likely(&pkey_disabled);
}
+extern bool __arch_pkey_access_rights_valid(unsigned long rights);
+
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ if (static_branch_likely(&pkey_disabled))
+ return false;
+
+ return __arch_pkey_access_rights_valid(rights);
+}
+
extern void pkey_mm_init(struct mm_struct *mm);
extern bool arch_supports_pkeys(int cap);
extern unsigned int arch_usable_pkeys(void);
diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index 65065ce..e63bc37 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -30,10 +30,4 @@
#define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */
#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_EXECUTE)
#endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 5d65c47..a4f288b 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -67,7 +67,7 @@ int pkey_initialize(void)
* Ensure that the bits a distinct.
*/
BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
- (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
+ (PKEY_DISABLE_READ | PKEY_DISABLE_WRITE | PKEY_DISABLE_ACCESS));
/*
* pkey_to_vmflag_bits() assumes that the pkey bits are contiguous
@@ -259,11 +259,20 @@ 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;
}
+bool __arch_pkey_access_rights_valid(unsigned long rights)
+{
+ unsigned long mask = PKEY_DISABLE_READ | PKEY_DISABLE_WRITE |\
+ PKEY_DISABLE_ACCESS;
+ return (rights & mask);
+}
+
void thread_pkey_regs_save(struct thread_struct *thread)
{
if (static_branch_likely(&pkey_disabled))
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 19b137f..4f36a7e 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -14,6 +14,15 @@ static inline bool arch_pkeys_enabled(void)
return boot_cpu_has(X86_FEATURE_OSPKE);
}
+extern bool __arch_pkey_access_rights_valid(unsigned long rights);
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ if (!boot_cpu_has(X86_FEATURE_OSPKE))
+ return false;
+
+ return __arch_pkey_access_rights_valid(rights);
+}
+
/*
* Try to dedicate one of the protection keys to be used as an
* execute-only protection key.
diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
index 6e98e0a..fcfe1b2 100644
--- a/arch/x86/mm/pkeys.c
+++ b/arch/x86/mm/pkeys.c
@@ -72,6 +72,17 @@ int __execute_only_pkey(struct mm_struct *mm)
return execute_only_pkey;
}
+bool __arch_pkey_access_rights_valid(unsigned long rights)
+{
+ unsigned long mask = PKEY_DISABLE_READ | PKEY_DISABLE_WRITE |\
+ PKEY_DISABLE_ACCESS;
+ if (!(rights & mask))
+ return false;
+
+ /* return failure if only PKEY_DISABLE_READ is specified */
+ return ((rights & mask) != PKEY_DISABLE_READ);
+}
+
static inline bool vma_is_pkey_exec_only(struct vm_area_struct *vma)
{
/* Do this check first since the vm_flags should be hot */
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 2955ba97..2c330fa 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -48,6 +48,11 @@ static inline void copy_init_pkru_to_fpregs(void)
{
}
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+ return false;
+}
+
#endif /* ! CONFIG_ARCH_HAS_PKEYS */
#endif /* _LINUX_PKEYS_H */
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index e7ee328..d2e1a5e 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -71,7 +71,5 @@
#define PKEY_DISABLE_ACCESS 0x1
#define PKEY_DISABLE_WRITE 0x2
-#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
- PKEY_DISABLE_WRITE)
-
+#define PKEY_DISABLE_READ 0x10
#endif /* __ASM_GENERIC_MMAN_COMMON_H */
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 6d33162..f4cefc3 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -597,7 +597,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
if (flags)
return -EINVAL;
/* check for unsupported init values */
- if (init_val & ~PKEY_ACCESS_MASK)
+ if (!arch_pkey_access_rights_valid(init_val))
return -EINVAL;
down_write(¤t->mm->mmap_sem);
-----------------------------------------------------------------------------
