On Fri, May 24, 2019 at 11:34 AM Xing, Cedric [off-list ref] wrote:

quoted

From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
owner@vger.kernel.org] On Behalf Of Sean Christopherson
Sent: Friday, May 24, 2019 10:55 AM

On Fri, May 24, 2019 at 10:42:43AM -0700, Sean Christopherson wrote:

quoted

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

quoted

non-enclave DSOs while the enclave is being built, i.e. do we really

need

quoted

to force userspace to fully map the enclave in normal memory?

Consider the Graphene scenario where it's building an enclave on the

fly.

quoted

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

quoted

(or EXECMEM if Graphene isn't careful).  In a non-SGX scenario this

makes

quoted

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

quoted

at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it

*is*

quoted

possible to verify the resulting RX contents.  E.g. for the purposes

of

quoted

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.

quoted

  - Prevent userspace from mapping the enclave with permissions beyond

the

quoted

    original permissions of the enclave.  This can be done by

populating

quoted

    VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as

Andy's

quoted

    proposals).  E.g. pre-EINIT, mmap() and mprotect() can only

succeed

quoted

    with PROT_NONE.

  - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for

simplicity,

quoted

    or provide an alternate SGX_IOC_MPROTECT if we want to sidestep

the

quoted

    FILE__WRITE requirement.

One more thought.  EADD (and the equivalent SGX2 flow) could do
security_mmap_file() with a NULL file on the SECINFO permissions, which
would trigger PROCESS_EXECMEM if an enclave attempts to map a page RWX.

If "initial permissions" for enclaves are less restrictive than shared objects, then it'd become a backdoor for circumventing LSM when enclave whitelisting is *not* in place. For example, an adversary may load a page, which would otherwise never be executable, as an executable page in EPC.

In the case a RWX page is needed, the calling process has to have a RWX page serving as the source for EADD so PROCESS__EXECMEM will have been checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on /dev/sgx/enclave, which I see as a security benefit because it only affects the enclave but not the whole process hosting it.

So the permission would be like FILE__EXECMOD on the source enclave
page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
MAP_SHARED, PROT_WRITE isn't going to work because that means you can
modify the file.

I'm starting to think that looking at the source VMA permission bits
or source PTE permission bits is putting a bit too much policy into
the driver as opposed to the LSM.  How about delegating the whole
thing to an LSM hook?  The EADD operation would invoke a new hook,
something like:

int security_enclave_load_bytes(void *source_addr, struct
vm_area_struct *source_vma, loff_t source_offset, unsigned int
maxperm);

Then you don't have to muck with mapping anything PROT_EXEC.  Instead
you load from a mapping of a file and the LSM applies whatever policy
it feels appropriate.  If the first pass gets something wrong, the
application or library authors can take it up with the SELinux folks
without breaking the whole ABI :)

(I'm proposing passing in the source_vma because this hook would be
called with mmap_sem held for read to avoid a TOCTOU race.)

If we go this route, the only substantial change to the existing
driver that's needed for an initial upstream merge is the maxperm
mechanism and whatever hopefully minimal API changes are needed to
allow users to conveniently set up the mappings.  And we don't need to
worry about how to hack around mprotect() calling into the LSM,
because the LSM will actually be aware of SGX and can just do the
right thing.

--Andy