Re: [PATCH v20 00/28] Intel SGX1 support

[Sean Christopherson]

On Tue, May 14, 2019 at 02:27:08PM -0700, Andy Lutomirski wrote:
> On Tue, May 14, 2019 at 1:45 PM Sean Christopherson
> <sean.j.christopherson@xxxxxxxxx> wrote:
> >
> > On Tue, May 14, 2019 at 08:13:36AM -0700, Andy Lutomirski wrote:
> > > On Tue, May 14, 2019 at 3:43 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@xxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, May 13, 2019 at 01:29:26PM +0300, Jarkko Sakkinen wrote:
> > > > > I did study through SDK's file format and realized that it does not
> > > > > does make sense after all to embed one.
> > > > >
> > > > > To implement it properly you would probably need a new syscall (lets say
> > > > > sgx_load_enclave) and also that enclaves are not just executables
> > > > > binaries. It is hard to find a generic format for them as applications
> > > > > range from simply protecting part of an application to running a
> > > > > containter inside enclave.
> > > >
> > > > I'm still puzzling what kind of changes you were discussing considering
> > > > SGX_IOC_ENCLAVE_ADD_PAGE.
> > >
> > > I think it's as simple as requiring that, if SECINFO.X is set, then
> > > the src pointer points to the appropriate number of bytes of
> > > executable memory.  (Unless there's some way for an enclave to change
> > > SECINFO after the fact -- is there?)
> >
> > Nit: SECINFO is just the struct passed to EADD, I think what you're really
> > asking is "can the EPCM permissions be changed after the fact".
> >
> > And the answer is, yes.
> >
> > On SGX2 hardware, the enclave can extend the EPCM permissions at runtime
> > via ENCLU[EMODPE], e.g. to make a page writable.
> >
> > Hardware also doesn't prevent doing EADD to the same virtual address
> > multiple times, e.g. an enclave could EADD a RX page, and then EADD a
> > RW page at the same virtual address with different data.  The second EADD
> > will affect MRENCLAVE, but so long as it's accounted for by the enclave's
> > signer, it's "legal".  SGX_IOC_ENCLAVE_ADD_PAGE *does* prevent adding the
> > "same" page to an enclave multiple times, so effectively this scenario is
> > blocked by the current implementation, but it's more of a side effect (of
> > a sane implementation) as opposed to deliberately preventing shenanigans.
> >
> > Regarding EMODPE, the kernel doesn't rely on EPCM permissions in any way
> > shape or form (the EPCM permissions are purely to protect the enclave
> > from the kernel), e.g. adding +X to a page in the EPCM doesn't magically
> > change the kernel's page tables and attempting to execute from the page
> > will still generate a (non-SGX) #PF.
> >
> > So rather than check SECINFO.X, I think we'd want to have EADD check that
> > the permissions in SECINFO are a subset of the VMA's perms (IIUC, this is
> > essentially what Cedric proposed).  That would prevent using EMODPE to
> > gain executable permissions, and would explicitly deny the scenario of a
> > double EADD to load non-executable data into an executable page.
> 
> Let me make sure I'm understanding this correctly: when an enclave
> tries to execute code, it only works if *both* the EPCM and the page
> tables grant the access, right?  This seems to be that 37.3 is trying
> to say.  So we should probably just ignore SECINFO for these purposes.

Yep.  More specifically, the EPCM is consulted if and only if the access
is allowed by the page tables.

I agree on ignoring SECINFO.

> But thinking this all through, it's a bit more complicated than any of
> this.  Looking at the SELinux code for inspiration, there are quite a
> few paths, but they boil down to two cases: EXECUTE is the right to
> map an unmodified file executably, and EXECMOD/EXECMEM (the
> distinction seems mostly irrelevant) is the right to create (via mmap
> or mprotect) a modified anonymous file mapping or a non-file-backed
> mapping that is executable.  So, if we do nothing, then mapping an
> enclave with execute permission will require either EXECUTE on the
> enclave inode or EXECMOD/EXECMEM, depending on exactly how this gets
> set up.

If we do literally nothing, then I'm pretty sure mapping an enclave will
require PROCESS__EXECMEM.  The mmap() for the actual enclave is done
using an anon inode, e.g. from /dev/sgx/enclave.  Anon inodes are marked
private, which means inode_has_perm() will always return "success".  The
only effective check is in file_map_prot_check() when default_noexec is
true, in which case requesting PROT_EXEC on private inodes requires
PROCESS__EXECMEM.

> So all is well, sort of.  The problem is that I expect there will be
> people who want enclaves to work in a process that does not have these
> rights.  To make this work, we probably need do some surgery on
> SELinux.  ISTM the act of copying (via the EADD ioctl) data from a
> PROT_EXEC mapping to an enclave should not be construed as "modifying"
> the enclave for SELinux purposes.  Actually doing this could be
> awkward, since the same inode will have executable parts and
> non-executable parts, and SELinux can't really tell the difference.

Rather the do surgery on SELinux, why not go with Cedric's original
proposal and propagate the permissions from the source VMA to the EPC
VMA?  The enclave mmap() from userspace could then be done with RO
permissions so as to not run afoul of LSMs.  Adding PROT_EXEC after
EADD would require PROCESS__EXECMEM, but that's in line with mprotect()
on regular memory.  It also punts the EMODPE hiccup to userspace, e.g.
any enclave that wants to do fancy things with PROT_EXEC needs
PROCESS__EXECMEM.

The only downside I see is that SGX would be doing a bit of magic with
enclave VMAs.

> Maybe the enclave should track a bitmap of which pages have ever been
> either mapped for write or EADDed with a *source* that wasn't
> PROT_EXEC.  And then SELinux could learn to allow those pages (and
> only those pages) to be mapped executably without EXECUTE or EXECMOD
> or whatever permission.
> 
> Does this seem at all reasonable?

No?  I don't understand why you want to special case enclaves from an
LSM perspective.  Enforcing LSM policies on the source provides the
same security for enclaves as it does for normal code, no more, no less.

> I suppose it's not the end of the world if the initially merged
> version doesn't do this, as long as there's some reasonable path to
> adding a mechanism like this when there's demand for it.