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

[Stephen Smalley]

On 5/17/19 4:14 PM, Andy Lutomirski wrote:
> > On May 17, 2019, at 1:09 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> >
> > > 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).
>
> This is the idea behind the .sigstruct file. The driver could call a new hook to approve or reject the .sigstruct. The sigstruct contains a hash of the whole enclave and a signature by the author.

Ok, so same idea but moved to ENCLAVE_INIT and passing the vma or file 
for the sigstruct instead of the enclave.