| Message ID | 20230515130553.2311248-2-jeffxu@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Memory Mapping (VMA) protection using PKU - set 1 | expand |
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.
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h index 59a2c7dbc78f..943333ac0fee 100644 --- a/arch/powerpc/include/asm/pkeys.h +++ b/arch/powerpc/include/asm/pkeys.h @@ -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 */ diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h index 0da5c227f490..d97594b44d9a 100644 --- a/arch/x86/include/asm/mmu.h +++ b/arch/x86/include/asm/mmu.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; diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h index 2e6c04d8a45b..ecadf04a8251 100644 --- a/arch/x86/include/asm/pkeys.h +++ b/arch/x86/include/asm/pkeys.h @@ -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 */ diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c index 7418c367e328..a76981f44acf 100644 --- 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; diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h index 86be8bf27b41..81a482c3e051 100644 --- a/include/linux/pkeys.h +++ b/include/linux/pkeys.h @@ -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 */ diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h index f55bc680b5b0..8c69b9a7ff5b 100644 --- a/include/uapi/linux/mman.h +++ b/include/uapi/linux/mman.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 */ diff --git a/mm/mprotect.c b/mm/mprotect.c index 92d3d3ca390a..8a68fdca8487 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -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)