Adding dynamic memory management features (EDMM) with SGX2 supports

[rainfld]

This PR added three major new features which are supported by SGX2 EDMM instructions, including:

• Dynamic Heap Management. Graphene-SGX allocates EPC pages for the heap when the heap grows with the application is executed.
• Dynamic Stack Management. Same as heap, Graphene-SGX dynamically grows the stacks with EPC pages at runtime
• Dynamic Thread Management. Allocating necessary EPC memory for creating new threads at runtime

---

This change is 

[oscar-bot]

Can one of the admins verify this patch?

[oscar-bot]

Can one of the admins verify this patch?

[oscar-bot]

Can one of the admins verify this patch?

[rainfld]

Test this please

[chiache]

Do we need a SGX2 machine to test this patch?

[rainfld]

Yes. Currently SGX2 is available on Gemini-Lake based NUCs, Such as NUC7CJYH.

[donporter]

I'll get a NUC added to our CI setup as soon as possible to test SGX v2 support.

[donporter]

Reviewed 13 of 19 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @rainfld and @chiache)

---

Pal/src/host/Linux-SGX/ecall_types.h, line 7 at r2 (raw file):

    ECALL_ENCLAVE_START = 0,
    ECALL_THREAD_START,
    ECALL_STACK_EXPAND,

I am not sure I understand the rationale for these being ECALLs. My first thought would be that these are internal-only functions.

---

Pal/src/host/Linux-SGX/enclave_ecalls.c, line 20 at r2 (raw file):

extern void * enclave_base, * enclave_top;

struct thread_map {

I think it would be good to have a comment explaining what a thread map is, and its role within the SGX PAL.

Some of these fields are also not obvious to me: thread_index, aux_stack_addr, enclave_entry, and could benefit from a bit more explanation.

---

Pal/src/host/Linux-SGX/enclave_ecalls.c, line 36 at r2 (raw file):

pal_expand_stack

One thing that seems important here is ensuring an upper bound on the stack, and that this region doesn't grow into any other allocated regions.

It isn't obvious from the code that this will not happen, although there may be some SGX restrictions on replacing a page?

---

Pal/src/host/Linux-SGX/enclave_ecalls.c, line 70 at r2 (raw file):

 * ecall_args: pointer to the thread-dependent information for setup the new thread
 */
void pal_thread_setup(void * ecall_args){

I'm not sold this should be an ecall, but, if it is, we should probably put a bit more effort into sanity checking the inputs to an ecall. Like every input should probably be copied and checked that the result makes sense, rather than accept, say the address for the ssa on faith from the URTS.

This is beyond the scope of this particular PR, but I wonder if we don't want some annotation on untrusted pointers, so they cannot be used directly, similar to taint tracking.

---

Pal/src/host/Linux-SGX/enclave_ecalls.c, line 81 at r2 (raw file):

    // Setup TLS
    struct enclave_tls* tls = (struct enclave_tls *) thread_info->tls_addr;

So here, thread_info is untrusted, right? Should we be accepting a tls address from the URTS?

---

Pal/src/host/Linux-SGX/enclave_ecalls.c, line 82 at r2 (raw file):

    // Setup TLS
    struct enclave_tls* tls = (struct enclave_tls *) thread_info->tls_addr;
    tls->enclave_size = GET_ENCLAVE_TLS(enclave_size);

Presumably, this is using the TLS of another thread to copy in values?

---

Pal/src/host/Linux-SGX/enclave_ecalls.c, line 145 at r2 (raw file):

    SET_ENCLAVE_TLS(ustack_top,  untrusted_stack);
    SET_ENCLAVE_TLS(ustack,      untrusted_stack);
    SET_ENCLAVE_TLS(ocall_pending, 0);

Can you explain this line and the role of ocall_pending in handling an ecall? Perhaps this should be an assert?

Update: This becomes clearer in the assembly below. The symbol makes sense.

---

Pal/src/host/Linux-SGX/enclave_entry.S, line 430 at r2 (raw file):

 * sgx_modpe:
 * EMODPE pages for dynamic memory management
 */

Can you expand this a bit more with the expected arguments and behavior (or put this in a header)? I was able to find the MODPE instruction by googling, but it might be worth spelling out what is going to happen if you call this function.

---

Pal/src/host/Linux-SGX/enclave_framework.c, line 117 at r2 (raw file):

    smi.flags |= SGX_SECINFO_FLAGS_X;

    while (lo < addr)

One small thing that bugs me (and this is more of a maintainer-level conversation), is should we be enforcing a policy on when we do and don't accept pages? Or perhaps this should be enforced by the caller of this function?

---

Pal/src/host/Linux-SGX/enclave_pages.c, line 73 at r2 (raw file):

}

void allocate_page_range(void * addr, uint64_t size)

It looks like this only gets called after the check for the heap region, etc. It would be good to explicate that we assume the caller should have already checked that this is a desired change to the address space.

---

Pal/src/host/Linux-SGX/sgx_api.h, line 44 at r2 (raw file):

int sgx_verify_report (sgx_arch_report_t * report);

int sgx_accept(sgx_arch_secinfo_t* si, const void * addr);

This could also be in the code (vs header), but it might be good to summarize the relationship among accept, modpe, and accept_pages.

---

Pal/src/host/Linux-SGX/sgx_enclave.c, line 723 at r2 (raw file):

}

int ecall_stack_expand(void * addr)

Here (or in the header), these functions need a high-level comment about what they do (assuming we keep the ecall part)

---

Pal/src/host/Linux-SGX/sgx_entry.S, line 14 at r2 (raw file):

        # Save registers
	push %rbx

Just curious, any sense why we got away with not saving/restoring these registers previously?

---

Pal/src/host/Linux-SGX/sgx_framework.c, line 313 at r2 (raw file):

    ret = INLINE_SYSCALL(ioctl, 3, isgx_device, SGX_IOC_ENCLAVE_MKTCS, &params);
    if (IS_ERR(ret)) {
	SGX_DBG(DBG_I, "Enclave MKTCS returned %d\n", ret);

Spacing looks off? Maybe just reviewable?

---

Pal/src/host/Linux-SGX/sgx_main.c, line 300 at r2 (raw file):

    if (enclave->pal_sec.edmm_mode)
        enclave->size = parse_int("4G");

Should this be hard coded? Or maybe a ! in the if?

---

Pal/src/host/Linux-SGX/sgx_thread.c, line 24 at r2 (raw file):

struct thread_map {

This is also defined in another .c file. Hoist into a .h?

---

Pal/src/host/Linux-SGX/signer/pal-sgx-sign, line 181 at r2 (raw file):

                    attributes.pop(manifest_options[opt])

    if 'sgx.disable_avx' in manifest:

I'm guessing this is left over from another pending PR?

[donporter]

Ok to test

[donporter]

BTW, the other thing that is missing here are some unit tests for this functionality.

[dimakuv]

The EDMM feature will be developed by @vijaydhanraj.

[johnvenn]

Per my understanding, this feature requires driver change to support those Intel instructions to expand or trim EPC pages, but now the in-kernel(DCAP) driver doesn't support EDMM. Do you mean this feature can only work with legacy OOT driver?

[vijaydhanraj]

For our initial development, we are evaluating with the legacy driver.

[mkow]

Per my understanding, this feature requires driver change to support those Intel instructions to expand or trim EPC pages, but now the in-kernel(DCAP) driver doesn't support EDMM. Do you mean this feature can only work with legacy OOT driver?

Yes. The upstreamed in-tree driver also doesn't support it, but AFAIK there are plans to add this feature to it.

[duanbing]

I think that's not SDK, it's SGX driver? https://github.com/intel/linux-sgx-driver/blob/master/sgx_ioctl.c#L383.

[dimakuv]

@duanbing Yes, you are correct. But this PR is 3 years old and not planned for merging. We keep this PR open simply to not forget about a new version of the EDMM support :)

[mkow]

@vijaydhanraj is reviving these efforts, if you're interested then please take a look at #2190.

[mkow]

Superseded by #2190, closing.