Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Jethro Beekman]

On 2020-09-25 18:55, Andy Lutomirski wrote:
> vdso_sgx_enter_enclave() sucks. I don't think anyone seriously likes
> it, but maybe it's the best we can do.

I don't agree that it sucks. I think it's pretty good. My goal is to 1) support existing enclaves and 2) have it as close to ENCLU as possible. Goal 2 since I want to use it in cross-platform code and other platforms don't have this kind of call, so I'm forced to use assembly anyway.

The only concerns I have are that I need 256 bytes of memory (but I can put that on the stack I suppose) and the automatic restarting, but I've agreed to postpone the latter issue post-merge.

Now I'm speaking from a perspective of someone who's not planning to use the callback/user stack, so perhaps people using those features might think the current implementation is not that great.

> 
> I'm wondering if it's worth trying to do better.  Here's what I'd like
> if I could wave a magic wand:
> 
> struct sgx_enclave_run {
>        __u64 tcs;
>        __u32 flags;
>        __u32 exit_reason;
> 
>     /*
>      * These values are exposed to the enclave on entry, and the values
>      * left behind by the enclave are returned here.
>      * Some enclaves might write to memory pointed to by rsp.
>      */
>        __u64 rsp, rbp, r8, r9, r10, r11, r12, r13, r14, r15;
>        /* Maybe other regs too? */

I think it's fine to add a mechanism to pass rsp & rbp, and I support innovation in this space. But we should aim to keep the other register passing as close as possible to the instruction/what the current implementation does. For the other state, it certainly encompasses more than just the 16 GPRs. There's also rflags, XMM, etc. Future processors might add even more state. Capturing all state in this structure seems unnecessary.

On 2020-09-25 21:09, Sean Christopherson wrote:
> For the code, given the constraints of SGX and the number of runtimes we're
> enabling, I don't think it's bad at all.  It's not hard to maintain, there are
> no horrendous hacks, and it doesn't play games with the caller's state, i.e.
> there's no additional magic required.  In other words, I really like that we
> have in hand _works_, and works for a variety of runtimes and their ABIs.
> 
> The API isn't glorious, but it's not awful either.

Yup.

On 2020-09-25 22:20, Andy Lutomirski wrote:
> On the other hand, if we had some confidence that the existing corpus
> of enclaves plays games with RSP but not RBP

Pretty sure this is the case.

> languages that aren't quite as C-like as C.  In a language with
> stackless coroutines or async/await or continuations or goroutines,
> this could all get quite awkward.  Sure, a really nice Rust or Go SGX
> untrusted runtime could just declare that it won't support enclaves
> that touch the stack, but that's a bit of an unfortunate restriction
> given that removing stack access from an existing enclave will
> inevitably change MRENCLAVE.

I can say with confidence that v38 proposal can be used by async Rust code if the enclave doesn't use the user stack.

> If everyone wants to tell me that what we have now (plus .cfi
> annotations and perhaps a CET fix) is good enough, then so be it.

Good enough for me.

On 2020-09-26 00:29, Sean Christopherson wrote:
> We do have this confidence, because it's required by the current version
> of the VDSO.

Do you mean “it's required by the current version of the VDSO” and nobody has complained about it?

> But the callback is optional...

Yup.

--
Jethro Beekman | Fortanix

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature