| Message ID | 20190619222401.14942-8-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
On Wed, Jun 19, 2019 at 03:23:56PM -0700, Sean Christopherson wrote: > enclave_map() is an SGX specific variant of file_mprotect() and > mmap_file(), and is provided so that LSMs can apply W^X restrictions to > enclaves. > > 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, applying W^X restrictions on /dev/sgx/enclave using > existing LSM hooks is for all intents and purposes impossible, e.g. > denying either W or X would deny access to any enclave. > > 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> All the non-LSM changes are almost cleared from my part. I would suggest that we scrape v21 together as soon as you return from your vacation discluding the LSM hooks. There is no any particular reason to get the LSM changes to the mainline before the SGX foundations so now is the right time close things underlying them. I'm now in the same boat with your changes to the ioctl API, which means that we are ready to go. I feel a tiny bit bad that it took me so long time with [1] but I'm a simple minded person so what I can do :-) Once you can come back please deal with the suggestions that I made and provide a "pure" SRCU patch (apologies for repeating myself). I will the squash them to the existing patch set. After that is fully done we can make v21 scope decision when it comes to the enclave life-cycle. Even if the LSM changes would not be upstreamed as part of the foundations I can start holding versions of them in my tree but only after v21 is out. Can you cope with this plan? [1] https://patchwork.kernel.org/patch/11005431/ /Jarkko
> From: Christopherson, Sean J > Sent: Wednesday, June 19, 2019 3:24 PM > > diff --git a/security/security.c b/security/security.c > index 613a5c00e602..03951e08bdfc 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux) > call_void_hook(bpf_prog_free_security, aux); > } > #endif /* CONFIG_BPF_SYSCALL */ > + > +#ifdef CONFIG_INTEL_SGX > +int security_enclave_map(unsigned long prot) > +{ > + return call_int_hook(enclave_map, 0, prot); > +} > +#endif /* CONFIG_INTEL_SGX */ Why is this new security_enclave_map() necessary while security_mmap_file() will also be invoked?
On 6/21/19 12:54 PM, Xing, Cedric wrote: >> From: Christopherson, Sean J >> Sent: Wednesday, June 19, 2019 3:24 PM >> >> diff --git a/security/security.c b/security/security.c >> index 613a5c00e602..03951e08bdfc 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux) >> call_void_hook(bpf_prog_free_security, aux); >> } >> #endif /* CONFIG_BPF_SYSCALL */ >> + >> +#ifdef CONFIG_INTEL_SGX >> +int security_enclave_map(unsigned long prot) >> +{ >> + return call_int_hook(enclave_map, 0, prot); >> +} >> +#endif /* CONFIG_INTEL_SGX */ > > Why is this new security_enclave_map() necessary while security_mmap_file() will also be invoked? security_mmap_file() doesn't know about enclaves. It will just end up checking FILE__READ, FILE__WRITE, and FILE__EXECUTE to /dev/sgx/enclave. This was noted in the patch description.
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > owner@vger.kernel.org] On Behalf Of Stephen Smalley > Sent: Tuesday, June 25, 2019 1:48 PM > > On 6/21/19 12:54 PM, Xing, Cedric wrote: > >> From: Christopherson, Sean J > >> Sent: Wednesday, June 19, 2019 3:24 PM > >> > >> diff --git a/security/security.c b/security/security.c index > >> 613a5c00e602..03951e08bdfc 100644 > >> --- a/security/security.c > >> +++ b/security/security.c > >> @@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct > bpf_prog_aux *aux) > >> call_void_hook(bpf_prog_free_security, aux); > >> } > >> #endif /* CONFIG_BPF_SYSCALL */ > >> + > >> +#ifdef CONFIG_INTEL_SGX > >> +int security_enclave_map(unsigned long prot) { > >> + return call_int_hook(enclave_map, 0, prot); } #endif /* > >> +CONFIG_INTEL_SGX */ > > > > Why is this new security_enclave_map() necessary while > security_mmap_file() will also be invoked? > > security_mmap_file() doesn't know about enclaves. It will just end up > checking FILE__READ, FILE__WRITE, and FILE__EXECUTE to /dev/sgx/enclave. > This was noted in the patch description. Surely I understand all those. As I mentioned in my other email, enclave_load() could indicate to LSM that a file is an enclave. Of course mmap() could be invoked before any pages are loaded so LSM wouldn't know at the first mmap(), but that doesn't matter as an empty enclave wouldn't post any threats anyway.
On Thu, Jun 27, 2019 at 01:29:39PM -0700, Xing, Cedric wrote: > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > > owner@vger.kernel.org] On Behalf Of Stephen Smalley > > Sent: Tuesday, June 25, 2019 1:48 PM > > > > On 6/21/19 12:54 PM, Xing, Cedric wrote: > > >> From: Christopherson, Sean J > > >> Sent: Wednesday, June 19, 2019 3:24 PM > > >> > > >> diff --git a/security/security.c b/security/security.c index > > >> 613a5c00e602..03951e08bdfc 100644 > > >> --- a/security/security.c > > >> +++ b/security/security.c > > >> @@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct > > bpf_prog_aux *aux) > > >> call_void_hook(bpf_prog_free_security, aux); > > >> } > > >> #endif /* CONFIG_BPF_SYSCALL */ > > >> + > > >> +#ifdef CONFIG_INTEL_SGX > > >> +int security_enclave_map(unsigned long prot) { > > >> + return call_int_hook(enclave_map, 0, prot); } #endif /* > > >> +CONFIG_INTEL_SGX */ > > > > > > Why is this new security_enclave_map() necessary while > > security_mmap_file() will also be invoked? > > > > security_mmap_file() doesn't know about enclaves. It will just end up > > checking FILE__READ, FILE__WRITE, and FILE__EXECUTE to /dev/sgx/enclave. > > This was noted in the patch description. > > Surely I understand all those. As I mentioned in my other email, > enclave_load() could indicate to LSM that a file is an enclave. Of course > mmap() could be invoked before any pages are loaded so LSM wouldn't know at > the first mmap(), but that doesn't matter as an empty enclave wouldn't post > any threats anyway. security_mmap_file() is invoked before the final address is known, and MAP_FIXED isn't technically required.
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c index dabfe2a7245a..4379a2fb1f82 100644 --- a/arch/x86/kernel/cpu/sgx/driver/main.c +++ b/arch/x86/kernel/cpu/sgx/driver/main.c @@ -133,13 +133,20 @@ static unsigned long sgx_allowed_rwx(struct sgx_encl *encl, static int sgx_mmap(struct file *file, struct vm_area_struct *vma) { struct sgx_encl *encl = file->private_data; - unsigned long allowed_rwx; + unsigned long allowed_rwx, prot; int ret; allowed_rwx = sgx_allowed_rwx(encl, vma); if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx) return -EACCES; + prot = _calc_vm_trans(vma->vm_flags, VM_READ, PROT_READ) | + _calc_vm_trans(vma->vm_flags, VM_WRITE, PROT_WRITE) | + _calc_vm_trans(vma->vm_flags, VM_EXEC, PROT_EXEC); + ret = security_enclave_map(prot); + if (ret) + return ret; + ret = sgx_encl_mm_add(encl, vma->vm_mm); if (ret) return ret; diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index c6436bbd4a68..059d90dcaa27 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -2,6 +2,7 @@ // Copyright(c) 2016-18 Intel Corporation. #include <linux/mm.h> +#include <linux/security.h> #include <linux/shmem_fs.h> #include <linux/suspend.h> #include <linux/sched/mm.h> @@ -387,9 +388,20 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr, return ret < 0 ? ret : i; } +#ifdef CONFIG_SECURITY +static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start, + unsigned long end, unsigned long prot) +{ + return security_enclave_map(prot); +} +#endif + const struct vm_operations_struct sgx_vm_ops = { .fault = sgx_vma_fault, .access = sgx_vma_access, +#ifdef CONFIG_SECURITY + .may_mprotect = sgx_vma_mprotect, +#endif }; /** diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 47f58cfb6a19..7c1357105e61 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1446,6 +1446,11 @@ * @bpf_prog_free_security: * Clean up the security information stored inside bpf prog. * + * Security hooks for Intel SGX enclaves. + * + * @enclave_map: + * @prot contains the protection that will be applied by the kernel. + * Return 0 if permission is granted. */ union security_list_options { int (*binder_set_context_mgr)(struct task_struct *mgr); @@ -1807,6 +1812,10 @@ union security_list_options { int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux); void (*bpf_prog_free_security)(struct bpf_prog_aux *aux); #endif /* CONFIG_BPF_SYSCALL */ + +#ifdef CONFIG_INTEL_SGX + int (*enclave_map)(unsigned long prot); +#endif /* CONFIG_INTEL_SGX */ }; struct security_hook_heads { @@ -2046,6 +2055,9 @@ struct security_hook_heads { struct hlist_head bpf_prog_alloc_security; struct hlist_head bpf_prog_free_security; #endif /* CONFIG_BPF_SYSCALL */ +#ifdef CONFIG_INTEL_SGX + struct hlist_head enclave_map; +#endif /* CONFIG_INTEL_SGX */ } __randomize_layout; /* diff --git a/include/linux/security.h b/include/linux/security.h index 659071c2e57c..6a1f54ba6794 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1829,5 +1829,16 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux) #endif /* CONFIG_SECURITY */ #endif /* CONFIG_BPF_SYSCALL */ +#ifdef CONFIG_INTEL_SGX +#ifdef CONFIG_SECURITY +int security_enclave_map(unsigned long prot); +#else +static inline int security_enclave_map(unsigned long prot) +{ + return 0; +} +#endif /* CONFIG_SECURITY */ +#endif /* CONFIG_INTEL_SGX */ + #endif /* ! __LINUX_SECURITY_H */ diff --git a/security/security.c b/security/security.c index 613a5c00e602..03951e08bdfc 100644 --- a/security/security.c +++ b/security/security.c @@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux) call_void_hook(bpf_prog_free_security, aux); } #endif /* CONFIG_BPF_SYSCALL */ + +#ifdef CONFIG_INTEL_SGX +int security_enclave_map(unsigned long prot) +{ + return call_int_hook(enclave_map, 0, prot); +} +#endif /* CONFIG_INTEL_SGX */
enclave_map() is an SGX specific variant of file_mprotect() and mmap_file(), and is provided so that LSMs can apply W^X restrictions to enclaves. 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, applying W^X restrictions on /dev/sgx/enclave using existing LSM hooks is for all intents and purposes impossible, e.g. denying either W or X would deny access to any enclave. 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/main.c | 9 ++++++++- arch/x86/kernel/cpu/sgx/encl.c | 12 ++++++++++++ include/linux/lsm_hooks.h | 12 ++++++++++++ include/linux/security.h | 11 +++++++++++ security/security.c | 7 +++++++ 5 files changed, 50 insertions(+), 1 deletion(-)