Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Stephen Smalley]

On 6/3/19 10:42 AM, Sean Christopherson wrote:
> On Mon, Jun 03, 2019 at 10:19:18AM -0400, Stephen Smalley wrote:
> > On 5/31/19 7:31 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, 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.  The notable
> > > difference from file_mprotect() is the allowed_prot parameter, which
> > > is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
> > > flags.  The purpose of allowed_prot is to enable checks such as
> > > SELinux's FILE__EXECMOD permission without having to track and update
> > > VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
> > > overstep its bounds simply by restricting an enclave VMA's protections
> > > by vetting what is maximally allowed during build time.
> > >
> > > An alternative to the allowed_prot approach would be to use an enclave's
> > > SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
> > > as a proxy for the enclave.  For example, SGX could take and hold a
> > > reference to the file containing the SIGSTRUCT (if it's in a file) and
> > > call security_enclave_load() during mprotect().  While the SIGSTRUCT
> > > approach would provide better precision, the actual value added was
> > > deemed to be negligible.  On the other hand, pinning a file for the
> > > lifetime of the enclave is ugly, and essentially caching LSM policies
> > > in each page's allowed_prot avoids having to make an extra LSM upcall
> > > during mprotect().
> > >
> > > 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@xxxxxxxxxxxxxx
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > ---
> > >   arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 +++++++++-----
> > >   include/linux/lsm_hooks.h              | 16 ++++++++++++++++
> > >   include/linux/security.h               |  2 ++
> > >   security/security.c                    |  8 ++++++++
> > >   4 files changed, 35 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > > index 5f71be7cbb01..260417ecbcff 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>
> > > @@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
> > >   				 unsigned long *allowed_prot)
> > >   {
> > >   	struct vm_area_struct *vma;
> > > +	int ret = 0;
> > > -	if (!(*allowed_prot & VM_EXEC))
> > > +	if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
> > >   		goto do_check;
> > >   	down_read(&current->mm->mmap_sem);
> > >   	vma = find_vma(current->mm, src);
> > >   	if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
> > >   		*allowed_prot &= ~VM_EXEC;
> > > +#ifdef CONFIG_SECURITY
> > > +	ret = security_enclave_load(vma, prot, allowed_prot);
> > > +#endif
> >
> > Normally you'd define a static inline stub for the hook in the #else clause
> > for CONFIG_SECURITY in include/linux/security.h and avoid any ifdef here.
>
> Ah, right.
>   
> > What ensures that the mapping referenced by src can't be changed to an
> > entirely different one (with a different vm_file) between the time of check
> > (here) and the time of use?
>
> Nothing.  Holding mmap_sem across copy_from_user() would suffice, correct?

I don't believe you can do that; copy_from_user() could stall 
indefinitely.  Not sure how to do what you want here or if it requires 
changing the interface.

> > >   	up_read(&current->mm->mmap_sem);
> > >   do_check:
> > > -	if (prot & ~*allowed_prot)
> > > -		return -EACCES;
> > > -
> > > -	return 0;
> > > +	if (!ret && (prot & ~*allowed_prot))
> > > +		ret = -EACCES;
> > > +	return ret;
> > >   }
> > >   static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,