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

["Xing, Cedric" <cedric.xing@intel.com>]

> On Wed, Mar 20, 2019 at 12:57:52PM -0700, Xing, Cedric 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.
> 
> Yes it does, keyword being "normal".  Though I guess we could do a bit
> of bikeshedding on the definition of normal...

I don't understand what you mean by "normal". As I said, I tend to have a x86_64 ABI compliant version and by saying that I mean it'd be a 100% "normal" function callable from C. And the version I provided in this thread is a trimmed down version that doesn't preserve any registers except RSP/RBP so a C wrapper will be necessary. Other than that I'm not aware of any anomalies. Could you elaborate on what "abnormal" operations necessary to invoke this vDSO under what circumstances? And it'll be very helpful if you could present a "normal" function to demonstrate how your code could work with it while mine couldn't.

> Actually, this series doesn't force anything on Intel's SDK, as there is
> nothing in the documentation that states the vDSO *must* be used to
> enter enclaves.  In other words, unless it's expressly forbidden,
> applications are free to enter enclaves directly and do as they wish
> with the untrusted stack.  The catch being that any such usage will need
> to deal with enclave exceptions being delivered as signals, i.e. the
> vDSO implementation is a carrot, not a stick.

If you want to bike-shedding on *must*, well, no one *must* use *anything* from the *anyone*! But is that the expectation? Or if you don't expect your API to be used then what are you doing here?

Intel SDK doesn't have to use this API. But we (the SDK team) are truly willing to use this API because we share the same concern with you over signals and would like to move to something better.

> 
> AIUI, excepting libraries that want to manipulate the untrusted stack,
> there is a general consensus that the proposed vDSO implementation is
> the right approach, e.g. full x86_64 ABI compatibility was explored in
> the past and was deemed to add unnecessary complexity and overhead.
> 
> The vDSO *could* be written in such a way that it supports preserving
> RBP or RSP, but for all intents and purposes such an implementation
> would yield two distinct ABIs that just happen to be implemented in a
> single function.
> And *if* we want to deal with maintaining two ABIs, supporting the
> kernel's existing signal ABI is a lot cleaner (from a kernel perspective)
> than polluting the vDSO.

Disagreed! What I'm proposing is one ABI - enclave preserves RBP! No requirements on RSP. Of course RSP is still interpreted as the line between vacant and used parts of the stack, or nothing will work regardless the proposal.

The hosting process may have an agreement with the enclave to preserve RSP. But that would be completely between them, and would be just a coincidence instead of a consequence of the ABI from the perspective of this vDSO API.

> 
> In other words, if there is a desire to support enclaves which modify
> the untrusted stack, and it sounds like there is, then IMO our time is
> better spent discussing whether or not to officially support the signal
> ABI for enclaves.

Disagreed! We share the same concern over signals so let's work this out! 

> 
> > > 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.
> 
> I think Andy's point is that a single PUSH (to save %rcx) won't break
> unwinding, etc..., but unwinders and whantot will have a rough go of it
> if %rsp points at complete garbage.

The unwanders use CFI directives to determine frame pointers. RSP is the frame point by default but could be easily changed - e.g. by ".cfi_def_cfa_register %rbp". I could add proper CFI directives if so desired. I omitted them because you omitted them too in your code (in case you don't know, CFI directives are needed around push/pop %rcx in your code to stay compliant with ELF/DWARF spec), and I didn't want to confuse those who didn't understand CFI.

> 
> > 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's not just the performance cost, making __vdso_sgx_enter_enclave()
> compatible with the x86_64 ABI adds complexity to both its code and its
> documentation, e.g. to describe how data is marshalled to/from enclaves.

Well, technically speaking an ABI compliant API should *reduce* documentation. But given how easy it is to write a C compliant wrapper, I think a custom ABI is perfectly fine.