Date:   Wed, 20 Mar 2019 14:03:13 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     "Xing, Cedric" <cedric.xing@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "npmccallum@...hat.com" <npmccallum@...hat.com>,
        "Ayoun, Serge" <serge.ayoun@...el.com>,
        "Katz-zamir, Shay" <shay.katz-zamir@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Svahn, Kai" <kai.svahn@...el.com>, "bp@...en8.de" <bp@...en8.de>,
        "josh@...htriplett.org" <josh@...htriplett.org>,
        "Huang, Kai" <kai.huang@...el.com>,
        "rientjes@...gle.com" <rientjes@...gle.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Haitao Huang <haitao.huang@...ux.intel.com>,
        Jethro Beekman <jethro@...tanix.com>,
        "Dr . Greg Wettstein" <greg@...ellic.com>
Subject: Re: [PATCH v19,RESEND 24/27] x86/vdso: Add
 __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

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...

> >  - 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.

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.

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.

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.

> > 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.

> 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.

Powered by blists - more mailing lists