Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Fri, May 17, 2019 at 04:09:22PM -0400, Stephen Smalley wrote:
> On 5/17/19 3:28 PM, Sean Christopherson wrote:
> >On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
> >Yep, and that's by design in the overall proposal.  The trick is that
> >ENCLAVE_ADD takes a source VMA and copies the contents *and* the
> >permissions from the source VMA.  The source VMA points at regular memory
> >that was mapped and populated using existing mechanisms for loading DSOs.
> >
> >E.g. at a high level:
> >
> >source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
> >for_each_chunk {
> >         <hand waving - mmap()/mprotect() the enclave file into regular memory>
> >}
> >
> >enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
> >enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
> >
> >ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
> >for_each_chunk {
> >         struct sgx_enclave_add ioctlargs = {
> >                 .offset = chunk.offset,
> >                 .source = chunk.addr,
> >                 .size   = chunk.size,
> >                 .type   = chunk.type, /* SGX specific metadata */
> >         }
> >         ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
> >}
> >ioctl(fd, ENCLAVE_INIT, ...);
> >
> >
> >Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
> >ensures userspace isn't bypassing LSM policies by virtue of copying the
> >permissions for EPC VMAs from regular VMAs that have already gone through
> >LSM checks.
> 
> Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice?  Do you
> do anything other than ioctl() calls on it?

Hmm, in the current implementation, yes, O_RDWR is required.  An enclave
and its associated EPC memory are represented and referenced by its fd,
which is backed by /dev/sgx/enclave.  An enclave is not just code, e.g.
also has a heap, stack, variables, etc..., which need to be mapped
accordingly.  In the current implementation, userspace directly does
mprotect() or mmap() on EPC VMAs, and so setting PROT_WRITE for the heap
and whatnot requires opening /dev/sgx/enclave with O_RDWR.

I *think* /dev/sgx/enclave could be opened O_RDONLY if ENCLAVE_ADD stuffed
the EPC VMA permissions, assuming the use case doesn't require changing
permissions after the enclave has been created.

The other reason userspace would need to open /dev/sgx/enclave O_RDWR
would be to debug an enclave, e.g. pwrite() works on the enclave fd due
to SGX restrictions on modifying EPC memory from outside the enclave.
But that's an obvious case where FILE__WRITE should be required.

> What's the advantage of allocating an anon inode in the above?  At present
> anon inodes are exempted from inode-based checking, thereby losing the
> ability to perform SELinux ioctl whitelisting, unlike the file-backed
> /dev/sgx/enclave inode.

Purely to trigger the EXECMEM check on any PROT_EXEC mapping.  However,
the motiviation for that was due to my bad assumption that FILE__WRITE
and FILE__EXECUTE are global and not per process.  If we can do as you
suggest and allow creation of enclaves with O_RDONLY, then keeping a
file-backed inode is definitely better as it means most processes only
need FILE__READ and FILE__* in general has actual meaning.

Thanks a bunch for your help!