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

[Andy Lutomirski]

On Wed, Mar 20, 2019 at 12:57 PM Xing, Cedric <cedric.xing@xxxxxxxxx> wrote:
>
> > 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?

I mean use in languages and runtimes that allow a function and its
callees to pause and then resume later.  Something like (pseudocode,
obviously):

void invoke_the_enclave()
{
  do_eenter_through_vdso();
}

void some_ocall_handler(void *ptr)
{
  yield;
}

If the enclave has ptr pointing to the untrusted stack, then this gets
quite awkward for the runtime to handle efficiently.  IMO a much nicer
approach would be:

void invoke_the_enclave()
{
  char buffer[1024];
  while (true)
  {
    eenter (through vdso);
    if (exit was an ocall request) {
      some_ocall_handler(buffer);
    }
  }
}

And now there is nothing funny happening behind the runtime's back
when some_ocall_handler tries to yield.