| Message ID | 20190619222401.14942-10-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
> From: Christopherson, Sean J > Sent: Wednesday, June 19, 2019 3:24 PM > > diff --git a/include/linux/security.h b/include/linux/security.h > index 6a1f54ba6794..572ddfc53039 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -1832,11 +1832,18 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux) > #ifdef CONFIG_INTEL_SGX > #ifdef CONFIG_SECURITY > int security_enclave_map(unsigned long prot); > +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot, > + bool measured); > #else > static inline int security_enclave_map(unsigned long prot) > { > return 0; > } > +static inline int security_enclave_load(struct vm_area_struct *vma, > + unsigned long prot, bool measured) > +{ > + return 0; > +} > #endif /* CONFIG_SECURITY */ > #endif /* CONFIG_INTEL_SGX */ Parameters to security_enclave_load() are specific on what's being loading only, but unspecific on which enclave to be loaded into. That kills the possibility of an LSM module making enclave dependent decisions. Btw, if enclave (in the form of struct file) is also passed in as a parameter, it'd let LSM know that file is an enclave, hence would be able to make the same decision in security_mmap_file() as in security_enclave_map(). In other words, you wouldn't need security_enclave_map().
On 6/21/19 1:05 PM, Xing, Cedric wrote: >> From: Christopherson, Sean J >> Sent: Wednesday, June 19, 2019 3:24 PM >> >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 6a1f54ba6794..572ddfc53039 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -1832,11 +1832,18 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux) >> #ifdef CONFIG_INTEL_SGX >> #ifdef CONFIG_SECURITY >> int security_enclave_map(unsigned long prot); >> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot, >> + bool measured); >> #else >> static inline int security_enclave_map(unsigned long prot) >> { >> return 0; >> } >> +static inline int security_enclave_load(struct vm_area_struct *vma, >> + unsigned long prot, bool measured) >> +{ >> + return 0; >> +} >> #endif /* CONFIG_SECURITY */ >> #endif /* CONFIG_INTEL_SGX */ > > Parameters to security_enclave_load() are specific on what's being loading only, but unspecific on which enclave to be loaded into. That kills the possibility of an LSM module making enclave dependent decisions. > > Btw, if enclave (in the form of struct file) is also passed in as a parameter, it'd let LSM know that file is an enclave, hence would be able to make the same decision in security_mmap_file() as in security_enclave_map(). In other words, you wouldn't need security_enclave_map(). Sorry, you want security_enclave_load() to stash a reference to the enclave file in some security module-internal state, then match it upon later security_mmap_file() calls to determine that it is dealing with an enclave, and then adjust its logic accordingly? When do we release that reference?
On 6/25/19 5:01 PM, Stephen Smalley wrote: > On 6/21/19 1:05 PM, Xing, Cedric wrote: >>> From: Christopherson, Sean J >>> Sent: Wednesday, June 19, 2019 3:24 PM >>> >>> diff --git a/include/linux/security.h b/include/linux/security.h >>> index 6a1f54ba6794..572ddfc53039 100644 >>> --- a/include/linux/security.h >>> +++ b/include/linux/security.h >>> @@ -1832,11 +1832,18 @@ static inline void >>> security_bpf_prog_free(struct bpf_prog_aux *aux) >>> #ifdef CONFIG_INTEL_SGX >>> #ifdef CONFIG_SECURITY >>> int security_enclave_map(unsigned long prot); >>> +int security_enclave_load(struct vm_area_struct *vma, unsigned long >>> prot, >>> + bool measured); >>> #else >>> static inline int security_enclave_map(unsigned long prot) >>> { >>> return 0; >>> } >>> +static inline int security_enclave_load(struct vm_area_struct *vma, >>> + unsigned long prot, bool measured) >>> +{ >>> + return 0; >>> +} >>> #endif /* CONFIG_SECURITY */ >>> #endif /* CONFIG_INTEL_SGX */ >> >> Parameters to security_enclave_load() are specific on what's being >> loading only, but unspecific on which enclave to be loaded into. That >> kills the possibility of an LSM module making enclave dependent >> decisions. >> >> Btw, if enclave (in the form of struct file) is also passed in as a >> parameter, it'd let LSM know that file is an enclave, hence would be >> able to make the same decision in security_mmap_file() as in >> security_enclave_map(). In other words, you wouldn't need >> security_enclave_map(). > > Sorry, you want security_enclave_load() to stash a reference to the > enclave file in some security module-internal state, then match it upon > later security_mmap_file() calls to determine that it is dealing with an > enclave, and then adjust its logic accordingly? When do we release that > reference? I guess you mean set a flag in the enclave file security struct upon security_enclave_load() and check that flag in security_mmap_file(). This seems somewhat similar to one of Sean's alternatives in the patch description for 06/12, except by pushing the information from sgx to LSM upon security_enclave_load() rather than pulling it via a is_sgx_enclave() helper. Not clear if it is still subject to the same limitations.
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > owner@vger.kernel.org] On Behalf Of Stephen Smalley > Sent: Tuesday, June 25, 2019 2:49 PM > > On 6/25/19 5:01 PM, Stephen Smalley wrote: > > On 6/21/19 1:05 PM, Xing, Cedric wrote: > >>> From: Christopherson, Sean J > >>> Sent: Wednesday, June 19, 2019 3:24 PM > >>> > >>> diff --git a/include/linux/security.h b/include/linux/security.h > >>> index 6a1f54ba6794..572ddfc53039 100644 > >>> --- a/include/linux/security.h > >>> +++ b/include/linux/security.h > >>> @@ -1832,11 +1832,18 @@ static inline void > >>> security_bpf_prog_free(struct bpf_prog_aux *aux) > >>> #ifdef CONFIG_INTEL_SGX > >>> #ifdef CONFIG_SECURITY > >>> int security_enclave_map(unsigned long prot); > >>> +int security_enclave_load(struct vm_area_struct *vma, unsigned long > >>> prot, > >>> + bool measured); > >>> #else > >>> static inline int security_enclave_map(unsigned long prot) > >>> { > >>> return 0; > >>> } > >>> +static inline int security_enclave_load(struct vm_area_struct *vma, > >>> + unsigned long prot, bool measured) { > >>> + return 0; > >>> +} > >>> #endif /* CONFIG_SECURITY */ > >>> #endif /* CONFIG_INTEL_SGX */ > >> > >> Parameters to security_enclave_load() are specific on what's being > >> loading only, but unspecific on which enclave to be loaded into. That > >> kills the possibility of an LSM module making enclave dependent > >> decisions. > >> > >> Btw, if enclave (in the form of struct file) is also passed in as a > >> parameter, it'd let LSM know that file is an enclave, hence would be > >> able to make the same decision in security_mmap_file() as in > >> security_enclave_map(). In other words, you wouldn't need > >> security_enclave_map(). > > > > Sorry, you want security_enclave_load() to stash a reference to the > > enclave file in some security module-internal state, then match it > > upon later security_mmap_file() calls to determine that it is dealing > > with an enclave, and then adjust its logic accordingly? When do we > > release that reference? > > I guess you mean set a flag in the enclave file security struct upon > security_enclave_load() and check that flag in security_mmap_file(). Yes, by invoking security_enclave_load(), the SGX subsystem indicates to LSM the file struct in subject refers to an enclave. But security_mmap_file() doesn't pass in the range being mmap()'ed so LSM still cannot decide. Instead of changing the definition of security_mmap_file(), I'd invoke security_file_mprotect() from sgx_mmap(). After all, creating a new mapping is equivalent to changing the target range from PROT_NONE to @prot being requested. I just sent out a patch series with all those details in code. > > This seems somewhat similar to one of Sean's alternatives in the patch > description for 06/12, except by pushing the information from sgx to LSM > upon security_enclave_load() rather than pulling it via a > is_sgx_enclave() helper. Not clear if it is still subject to the same > limitations. Yes, they are similar except who keeps track of that piece of information. As Dr. Greg pointed out, the new hooks do have to be SGX specific. But calling is_sgx_enclave() really ties LSM to SGX. In contrast, inferring it through security_enclave_load() makes LSM SGX-agnostic. Then the only SGX specific thing left in the hooks is the sgx_sigstruct. In theory, that's just a digital signature and as Dr. Greg pointed out, SGX is probably not the only technology that uses digital signature to identify executable contents. And in that sense, if we rename it to something generic with probably a tag indicating its format, then the whole thing would become SGX agnostic and could be useful for other TEEs on architectures other than x86.
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index 1fca70a36ce3..ae1b4d69441c 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -9,6 +9,7 @@ #include <linux/highmem.h> #include <linux/ratelimit.h> #include <linux/sched/signal.h> +#include <linux/security.h> #include <linux/shmem_fs.h> #include <linux/slab.h> #include <linux/suspend.h> @@ -564,7 +565,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr, return ret; } -static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) +static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot, + u16 mrmask) { struct vm_area_struct *vma; int ret; @@ -572,24 +574,24 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */ down_read(¤t->mm->mmap_sem); + vma = find_vma(current->mm, src); + if (!vma) { + ret = -EFAULT; + goto out; + } + /* Query vma's VM_MAYEXEC as an indirect path_noexec() check. */ - if (prot & PROT_EXEC) { - vma = find_vma(current->mm, src); - if (!vma) { - ret = -EFAULT; - goto out; - } - - if (!(vma->vm_flags & VM_MAYEXEC)) { - ret = -EACCES; - goto out; - } + if ((prot & PROT_EXEC) && !(vma->vm_flags & VM_MAYEXEC)) { + ret = -EACCES; + goto out; } + ret = security_enclave_load(vma, prot, mrmask == 0xffff); + if (ret) + goto out; + if (copy_from_user(dst, (void __user *)src, PAGE_SIZE)) ret = -EFAULT; - else - ret = 0; out: up_read(¤t->mm->mmap_sem); @@ -639,7 +641,7 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg) prot = addp.prot & (PROT_READ | PROT_WRITE | PROT_EXEC); - ret = sgx_encl_page_copy(data, addp.src, prot); + ret = sgx_encl_page_copy(data, addp.src, prot, addp.mrmask); if (ret) goto out; diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 7c1357105e61..3bc92c65f287 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1451,6 +1451,11 @@ * @enclave_map: * @prot contains the protection that will be applied by the kernel. * Return 0 if permission is granted. + * + * @enclave_load: + * @vma: the source memory region of the enclave page being loaded. + * @prot: the (maximal) protections of the enclave page. + * Return 0 if permission is granted. */ union security_list_options { int (*binder_set_context_mgr)(struct task_struct *mgr); @@ -1815,6 +1820,8 @@ union security_list_options { #ifdef CONFIG_INTEL_SGX int (*enclave_map)(unsigned long prot); + int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot, + bool measured); #endif /* CONFIG_INTEL_SGX */ }; @@ -2057,6 +2064,7 @@ struct security_hook_heads { #endif /* CONFIG_BPF_SYSCALL */ #ifdef CONFIG_INTEL_SGX struct hlist_head enclave_map; + struct hlist_head enclave_load; #endif /* CONFIG_INTEL_SGX */ } __randomize_layout; diff --git a/include/linux/security.h b/include/linux/security.h index 6a1f54ba6794..572ddfc53039 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1832,11 +1832,18 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux) #ifdef CONFIG_INTEL_SGX #ifdef CONFIG_SECURITY int security_enclave_map(unsigned long prot); +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot, + bool measured); #else static inline int security_enclave_map(unsigned long prot) { return 0; } +static inline int security_enclave_load(struct vm_area_struct *vma, + unsigned long prot, bool measured) +{ + return 0; +} #endif /* CONFIG_SECURITY */ #endif /* CONFIG_INTEL_SGX */ diff --git a/security/security.c b/security/security.c index 03951e08bdfc..00f483beb1cc 100644 --- a/security/security.c +++ b/security/security.c @@ -2365,4 +2365,9 @@ int security_enclave_map(unsigned long prot) { return call_int_hook(enclave_map, 0, prot); } +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot, + bool measured) +{ + return call_int_hook(enclave_load, 0, vma, prot, measured); +} #endif /* CONFIG_INTEL_SGX */
enclave_load() is roughly analogous to the existing file_mprotect(). Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be MAP_SHARED. Furthermore, all enclaves need read, write and execute VMAs. As a result, the existing/standard call to file_mprotect() does not provide any meaningful security for enclaves since an LSM can only deny/grant access to the EPC as a whole. security_enclave_load() is called when SGX is first loading an enclave page, i.e. copying a page from normal memory into the EPC. Although the prototype for enclave_load() is similar to file_mprotect(), e.g. SGX could theoretically use file_mprotect() and set reqprot=prot, a separate hook is desirable as the semantics of an enclave's protection bits are different than those of vmas, e.g. an enclave page tracks the maximal set of protections, whereas file_mprotect() operates on the actual protections being provided. Enclaves also have unique security properties, e.g. measured code, that LSMs may want to consider. In other words, LSMs will likely want to implement different policies for enclave page protections. Note, extensive discussion yielded no sane alternative to some form of SGX specific LSM hook[1]. [1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 32 ++++++++++++++------------ include/linux/lsm_hooks.h | 8 +++++++ include/linux/security.h | 7 ++++++ security/security.c | 5 ++++ 4 files changed, 37 insertions(+), 15 deletions(-)