Messages in this thread Patch in this message | | | Subject | Re: [RFC PATCH 9/9] security/selinux: Add enclave_load() implementation | From | Stephen Smalley <> | Date | Mon, 3 Jun 2019 11:01:32 -0400 |
| |
On 5/31/19 7:31 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). > > The basic concept is to require appropriate execute permissions on the > source of the enclave for pages that are requesting PROT_EXEC, e.g. if > an enclave page is being loaded from a regular file, require > FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an > anonymous/private mapping, require PROCESS__EXECMEM since the process > is essentially executing from the mapping, albeit in a roundabout way. > > Note, FILE__READ and FILE__WRITE are intentionally not required even if > the source page is backed by a regular file. 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 | 85 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 3ec702cf46ca..f436a055dda7 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6726,6 +6726,87 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) > } > #endif > > +#ifdef CONFIG_INTEL_SGX > +int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot, > + unsigned long *allowed_prot) > +{ > + const struct cred *cred = current_cred(); > + u32 sid = cred_sid(cred); > + int rc; > + > + /* SGX is supported only in 64-bit kernels. */ > + WARN_ON_ONCE(!default_noexec); > + > + /* > + * SGX is responsible for checking @prot vs @allowed_prot, and SELinux > + * only cares about execute related permissions for enclaves. > + */ > + if (!(*allowed_prot & PROT_EXEC)) > + return 0; > + > + /* > + * Loading an executable enclave page from a VMA that is not executable > + * itself requires EXECUTE permissions on the source file, or if there > + * is no regular source file, EXECMEM since the page is being loaded > + * from a non-executable anonymous mapping. > + */ > + if (!(vma->vm_flags & VM_EXEC)) { > + if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file))) > + rc = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
We might need an EXECMOD check here as well if (vma->vm_file && vma->anon_vma). The scenario would be that the host application mapped the file with PROT_WRITE, modified it, but haven't mapped it PROT_EXEC. Now the enclave loader requests PROT_EXEC without PROT_WRITE or allows it. FILE__EXECUTE is insufficient for this case.
> + else > + rc = avc_has_perm(&selinux_state, > + sid, sid, SECCLASS_PROCESS, > + PROCESS__EXECMEM, NULL);
These calls will audit FILE__EXECUTE or PROCESS__EXECMEM denials even when userspace never asked for PROT_EXEC. Possibly we should use avc_has_perm_noaudit() and only call avc_audit() if (prot & PROT_EXEC)? And similarly introduce file_has_perm_noaudit() -> inode_has_perm_noaudit() -> avc_has_perm_noaudit() or inline here and switch to avc_has_perm_noaudit() throughout?
> + > + /* > + * Reject the load if the enclave *needs* the page to be > + * executable, otherwise prevent it from becoming executable. > + */ > + if (rc) { > + if (prot & PROT_EXEC) > + return rc; > + > + *allowed_prot &= ~PROT_EXEC; > + } > + } > + > + /* > + * An enclave page that may do RW->RX or W+X requires EXECMOD (backed > + * by a regular file) or EXECMEM (loaded from an anonymous mapping).
At present EXECMEM is also triggered for W+X private file mappings, to allow denying W+X while permitting exceptions for W->X for text relocations.
> + * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and > + * reflects the nature of enclaves and the EPC, e.g. EPC is effectively > + * a non-persistent shared file, but each enclave is a private domain > + * within that shared file, so delegate to the source of the enclave. > + */ > + if ((*allowed_prot & PROT_EXEC) && (*allowed_prot & PROT_WRITE)) { > + if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file))) > + rc = file_has_perm(cred, vma->vm_file, FILE__EXECMOD); > + else > + rc = avc_has_perm(&selinux_state, > + sid, sid, SECCLASS_PROCESS, > + PROCESS__EXECMEM, NULL);
Same issue wrt auditing here. Could also potentially skip the EXECMEM check this time if we performed it above (if so, then we must have passed it because *allowed_prot still had PROT_EXEC set).
> + /* > + * Clear ALLOW_EXEC instead of ALLOWED_WRITE if permissions are > + * lacking and @prot has neither PROT_WRITE or PROT_EXEC. If > + * userspace wanted RX they would have requested RX, and due to > + * lack of permissions they can never get RW->RX, i.e. the only > + * useful transition is R->RW. > + */ > + if (rc) { > + if ((prot & PROT_EXEC) && (prot & PROT_WRITE)) > + return rc; > + > + if (prot & PROT_EXEC) > + *allowed_prot &= ~PROT_WRITE; > + else > + *allowed_prot &= ~PROT_EXEC; > + } > + } > + > + return 0; > +} > +#endif > + > struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { > .lbs_cred = sizeof(struct task_security_struct), > .lbs_file = sizeof(struct file_security_struct), > @@ -6968,6 +7049,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free), > LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free), > #endif > + > +#ifdef CONFIG_INTEL_SGX > + LSM_HOOK_INIT(enclave_load, selinux_enclave_load), > +#endif > }; > > static __init int selinux_init(void) >
| |