| Message ID | 20190617222438.2080-11-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM, round 3 | expand |
On 6/17/19 6:24 PM, Sean Christopherson wrote: > The goal of selinux_enclave_load() is to provide a facsimile of the > existing selinux_file_mprotect() and file_map_prot_check() policies, > but tailored to the unique properties of SGX. > > For example, an enclave page is technically backed by a MAP_SHARED file, > but the "file" is essentially shared memory that is never persisted > anywhere and also requires execute permissions (for some pages). > > Enclaves are also less priveleged than normal user code, e.g. SYSCALL > instructions #UD if attempted in an enclave. For this reason, add SGX > specific permissions instead of reusing existing permissions such as > FILE__EXECUTE so that policies can allow running code in an enclave, or > allow dynamically loading code in an enclave without having to grant the > same capability to normal user code outside of the enclave. > > Intended use of each permission: > > - SGX_EXECMOD: dynamically load code within the enclave itself > - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene > - SGX_EXECANON: load code from anonymous memory (likely Graphene) > - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior > > Note, equivalents to FILE__READ and FILE__WRITE are intentionally never > required. Writes to the enclave page are contained to the EPC, i.e. > never hit the original file, and read permissions have already been > vetted (or the VMA doesn't have PROT_READ, in which case loading the > page into the enclave will fail). > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > security/selinux/hooks.c | 55 +++++++++++++++++++++++++++-- > security/selinux/include/classmap.h | 5 +-- > 2 files changed, 55 insertions(+), 5 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 22e0f4a71333..ea452a416fe1 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) > #endif > > #ifdef CONFIG_INTEL_SGX > +static inline int sgx_has_perm(u32 sid, u32 requested) > +{ > + return avc_has_perm(&selinux_state, sid, sid, > + SECCLASS_PROCESS2, requested, NULL); > +} > + > static int selinux_enclave_map(unsigned long prot) > { > const struct cred *cred = current_cred(); > @@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot) > WARN_ON_ONCE(!default_noexec); > > if ((prot & PROT_EXEC) && (prot & PROT_WRITE)) > - return avc_has_perm(&selinux_state, sid, sid, > - SECCLASS_PROCESS2, PROCESS2__SGX_EXECMEM, > - NULL); > + return sgx_has_perm(sid, PROCESS2__SGX_EXECMEM); > + > return 0; > } > + > +static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot, > + bool measured) > +{ > + const struct cred *cred = current_cred(); > + u32 sid = cred_sid(cred); > + int ret; > + > + /* SGX is supported only in 64-bit kernels. */ > + WARN_ON_ONCE(!default_noexec); > + > + /* Only executable enclave pages are restricted in any way. */ > + if (!(prot & PROT_EXEC)) > + return 0; > + > + /* > + * WX at load time only requires EXECMOD, e.g. to allow W->X. Actual > + * WX mappings require EXECMEM (see selinux_enclave_map()). > + */ > + if (prot & PROT_WRITE) { > + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECMOD); So, security_enclave_load() can be called with PROT_WRITE|PROT_EXEC, but the subsequent calls to security_enclave_map() won't have both set unless a mapping is actually simultaneously WX? Is that correct? Trying to make sure that every PROCESS2__SGX_EXECMOD check here won't be followed immediately by a PROCESS2__SGX_EXECMEM check from security_enclave_map(), thereby rendering any distinction between them moot. > + if (ret) > + goto out; > + } > + if (!measured) { > + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR); I'm unclear on the concept of loading unmeasured code into an enclave; I thought that wasn't supposed to happen apart from SGX2. How does that occur? > + if (ret) > + goto out; > + } > + > + if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) || I think this should be IS_PRIVATE(), not !IS_PRIVATE()? > + vma->anon_vma) > + /* > + * Loading enclave code from an anonymous mapping or from a > + * modified private file mapping. > + */ > + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON); I might have expected this to be EXECMEM. It is actually a blend of EXECMEM and EXECMOD, so I'm ok with the new name. However, I'm wondering if you should use an entirely different set of permission names than EXECMEM or EXECMOD for the other checks too since none of them quite align with the existing usage; your SGX__EXECMEM is effectively MAPWX and your SGX__EXECMOD is effectively MAPXAFTERW. Or something like that. > + else > + /* Loading from a shared or unmodified private file mapping. */ > + ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE); > +out: > + return ret; > +} > #endif > > struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { > @@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > > #ifdef CONFIG_INTEL_SGX > LSM_HOOK_INIT(enclave_map, selinux_enclave_map), > + LSM_HOOK_INIT(enclave_load, selinux_enclave_load), > #endif > }; > > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 0f525f5b926f..29a0a74268cd 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = { > "setsockcreate", "getrlimit", NULL } }, > { "process2", > { "nnp_transition", "nosuid_transition", > - "sgx_execmem", NULL } }, > + "sgx_execmem", "sgx_execmod", "sgx_execanon", "sgx_execunmr", > + NULL } }, > { "system", > { "ipc_info", "syslog_read", "syslog_mod", > "syslog_console", "module_request", "module_load", NULL } }, > @@ -64,7 +65,7 @@ struct security_class_mapping secclass_map[] = { > "quotaget", NULL } }, > { "file", > { COMMON_FILE_PERMS, > - "execute_no_trans", "entrypoint", NULL } }, > + "execute_no_trans", "entrypoint", "sgx_execute", NULL } }, If there is any possibility of a mapping of a non-regular file reaching the point of the permission check, then the permission needs to either be added to COMMON_FILE_PERMS or be added to each of the *_file classes for which it can legitimately be checked. That's why execmod is in COMMON_FILE_PERMS; it can show up on e.g. a modified private file mapping of a device file in addition to regular files. > { "dir", > { COMMON_FILE_PERMS, "add_name", "remove_name", > "reparent", "search", "rmdir", NULL } }, >
On Tue, Jun 18, 2019 at 10:49:55AM -0400, Stephen Smalley wrote: > On 6/17/19 6:24 PM, Sean Christopherson wrote: > >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >index 22e0f4a71333..ea452a416fe1 100644 > >--- a/security/selinux/hooks.c > >+++ b/security/selinux/hooks.c > >@@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) > > #endif > > #ifdef CONFIG_INTEL_SGX > >+static inline int sgx_has_perm(u32 sid, u32 requested) > >+{ > >+ return avc_has_perm(&selinux_state, sid, sid, > >+ SECCLASS_PROCESS2, requested, NULL); > >+} > >+ > > static int selinux_enclave_map(unsigned long prot) > > { > > const struct cred *cred = current_cred(); > >@@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot) > > WARN_ON_ONCE(!default_noexec); > > if ((prot & PROT_EXEC) && (prot & PROT_WRITE)) > >- return avc_has_perm(&selinux_state, sid, sid, > >- SECCLASS_PROCESS2, PROCESS2__SGX_EXECMEM, > >- NULL); > >+ return sgx_has_perm(sid, PROCESS2__SGX_EXECMEM); > >+ > > return 0; > > } > >+ > >+static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot, > >+ bool measured) > >+{ > >+ const struct cred *cred = current_cred(); > >+ u32 sid = cred_sid(cred); > >+ int ret; > >+ > >+ /* SGX is supported only in 64-bit kernels. */ > >+ WARN_ON_ONCE(!default_noexec); > >+ > >+ /* Only executable enclave pages are restricted in any way. */ > >+ if (!(prot & PROT_EXEC)) > >+ return 0; > >+ > >+ /* > >+ * WX at load time only requires EXECMOD, e.g. to allow W->X. Actual > >+ * WX mappings require EXECMEM (see selinux_enclave_map()). > >+ */ > >+ if (prot & PROT_WRITE) { > >+ ret = sgx_has_perm(sid, PROCESS2__SGX_EXECMOD); > > So, security_enclave_load() can be called with PROT_WRITE|PROT_EXEC, but the > subsequent calls to security_enclave_map() won't have both set unless a > mapping is actually simultaneously WX? Is that correct? Trying to make sure > that every PROCESS2__SGX_EXECMOD check here won't be followed immediately by > a PROCESS2__SGX_EXECMEM check from security_enclave_map(), thereby rendering > any distinction between them moot. Yes, @prot passed to security_enclave_map() will be the actual protection bits being set by mmap() or mprotect(). Note, I intentionally omitted @reqprot from .enclave_map() as SGX doesn't have that information when it calls security_enclave_map() in its mmap() hook. Given that checking @reqprot reduces security, I assumed this is a non-issue. > >+ if (ret) > >+ goto out; > >+ } > >+ if (!measured) { > >+ ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR); > > I'm unclear on the concept of loading unmeasured code into an enclave; I > thought that wasn't supposed to happen apart from SGX2. How does that > occur? Adding a page, i.e. EADD, updates enclave's measurement with the offset and protection bits of the page, but not the page contents. The contents of the page are included in the enclave measurement by executing EEXTEND. Whether or not a page (technically 256 byte chunks) is measured is purely controlled by userspace (via the @mrmask param in the add page ioctl()). Put differently, SGX ISA does not require code pages to be measured, nor does the SGX driver. The SGX driver also does *not* zero out unmeasured pages. This means an enclave can load unmeasured RX or X pages. The expected use case is Graphene and the like, which is essentially a two step load. The measured enclave code is a shim that wraps an unmodified non-SGX application, e.g. redis. Before running the unmeasured code, the measured portion of the enclave verifies the unmeasured code. > >+ if (ret) > >+ goto out; > >+ } > >+ > >+ if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) || > > I think this should be IS_PRIVATE(), not !IS_PRIVATE()? Argh, thanks! > >+ vma->anon_vma) > >+ /* > >+ * Loading enclave code from an anonymous mapping or from a > >+ * modified private file mapping. > >+ */ > >+ ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON); > > I might have expected this to be EXECMEM. It is actually a blend of EXECMEM > and EXECMOD, so I'm ok with the new name. Ya, my thought was that we'd want to distinguish between building an enclave in memory (EXECANON) and the enclave itself modifying its code (EXECMOD). > However, I'm wondering if you > should use an entirely different set of permission names than EXECMEM or > EXECMOD for the other checks too since none of them quite align with the > existing usage; your SGX__EXECMEM is effectively MAPWX and your SGX__EXECMOD > is effectively MAPXAFTERW. Or something like that. Hmm, I was thinking that reusing the names would aid in understanding the SGX specific concepts, but it's definitely a double edge sword. How about SGX__MAPWX (was EXECMEM) and SGX__EXECDIRTY (was EXECMOD)? MAPXAFTERW isn't technically accurate for the SGX1 case of loading a WX page, e.g. the permission is required even if the page is never mapped. > >+ else > >+ /* Loading from a shared or unmodified private file mapping. */ > >+ ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE); > >+out: > >+ return ret; > >+} > > #endif > > struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { > >@@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > > #ifdef CONFIG_INTEL_SGX > > LSM_HOOK_INIT(enclave_map, selinux_enclave_map), > >+ LSM_HOOK_INIT(enclave_load, selinux_enclave_load), > > #endif > > }; > >diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > >index 0f525f5b926f..29a0a74268cd 100644 > >--- a/security/selinux/include/classmap.h > >+++ b/security/selinux/include/classmap.h > >@@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = { > > "setsockcreate", "getrlimit", NULL } }, > > { "process2", > > { "nnp_transition", "nosuid_transition", > >- "sgx_execmem", NULL } }, > >+ "sgx_execmem", "sgx_execmod", "sgx_execanon", "sgx_execunmr", > >+ NULL } }, > > { "system", > > { "ipc_info", "syslog_read", "syslog_mod", > > "syslog_console", "module_request", "module_load", NULL } }, > >@@ -64,7 +65,7 @@ struct security_class_mapping secclass_map[] = { > > "quotaget", NULL } }, > > { "file", > > { COMMON_FILE_PERMS, > >- "execute_no_trans", "entrypoint", NULL } }, > >+ "execute_no_trans", "entrypoint", "sgx_execute", NULL } }, > > If there is any possibility of a mapping of a non-regular file reaching the > point of the permission check, then the permission needs to either be added > to COMMON_FILE_PERMS or be added to each of the *_file classes for which it > can legitimately be checked. That's why execmod is in COMMON_FILE_PERMS; it > can show up on e.g. a modified private file mapping of a device file in > addition to regular files. Ah, yes. Adding it to COMMON_FILE_PERMS seems like the safest bet. > > { "dir", > > { COMMON_FILE_PERMS, "add_name", "remove_name", > > "reparent", "search", "rmdir", NULL } }, > > >
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 22e0f4a71333..ea452a416fe1 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) #endif #ifdef CONFIG_INTEL_SGX +static inline int sgx_has_perm(u32 sid, u32 requested) +{ + return avc_has_perm(&selinux_state, sid, sid, + SECCLASS_PROCESS2, requested, NULL); +} + static int selinux_enclave_map(unsigned long prot) { const struct cred *cred = current_cred(); @@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot) WARN_ON_ONCE(!default_noexec); if ((prot & PROT_EXEC) && (prot & PROT_WRITE)) - return avc_has_perm(&selinux_state, sid, sid, - SECCLASS_PROCESS2, PROCESS2__SGX_EXECMEM, - NULL); + return sgx_has_perm(sid, PROCESS2__SGX_EXECMEM); + return 0; } + +static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot, + bool measured) +{ + const struct cred *cred = current_cred(); + u32 sid = cred_sid(cred); + int ret; + + /* SGX is supported only in 64-bit kernels. */ + WARN_ON_ONCE(!default_noexec); + + /* Only executable enclave pages are restricted in any way. */ + if (!(prot & PROT_EXEC)) + return 0; + + /* + * WX at load time only requires EXECMOD, e.g. to allow W->X. Actual + * WX mappings require EXECMEM (see selinux_enclave_map()). + */ + if (prot & PROT_WRITE) { + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECMOD); + if (ret) + goto out; + } + if (!measured) { + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR); + if (ret) + goto out; + } + + if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) || + vma->anon_vma) + /* + * Loading enclave code from an anonymous mapping or from a + * modified private file mapping. + */ + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON); + else + /* Loading from a shared or unmodified private file mapping. */ + ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE); +out: + return ret; +} #endif struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { @@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { #ifdef CONFIG_INTEL_SGX LSM_HOOK_INIT(enclave_map, selinux_enclave_map), + LSM_HOOK_INIT(enclave_load, selinux_enclave_load), #endif }; diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 0f525f5b926f..29a0a74268cd 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = { "setsockcreate", "getrlimit", NULL } }, { "process2", { "nnp_transition", "nosuid_transition", - "sgx_execmem", NULL } }, + "sgx_execmem", "sgx_execmod", "sgx_execanon", "sgx_execunmr", + NULL } }, { "system", { "ipc_info", "syslog_read", "syslog_mod", "syslog_console", "module_request", "module_load", NULL } }, @@ -64,7 +65,7 @@ struct security_class_mapping secclass_map[] = { "quotaget", NULL } }, { "file", { COMMON_FILE_PERMS, - "execute_no_trans", "entrypoint", NULL } }, + "execute_no_trans", "entrypoint", "sgx_execute", NULL } }, { "dir", { COMMON_FILE_PERMS, "add_name", "remove_name", "reparent", "search", "rmdir", NULL } },
The goal of selinux_enclave_load() is to provide a facsimile of the existing selinux_file_mprotect() and file_map_prot_check() policies, but tailored to the unique properties of SGX. For example, an enclave page is technically backed by a MAP_SHARED file, but the "file" is essentially shared memory that is never persisted anywhere and also requires execute permissions (for some pages). Enclaves are also less priveleged than normal user code, e.g. SYSCALL instructions #UD if attempted in an enclave. For this reason, add SGX specific permissions instead of reusing existing permissions such as FILE__EXECUTE so that policies can allow running code in an enclave, or allow dynamically loading code in an enclave without having to grant the same capability to normal user code outside of the enclave. Intended use of each permission: - SGX_EXECMOD: dynamically load code within the enclave itself - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene - SGX_EXECANON: load code from anonymous memory (likely Graphene) - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior Note, equivalents to FILE__READ and FILE__WRITE are intentionally never required. Writes to the enclave page are contained to the EPC, i.e. never hit the original file, and read permissions have already been vetted (or the VMA doesn't have PROT_READ, in which case loading the page into the enclave will fail). Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- security/selinux/hooks.c | 55 +++++++++++++++++++++++++++-- security/selinux/include/classmap.h | 5 +-- 2 files changed, 55 insertions(+), 5 deletions(-)