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

[Sean Christopherson]

On Fri, May 24, 2019 at 11:41:29AM -0400, Stephen Smalley wrote:
> On 5/24/19 3:24 AM, Xing, Cedric wrote:
> >/**
> >  * Summary:
> >  * - The enclave file resembles a shared object that contains RO/RX/RW segments
> >  * - FILE__* are assigned to /dev/sgx/enclave, to determine acceptable permissions to mmap()/mprotect(), valid combinations are
> >  *   + FILE__READ - Allow SGX1 enclaves only
> >  *   + FILE__READ|FILE__WRITE - Allow SGX2 enclaves to expand data segments (e.g. heaps, stacks, etc.)
> >  *   + FILE__READ|FILE__WRITE|FILE__EXECUTE - Allow SGX2 enclaves to expend both data and code segments. This is necessary to support dynamically linked enclaves (e.g. Graphene)
> >  *   + FILE__READ|FILE__EXECUTE - Allow RW->RX changes for SGX1 enclaves - necessary to support dynamically linked enclaves (e.g. Graphene) on SGX1. EXECMEM is also required for this to work
> 
> I think EXECMOD would fit better than EXECMEM for this case; the former is
> applied for RW->RX changes for private file mappings while the latter is
> applied for WX private file mappings.
> 
> >  *   + <None> - Disallow the calling process to launch any enclaves
> >  */
> >
> >/* Step 1: mmap() the enclave file according to the segment attributes (similar to what dlopen() would do for regular shared objects) */
> >int image_fd = open("/path/to/enclave/file", O_RDONLY);
> 
> FILE__READ checked to enclave file upon open().
> 
> >foreach phdr in loadable segments /* phdr->p_type == PT_LOAD */ {
> >     /* <segment permission> below is subject to LSM checks */
> >     loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE, <segment permission>, image_fd, phdr->p_offset);
> 
> FILE__READ revalidated and FILE__EXECUTE checked to enclave file upon mmap()
> for PROT_READ and PROT_EXEC respectively.  FILE__WRITE not checked even for
> PROT_WRITE mappings since it is a private file mapping and writes do not
> reach the file.  EXECMEM checked if any segment permission has both W and X
> simultaneously.  EXECMOD checked on any subsequent mprotect() RW->RX changes
> (if modified).

Hmm, I've been thinking more about pulling permissions from the source
page.  Conceptually I'm not sure we need to meet the same requirements as
non-enclave DSOs while the enclave is being built, i.e. do we really need
to force userspace to fully map the enclave in normal memory?
 
Consider the Graphene scenario where it's building an enclave on the fly.
Pulling permissions from the source VMAs means Graphene has to map the
code pages of the enclave with X.  This means Graphene will need EXEDMOD
(or EXECMEM if Graphene isn't careful).  In a non-SGX scenario this makes
perfect sense since there is no way to verify the end result of RW->RX.

But for SGX, assuming enclaves are whitelisted by their sigstruct (checked
at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it *is*
possible to verify the resulting RX contents.  E.g. for the purposes of
LSMs, can't we use the .sigstruct file as a proxy for the enclave and
require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?

Stephen, is my logic sound?


If so...

  - Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the enclave.

  - Prevent userspace from mapping the enclave with permissions beyond the
    original permissions of the enclave.  This can be done by populating
    VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as Andy's
    proposals).  E.g. pre-EINIT, mmap() and mprotect() can only succeed
    with PROT_NONE.

  - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for simplicity,
    or provide an alternate SGX_IOC_MPROTECT if we want to sidestep the
    FILE__WRITE requirement.

No changes are required to LSMs, SGX1 has a single LSM touchpoint in its
mmap(), and I *think* the only required userspace change is to mmap()
PROT_NONE when allocating the enclave's virtual address range.

As for Graphene, it doesn't need extra permissions to run its enclaves,
it just needs a way to install .sigstruct, which is a generic permissions
problem and not SGX specific.


For SGX2 maybe:

  - No additional requirements to map an EAUG'd page as RW page.  Not
    aligned with standard MAP_SHARED behavior, but we really don't want
    to require FILE__WRITE, and thus allow writes to .sigstruct. 
 
  - Require FILE__EXECMOD on the .sigstruct to map previously writable
    page as executable (which indirectly includes all EAUG'd pages).
    Wiring this up will be a little funky, but we again we don't want
    to require FILE__WRITE on .sigstruct.