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

[Sean Christopherson]

On Wed, Mar 20, 2019 at 11:30:26AM -0700, Xing, Cedric wrote:
> > +/**
> > + * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> > + *
> > + * %eax:        ENCLU leaf, must be EENTER or ERESUME
> > + * %rbx:        TCS, must be non-NULL
> > + * %rcx:        Optional pointer to 'struct sgx_enclave_exception'
> > + *
> > + * Return:
> > + *  0 on a clean entry/exit to/from the enclave
> > + *  -EINVAL if ENCLU leaf is not allowed or if TCS is NULL
> > + *  -EFAULT if ENCLU or the enclave faults
> > + *
> > + * Note that __vdso_sgx_enter_enclave() is not compliant with the x86-
> > 64 ABI.
> > + * All registers except RSP must be treated as volatile from the
> > +caller's
> > + * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR,
> > FCW, etc...
> > + * Conversely, the enclave being run must preserve the untrusted RSP
> > and stack.
> 
> By requiring preservation of RSP at both AEX and EEXIT, this precludes
> the possibility of using the untrusted stack as temporary storage by
> enclaves. While that looks reasonable at first glance, I'm afraid it
> isn't the case in reality. The untrusted stack is inarguably the most
> convenient way for data exchange between an enclave and its enclosing
> process,

I vehemently disagree with "inarguably".  IMO, passing data via registers
is much more convenient.

Even if you qualify your assertion with "data of arbitrary size unknown
at build time", I still disagree.  Using the untrusted stack allows for
trickery when a debugger is involved, other than that I see no advantages
over allocating virtual memory and handing the pointer to the enclave
at launch time.  Sure, it requires a few more lines of code to setup,
but it's literally ~20 LoC out of thousands required to sign, build and
launch an enclave, but it doesn't require playing games with the stack.

Not to mention that the entire concept of using the untrusted stack is
based on the assumption that the enclave is making ocalls, e.g. stateless
enclaves or libraries that use a message queue have zero need/benefit
for using the untrusted stack.

> and is in fact being used for that purpose by almost all existing enclaves
> to date.

That's a bit misleading, since almost all existing enclaves are built
against Intel's SDK, which just so happens to unconditionally use the
untrusted stack.  It's not like all enclave developers made a concious
decision to use the untrusted stack.  If Intel rewrote the SDK to use
a different method then one could argue that the new approach is the
most common method of passing data.

> Given the expectation that this API will be used by all future SGX
> application, it looks unwise to ban the most convenient and commonly
> used approach for data exchange.
> 
> Given an enclave can touch everything (registers and memory) of the
> enclosing process, it's reasonable to restrict the enclave by means
> of "calling convention" to allow the enclosing process to retain its
> context. And for that purpose, SGX ISA does offer 2 registers (i.e.
> RSP and RBP) for applications to choose. Instead of preserving RSP,
> I'd prefer RBP, which will end up with more flexibility in all SGX
> applications in future.

I disagree that the SGX ISA intends for applications to choose between
preserving RSP and RBP, e.g. the SDM description of SSA.UR{B,S}P states:

  Non-Enclave (outside) {RBP,stack} pointer. Saved by EENTER, restored on AEX.

To me, the "Saved/restored" wording implies that URBP and URSP should
*never* be touched by the enclave.  Sure, the proposed vDSO interface
doesn't require RBP to be preserved, but only because the goal was to
deviate from hardware as little as possible, not because anyone wants
to encourage enclaves to muck with RBP.