Message-ID: <CAOASepP_oGOenjCvAvLg+e+=fz4H0X=cyD+v5ywD0peeXEEmYg@mail.gmail.com>
Date:   Fri, 13 Mar 2020 16:14:01 -0400
From:   Nathaniel McCallum <npmccallum@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Jethro Beekman <jethro@...tanix.com>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-sgx@...r.kernel.org, akpm@...ux-foundation.org,
        dave.hansen@...el.com, Neil Horman <nhorman@...hat.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        andriy.shevchenko@...ux.intel.com, tglx@...utronix.de,
        "Svahn, Kai" <kai.svahn@...el.com>, bp@...en8.de,
        Josh Triplett <josh@...htriplett.org>, luto@...nel.org,
        kai.huang@...el.com, David Rientjes <rientjes@...gle.com>,
        cedric.xing@...el.com, Patrick Uiterwijk <puiterwijk@...hat.com>,
        Andy Lutomirski <luto@...capital.net>,
        Connor Kuehl <ckuehl@...hat.com>,
        Harald Hoyer <harald@...hat.com>,
        Lily Sturmann <lsturman@...hat.com>
Subject: Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX
 enclave call

On Fri, Mar 13, 2020 at 2:45 PM Sean Christopherson
<sean.j.christopherson@...el.com> wrote:
>
> On Fri, Mar 13, 2020 at 02:32:29PM -0400, Nathaniel McCallum wrote:
> > On Fri, Mar 13, 2020 at 12:46 PM Sean Christopherson
> > <sean.j.christopherson@...el.com> wrote:
> > >
> > > On Fri, Mar 13, 2020 at 11:48:54AM -0400, Nathaniel McCallum wrote:
> > > > Therefore, I'd like to propose that __vdso_sgx_enter_enclave():
> > > >   * Preserve %rbx.
> > >
> > > At first glance, that looks sane.  Being able to call __vdso... from C
> > > would certainly be nice.
> >
> > Agreed. I think ergonomically we want __vdso...() to be called from C
> > and the handler to be implemented in asm (optionally); without
> > breaking the ability to call __vdso..() from asm in special cases.
> >
> > I think all ergonomic issues get solved by the following:
> >    * Pass a void * into the handler from C through __vdso...().
> >    * Allow the handler to pop parameters off of the output stack without hacks.
> >
> > This allows the handler to pop extra arguments off the stack and write
> > them into the memory at the void *. Then the handler can be very small
> > and pass logic back to the caller of __vdso...().
> >
> > Here's what this all means for the enclave. For maximum usability, the
> > enclave should preserve all callee-saved registers (except %rbx, which
> > is preserved by __vdso..()). For each ABI rule that the enclave
> > breaks, you need logic in a handler to fix it. So if you push return
> > params on the stack, the handler needs to undo that.
>
> Or the untrusted runtime needs to wrap the __vdso() to save state that is
> clobbered by the enclave.  Just want to make it crystal clear that using a
> handler is only required for stack shenanigans.

Yup.

> > This doesn't compromise the ability to treat __vsdo...() like ENCLU if
> > you need the full power. But it does make it significantly easier to
> > consume when you don't have special needs. So as I see it, __vdso...()
> > should:
> >
> > 1. preserve %rbx
> > 2. take leaf in %rcx
> > 3. gain a void* stack param which is passed to the handler
>
> Unless I'm misunderstanding the request, this already exists.  %rsp at the
> time of EEXIT is passed to the handler.

Sorry, different stack parameter. I mean this:

typedef int (*sgx_enclave_exit_handler_t)(
    long rdi,
    long rsi,
    long rdx,
    long ursp,
    long r8,
    long r9,
    int ret,
    void *tcs,
    struct sgx_enclave_exception *e,
    void *misc
);

int __vdso_sgx_enter_enclave(
    long rdi,
    long rsi,
    long rdx,
    int leaf,
    long r8,
    long r9,
    void *tcs,
    struct sgx_enclave_exception *e,
    void *misc,
    sgx_enclave_exit_handler_t handler
);

This is so that the caller of __vdso...() can pass context into the
handler. Note that I've also reordered the stack parameters so that
the stack order can be reused.

> > 4. sub/add to %rsp rather than save/restore
>
> Can you elaborate on why you want to sub/add to %rsp instead of having the
> enclave unwind the stack?  Preserving %rsp across EEXIT/ERESUME seems more
> in line with function call semantics, which I assume is desirable?  E.g.
>
>   push param3
>   push param2
>   push param1
>
>   enclu[EEXIT]
>
>   add $0x18, %rsp

Before enclave EEXIT, the enclave restores %rsp to the value it had
before EENTER was called. Then it pushes additional output arguments
onto the stack. The enclave calls EENCLU[EEXIT].

We are now in __vdso...() on the way back to the caller. However, %rsp
has a different value than we entered the function with. This breaks
x86_64 ABI, obviously. The handler needs to fix this up: how does it
do so?

In the current code, __vdso..() saves the value of %rsp, calls the
handler and then restores %rsp. The handler can fix up the stack by
setting the correct value to %rbx and returning without restoring it.
But this requires internal knowledge of the __vdso...() function,
which could theoretically change in the future.

If instead the __vdso...() only did add/sub, then the handler could do:
1. pop return address
2. pop handler stack params
3. pop enclave additional output stack params
4. push handler stack params
5. push return address

While this is more work, it is standard calling convention work that
doesn't require internal knowledge of __vdso..(). Alternatively, if we
don't like the extra work, we can document the %rbx hack explicitly
into the handler documentation and make it part of the interface. But
we need some explicit way for the handler to pop enclave output stack
params that doesn't depend on internal knowledge of the __vdso...()
invariants.

> > That would make this a very usable and fast interface without
> > sacrificing any of its current power.
>

Powered by blists - more mailing lists