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

[Stephen Smalley]

On 5/17/19 3:28 PM, Sean Christopherson wrote:
> On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
> > On 5/17/19 1:12 PM, Andy Lutomirski wrote:
> > > How can that work?  Unless the API changes fairly radically, users
> > > fundamentally need to both write and execute the enclave.  Some of it will
> > > be written only from already executable pages, and some privilege should be
> > > needed to execute any enclave page that was not loaded like this.
> >
> > I'm not sure what the API is. Let's say they do something like this:
> >
> > fd = open("/dev/sgx/enclave", O_RDONLY);
> > addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
> > stuff addr into ioctl args
> > ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
> > ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
> > ioctl(fd, ENCLAVE_INIT, &ioctlargs);
>
> That's rougly the flow, except that that all enclaves need to have RW and
> X EPC pages.
>
> > The important points are that they do not open /dev/sgx/enclave with write
> > access (otherwise they will trigger FILE__WRITE at open time, and later
> > encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
> > allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
> > resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
> > Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
> > policy.
> >
> > If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
> > will trigger an EXECMEM check, at least as currently implemented, as we have
> > no useful backing inode information.
>
> 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?

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.

How would SELinux (or other security modules) restrict the authorized 
enclaves that can be loaded via this interface?  Would the sgx driver 
invoke a new LSM hook with the regular/source VMAs as parameters and 
allow the security module to reject the ENCLAVE_ADD operation?  That 
could be just based on the vm_file (e.g. whitelist what enclave files 
are permitted in general) or it could be based on both the process and 
the vm_file (e.g. only allow specific enclaves to be loaded into 
specific processes).