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

[Sean Christopherson]

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.

Oh, and EADD should probably also require EEXTEND for executable pages.

> Sadly, we don't really have the a nice in-kernel API for that right now.
> You could do down_read(mmap_sem) and find_vma().  Arguably there is no
> value to checking that PKRU allows execute to the data.
> 
> Hey, Dave, if you're still paying attention to this thread, should we
> have copy_from_user_exec() that does the right thing wrt the page
> permissions and PKRU.