[1/6] PKEY: Introduce PKEY_ENFORCE_API flag
Commit Message
From: Jeff Xu <jeffxu@google.com>
This patch introduces a new flag, PKEY_ENFORCE_API, to the pkey_alloc()
function. When a PKEY is created with this flag, it is enforced that any
thread that wants to make changes to the memory mapping (such as
mprotect/munmap) of the memory must have write access to the PKEY.
This is to prevent unauthorized access to protected memory.
PKEYs created without this flag will continue to work as they do now,
for backwards compatibility.
Signed-off-by: Jeff Xu<jeffxu@google.com>
---
arch/powerpc/include/asm/pkeys.h | 11 ++++++++-
arch/x86/include/asm/mmu.h | 7 ++++++
arch/x86/include/asm/pkeys.h | 42 ++++++++++++++++++++++++++++++--
arch/x86/mm/pkeys.c | 2 +-
include/linux/pkeys.h | 9 ++++++-
include/uapi/linux/mman.h | 5 ++++
mm/mprotect.c | 6 ++---
7 files changed, 74 insertions(+), 8 deletions(-)
Comments
On 5/15/23 06:05, jeffxu@chromium.org wrote:
> --- a/arch/x86/mm/pkeys.c
> +++ b/arch/x86/mm/pkeys.c
> @@ -20,7 +20,7 @@ int __execute_only_pkey(struct mm_struct *mm)
> /* Do we need to assign a pkey for mm's execute-only maps? */
> if (execute_only_pkey == -1) {
> /* Go allocate one to use, which might fail */
> - execute_only_pkey = mm_pkey_alloc(mm);
> + execute_only_pkey = mm_pkey_alloc(mm, 0);
> if (execute_only_pkey < 0)
> return -1;
> need_to_set_mm_pkey = true;
In your threat model, what mechanism prevents the attacker from
modifying executable mappings?
I was trying to figure out if the implicit execute-only pkey should have
the PKEY_ENFORCE_API bit set. I think that in particular would probably
cause some kind of ABI breakage, but it still reminded me that I have an
incomplete picture of the threat model.
On Tue, May 16, 2023 at 4:14 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/15/23 06:05, jeffxu@chromium.org wrote:
> > --- a/arch/x86/mm/pkeys.c
> > +++ b/arch/x86/mm/pkeys.c
> > @@ -20,7 +20,7 @@ int __execute_only_pkey(struct mm_struct *mm)
> > /* Do we need to assign a pkey for mm's execute-only maps? */
> > if (execute_only_pkey == -1) {
> > /* Go allocate one to use, which might fail */
> > - execute_only_pkey = mm_pkey_alloc(mm);
> > + execute_only_pkey = mm_pkey_alloc(mm, 0);
> > if (execute_only_pkey < 0)
> > return -1;
> > need_to_set_mm_pkey = true;
>
> In your threat model, what mechanism prevents the attacker from
> modifying executable mappings?
>
> I was trying to figure out if the implicit execute-only pkey should have
> the PKEY_ENFORCE_API bit set. I think that in particular would probably
> cause some kind of ABI breakage, but it still reminded me that I have an
> incomplete picture of the threat model.
Yes. The main reason for not adding it now is the ABI breakage.
As a next step, we could potentially develop mseal(), which fits more
to the code segment.
The PKEY_ENFORCE_API allows munmap(), so the user case is slightly different.
I will leave the threat model / V8 specific question to Stephan.
On Wed, May 17, 2023 at 1:14 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 5/15/23 06:05, jeffxu@chromium.org wrote:
> > --- a/arch/x86/mm/pkeys.c
> > +++ b/arch/x86/mm/pkeys.c
> > @@ -20,7 +20,7 @@ int __execute_only_pkey(struct mm_struct *mm)
> > /* Do we need to assign a pkey for mm's execute-only maps? */
> > if (execute_only_pkey == -1) {
> > /* Go allocate one to use, which might fail */
> > - execute_only_pkey = mm_pkey_alloc(mm);
> > + execute_only_pkey = mm_pkey_alloc(mm, 0);
> > if (execute_only_pkey < 0)
> > return -1;
> > need_to_set_mm_pkey = true;
>
> In your threat model, what mechanism prevents the attacker from
> modifying executable mappings?
There are different options how we can address this:
1) having a generic mseal() API as Jeff mentioned
2) tagging all code pages with the pkey we're using
(would this affect memory sharing between processes?)
3) prevent this with seccomp + userspace validation
If we have pkey support, we will only create executable memory using
pkey_mprotect and everything else can be blocked with seccomp. This would still
allow turning R-X memory into RW- memory, but you can't change it back without
going through our codepath that has added validation.
There's a similar challenge with making RO memory writable. For this we'll need
to use approach 1) or 2) instead.
> I was trying to figure out if the implicit execute-only pkey should have
> the PKEY_ENFORCE_API bit set. I think that in particular would probably
> cause some kind of ABI breakage, but it still reminded me that I have an
> incomplete picture of the threat model.
@@ -82,7 +82,7 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
* Relies on the mmap_lock to protect against concurrency in mm_pkey_alloc() and
* mm_pkey_free().
*/
-static inline int mm_pkey_alloc(struct mm_struct *mm)
+static inline int mm_pkey_alloc(struct mm_struct *mm, unsigned long flags)
{
/*
* Note: this is the one and only place we make sure that the pkey is
@@ -168,5 +168,14 @@ static inline bool arch_pkeys_enabled(void)
return mmu_has_feature(MMU_FTR_PKEY);
}
+static inline bool arch_check_pkey_alloc_flags(unsigned long flags)
+{
+ /* No flags supported yet. */
+ if (flags)
+ return false;
+
+ return true;
+}
+
extern void pkey_mm_init(struct mm_struct *mm);
#endif /*_ASM_POWERPC_KEYS_H */
@@ -66,6 +66,13 @@ typedef struct {
*/
u16 pkey_allocation_map;
s16 execute_only_pkey;
+ /*
+ * One bit per protection key.
+ * When set, thread must have write permission on corresponding
+ * PKRU in order to call memory mapping API, such as mprotect,
+ * munmap, etc.
+ */
+ u16 pkey_enforce_api_map;
#endif
} mm_context_t;
@@ -51,6 +51,17 @@ static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
mm_pkey_allocation_map(mm) &= ~(1U << pkey); \
} while (0)
+#define mm_pkey_enforce_api_map(mm) (mm->context.pkey_enforce_api_map)
+#define mm_set_pkey_enforce_api(mm, pkey) \
+ { \
+ mm_pkey_enforce_api_map(mm) |= (1U << pkey); \
+ }
+
+#define mm_clear_pkey_enforce_api(mm, pkey) \
+ { \
+ mm_pkey_enforce_api_map(mm) &= ~(1U << pkey); \
+ }
+
static inline
bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
@@ -74,11 +85,25 @@ bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
return mm_pkey_allocation_map(mm) & (1U << pkey);
}
+/*
+ * Return true if the pkey has ENFORCE_API flag during allocation.
+ */
+static inline bool mm_pkey_enforce_api(struct mm_struct *mm, int pkey)
+{
+ /*
+ * Only pkey created by user space has the flag.
+ * execute_only_pkey check is in mm_pkey_is_allocated().
+ */
+ if (pkey != ARCH_DEFAULT_PKEY && mm_pkey_is_allocated(mm, pkey))
+ return mm_pkey_enforce_api_map(mm) & (1U << pkey);
+
+ return false;
+}
+
/*
* Returns a positive, 4-bit key on success, or -1 on failure.
*/
-static inline
-int mm_pkey_alloc(struct mm_struct *mm)
+static inline int mm_pkey_alloc(struct mm_struct *mm, unsigned long flags)
{
/*
* Note: this is the one and only place we make sure
@@ -101,6 +126,9 @@ int mm_pkey_alloc(struct mm_struct *mm)
mm_set_pkey_allocated(mm, ret);
+ if (flags & PKEY_ENFORCE_API)
+ mm_set_pkey_enforce_api(mm, ret);
+
return ret;
}
@@ -110,6 +138,7 @@ int mm_pkey_free(struct mm_struct *mm, int pkey)
if (!mm_pkey_is_allocated(mm, pkey))
return -EINVAL;
+ mm_clear_pkey_enforce_api(mm, pkey);
mm_set_pkey_free(mm, pkey);
return 0;
@@ -123,4 +152,13 @@ static inline int vma_pkey(struct vm_area_struct *vma)
return (vma->vm_flags & vma_pkey_mask) >> VM_PKEY_SHIFT;
}
+static inline bool arch_check_pkey_alloc_flags(unsigned long flags)
+{
+ unsigned long valid_flags = PKEY_ENFORCE_API;
+
+ if (flags & ~valid_flags)
+ return false;
+
+ return true;
+}
#endif /*_ASM_X86_PKEYS_H */
@@ -20,7 +20,7 @@ int __execute_only_pkey(struct mm_struct *mm)
/* Do we need to assign a pkey for mm's execute-only maps? */
if (execute_only_pkey == -1) {
/* Go allocate one to use, which might fail */
- execute_only_pkey = mm_pkey_alloc(mm);
+ execute_only_pkey = mm_pkey_alloc(mm, 0);
if (execute_only_pkey < 0)
return -1;
need_to_set_mm_pkey = true;
@@ -3,6 +3,7 @@
#define _LINUX_PKEYS_H
#include <linux/mm.h>
+#include <linux/mman.h>
#define ARCH_DEFAULT_PKEY 0
@@ -25,7 +26,7 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
return (pkey == 0);
}
-static inline int mm_pkey_alloc(struct mm_struct *mm)
+static inline int mm_pkey_alloc(struct mm_struct *mm, unsigned long flags)
{
return -1;
}
@@ -46,6 +47,12 @@ static inline bool arch_pkeys_enabled(void)
return false;
}
+static inline bool arch_check_pkey_alloc_flags(unsigned long flags)
+{
+ if (flags)
+ return false;
+ return true;
+}
#endif /* ! CONFIG_ARCH_HAS_PKEYS */
#endif /* _LINUX_PKEYS_H */
@@ -41,4 +41,9 @@
#define MAP_HUGE_2GB HUGETLB_FLAG_ENCODE_2GB
#define MAP_HUGE_16GB HUGETLB_FLAG_ENCODE_16GB
+/*
+ * Flags for pkey_alloc
+ */
+#define PKEY_ENFORCE_API (1 << 0)
+
#endif /* _UAPI_LINUX_MMAN_H */
@@ -894,15 +894,15 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
int pkey;
int ret;
- /* No flags supported yet. */
- if (flags)
+ if (!arch_check_pkey_alloc_flags(flags))
return -EINVAL;
+
/* check for unsupported init values */
if (init_val & ~PKEY_ACCESS_MASK)
return -EINVAL;
mmap_write_lock(current->mm);
- pkey = mm_pkey_alloc(current->mm);
+ pkey = mm_pkey_alloc(current->mm, flags);
ret = -ENOSPC;
if (pkey == -1)