| Message ID | 20190606021145.12604-5-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
On 6/5/19 10:11 PM, Sean Christopherson wrote: > 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. 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 | 12 ++++++------ > include/linux/lsm_hooks.h | 13 +++++++++++++ > include/linux/security.h | 12 ++++++++++++ > security/security.c | 7 +++++++ > 4 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > index 44b2d73de7c3..29c0df672250 100644 > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > @@ -8,6 +8,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> > @@ -582,9 +583,6 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) > struct vm_area_struct *vma; > int ret; > > - if (!(prot & VM_EXEC)) > - return 0; > - Is there a real use case where LSM will want to be called if !(prot & VM_EXEC)? Also, you seem to be mixing prot and PROT_EXEC with vm_flags and VM_EXEC; other code does not appear to assume they are identical and explicitly converts, e.g. calc_vm_prot_bits(). > /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */ > down_read(¤t->mm->mmap_sem); > > @@ -599,15 +597,17 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) > * but with some future proofing against other cases that may deny > * execute permissions. > */ > - if (!(vma->vm_flags & VM_MAYEXEC)) { > + if ((prot & VM_EXEC) && !(vma->vm_flags & VM_MAYEXEC)) { > ret = -EACCES; > goto out; > } > > + ret = security_enclave_load(vma, prot); > + 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); > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 47f58cfb6a19..c6f47a7eef70 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1446,6 +1446,12 @@ > * @bpf_prog_free_security: > * Clean up the security information stored inside bpf prog. > * > + * Security hooks for Intel SGX enclaves. > + * > + * @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); > @@ -1807,6 +1813,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_load)(struct vm_area_struct *vma, unsigned long prot); > +#endif /* CONFIG_INTEL_SGX */ > }; > > struct security_hook_heads { > @@ -2046,6 +2056,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_load; > +#endif /* CONFIG_INTEL_SGX */ > } __randomize_layout; > > /* > diff --git a/include/linux/security.h b/include/linux/security.h > index 659071c2e57c..0b6d1eb7368b 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -1829,5 +1829,17 @@ 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_load(struct vm_area_struct *vma, unsigned long prot); > +#else > +static inline int security_enclave_load(struct vm_area_struct *vma, > + 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..c6f7f26969b2 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_load(struct vm_area_struct *vma, unsigned long prot) > +{ > + return call_int_hook(enclave_load, 0, vma, prot); > +} > +#endif /* CONFIG_INTEL_SGX */ >
On Wed, Jun 05, 2019 at 07:11:44PM -0700, Sean Christopherson wrote: > 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. 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> 4/5 and 5/5 should only be added after upstreaming SGX. /Jarkko
On Fri, Jun 07, 2019 at 03:58:34PM -0400, Stephen Smalley wrote: > On 6/5/19 10:11 PM, Sean Christopherson wrote: > >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. 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 | 12 ++++++------ > > include/linux/lsm_hooks.h | 13 +++++++++++++ > > include/linux/security.h | 12 ++++++++++++ > > security/security.c | 7 +++++++ > > 4 files changed, 38 insertions(+), 6 deletions(-) > > > >diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > >index 44b2d73de7c3..29c0df672250 100644 > >--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > >+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > >@@ -8,6 +8,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> > >@@ -582,9 +583,6 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) > > struct vm_area_struct *vma; > > int ret; > >- if (!(prot & VM_EXEC)) > >- return 0; > >- > > Is there a real use case where LSM will want to be called if !(prot & > VM_EXEC)? I don't think so? I have no objection to conditioning the LSM calls on the page being executable. I actually had the code written that way in the first RFC, but it felt weird for SGX to be making assumptions about LSM use cases. > Also, you seem to be mixing prot and PROT_EXEC with vm_flags and > VM_EXEC; other code does not appear to assume they are identical and > explicitly converts, e.g. calc_vm_prot_bits(). Argh, I'll clean that up. > > /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */ > > down_read(¤t->mm->mmap_sem);
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index 44b2d73de7c3..29c0df672250 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -8,6 +8,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> @@ -582,9 +583,6 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) struct vm_area_struct *vma; int ret; - if (!(prot & VM_EXEC)) - return 0; - /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */ down_read(¤t->mm->mmap_sem); @@ -599,15 +597,17 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot) * but with some future proofing against other cases that may deny * execute permissions. */ - if (!(vma->vm_flags & VM_MAYEXEC)) { + if ((prot & VM_EXEC) && !(vma->vm_flags & VM_MAYEXEC)) { ret = -EACCES; goto out; } + ret = security_enclave_load(vma, prot); + 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); diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 47f58cfb6a19..c6f47a7eef70 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1446,6 +1446,12 @@ * @bpf_prog_free_security: * Clean up the security information stored inside bpf prog. * + * Security hooks for Intel SGX enclaves. + * + * @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); @@ -1807,6 +1813,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_load)(struct vm_area_struct *vma, unsigned long prot); +#endif /* CONFIG_INTEL_SGX */ }; struct security_hook_heads { @@ -2046,6 +2056,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_load; +#endif /* CONFIG_INTEL_SGX */ } __randomize_layout; /* diff --git a/include/linux/security.h b/include/linux/security.h index 659071c2e57c..0b6d1eb7368b 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1829,5 +1829,17 @@ 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_load(struct vm_area_struct *vma, unsigned long prot); +#else +static inline int security_enclave_load(struct vm_area_struct *vma, + 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..c6f7f26969b2 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_load(struct vm_area_struct *vma, unsigned long prot) +{ + return call_int_hook(enclave_load, 0, vma, prot); +} +#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. 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 | 12 ++++++------ include/linux/lsm_hooks.h | 13 +++++++++++++ include/linux/security.h | 12 ++++++++++++ security/security.c | 7 +++++++ 4 files changed, 38 insertions(+), 6 deletions(-)