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

[Andy Lutomirski]

On Thu, May 16, 2019 at 3:23 PM Xing, Cedric <cedric.xing@xxxxxxxxx> wrote:
>
> Hi Andy,
>
> > > SIGSTRUCT isn't necessarily stored on disk so may not always have a fd.
> > How about the following?
> > > void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
> > > ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
> > >
> > > The idea here is SIGSTRUCT will still be passed in memory so it works
> > the same way when no LSM modules are loaded or basing its decision on
> > the .sigstruct file. Otherwise, an LSM module can figure out the backing
> > file (and offset within that file) by looking into the VMA covering
> > ss_pointer.
> >
> > I donât love this approach.  Application authors seem likely to use
> > read() instead of mmap(), and itâll still work in many cares. It would
> > also complicate the kernel implementation, and looking at the inode
> > backing the vma that backs a pointer is at least rather unusual.
> > Instead, if the sigstruct isnât on disk because itâs dynamic or came
> > from a network, the application can put it in a memfd.
>
> I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with.

Hmm.  That's a fair point, although opening /proc/self/exe could be
somewhat of a workaround.  It does suffer from a bit of an in-band
signaling problem, though, in that it's possible that some other
random bytes in the library resemble a SIGSTRUCT.

> I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.

I'm not opposed to this, but I also don't think this needs to be in
the initial upstream driver.  VM_SGX also isn't strictly necessary --
an LSM could inspect the VMA to decide whether it's an SGX VMA if it
really wanted to.

That being said, do you have any specific behavior differences in mind
aside from the oddities involved in loading the enclave.

>
> And if you are with me on that bigger picture, the next question is: what should be the default behavior of security_sgx_mprot() for existing/non-SGX-aware LSM modules/policies? I'd say a reasonable default is to allow R, RW and RX, but not anything else. It'd suffice to get rid of EXECMEM/EXECMOD requirements on enclave applications. For SGX1, EPCM permissions are immutable so it really doesn't matter what security_sgx_mprot() does. For SGX2 and beyond, there's still time and new SGX-aware LSM modules/policies will probably have emerged by then.

I hadn't thought about the SGX1 vs SGX2 difference.  If the driver
initially only wants to support SGX1, then I guess we really could get
away with constraining the EPC flags based on the source page
permission and not restricting mprotect() and mmap() permissions on
/dev/sgx/enclave at all.