RE: [PATCH v19,RESEND 24/27] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Xing, Cedric]

> Using the untrusted stack as a way to exchange data is very convenient,
> but that doesn't mean it's a good idea.  Here are some problems it
> causes:
> 
>  - It prevents using a normal function to wrap enclave entry (as we're
> seeing with this patch set).

It doesn't prevent. It's all about what's agreed between the enclave and its hosting process. With the optional "exit/exception callback" set to null, this will behave exactly the same as in the current patch. That's what I meant by "flexibility" and "superset of functionality".

> 
>  - It makes quite a few unfortunate assumptions about the layout of the
> untrusted stack.  It assumes that the untrusted stack is arbitrarily
> expandable, which is entirely untrue in languages like Go.

I'm with you that stack is not always good thing, hence I'm NOT ruling out any other approaches for exchanging data. But is stack "bad" enough to be ruled out completely? The point here is flexibility because the stack could be "good" for its convenience. After all, only buffers of "reasonable" sizes will be exchanged in most cases, and in the rare exceptions of stack overflow they'd probably get caught in validation anyway. The point here again is - flexibility. I'd say it's better to leave the choice to the SDK implementers than to force the choice on them.

> It assumes that the untrusted stack isn't further constrained by various
> CFI mechanisms (e.g. CET), and, as of last time I checked, the
> interaction between CET and SGX was still not specified.  

I was one of the architects participating in the CET ISA definition. The assembly provided was crafted with CET in mind and will be fully compatible with CET.

> It also
> assumes that the untrusted stack doesn't have ABI-imposed layout
> restrictions related to unwinding, and, as far as I know, this means
> that current enclaves with current enclave runtimes can interact quite
> poorly with debuggers, exception handling, and various crash dumping
> technologies.

Per comments from the patch set, I guess it's been agreed that this vDSO function will NOT be x86_64 ABI compatible. So I'm not sure why stacking unwinding is relevant here. However, I'm with you that we should take debugging/exception handling/reporting/crash dumping into consideration by making this vDSO API x86_64 ABI compatible. IMO it's trivial and the performance overhead in negligible (dwarfed by ENCLU anyway. I'd be more than happy to provide a x86_64 ABI compatible version if that's also your preference.

>  - It will make it quite unpleasant to call into an enclave in a
> coroutine depending on how the host untrusted runtime implements
> coroutines.

I'm not sure what you are referring to by "coroutine". But this vDSO API will be (expected to be) the only routine that actually calls into an enclave. Isn't that correct?

> 
> So I think it's a *good* thing if the effect is to make enclave SDKs
> change their memory management so that untrusted buffers are explicitly
> supplied by the host runtime.  

Intel SGX SDK will change no matter what. The point is flexibility, is to offer choices and let SDK implementers decide, instead of deciding for them ahead of time. 

> Honestly, I would have much preferred if
> the architecture did not give the enclave access to RSP and RBP at all.
> (And, for that matter, RIP.)

This reminds me of PUSHA/POPA instructions. We once thought those instruction would be appreciated by compilers but the fact turns out that most compilers prefer a mix of caller-saved/callee-saved GPRs instead of treating all GPRs caller or callee saved. Then when we believed everyone would prefer a mix after so many years, an exception emerged as GO was invented. That said, flexibility is the point and is the most important thing ISA is always trying to offer. The rest is just software convention. So we decided not to enforce RBP/RSP, unless there are security implications - e.g. RIP - EEXIT will be considered an indirect branch instruction and will have to land on an ENDBR once CET comes out.