Message-ID: <CAOASepMN1fmDaPjJJ-rpbLNPGUzE2LNB69s3X5mDBEhXziZ_UQ@mail.gmail.com>
Date:   Fri, 13 Mar 2020 12:07:55 -0400
From:   Nathaniel McCallum <npmccallum@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     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>,
        Jethro Beekman <jethro@...tanix.com>
Subject: Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX
 enclave call

On Thu, Mar 12, 2020 at 8:52 PM Sean Christopherson
<sean.j.christopherson@...el.com> wrote:
>
> On Wed, Mar 11, 2020 at 03:30:44PM -0400, Nathaniel McCallum wrote:
> > On Tue, Mar 3, 2020 at 6:40 PM Jarkko Sakkinen
> > <jarkko.sakkinen@...ux.intel.com> wrote:
> > > + * The exit handler's return value is interpreted as follows:
> > > + *  >0:                continue, restart __vdso_sgx_enter_enclave() with @ret as @leaf
> > > + *   0:                success, return @ret to the caller
> > > + *  <0:                error, return @ret to the caller
> > > + *
> > > + * The userspace exit handler is responsible for unwinding the stack, e.g. to
> > > + * pop @e, u_rsp and @tcs, prior to returning to __vdso_sgx_enter_enclave().
> >
> > Unless I misunderstand, this documentation...
>
> Hrm, that does appear wrong.  I'm guessing that was leftover from a previous
> incarnation of the code.  Or I botched the description, which is just as
> likely.

I figured out what happened on my end. This documentation error led me
to misread the code. More below.

> > > + * The exit handler may also transfer control, e.g. via longjmp() or a C++
> > > + * exception, without returning to __vdso_sgx_enter_enclave().
> > > + *
> > > + * Return:
> > > + *  0 on success,
> > > + *  -EINVAL if ENCLU leaf is not allowed,
> > > + *  -EFAULT if an exception occurs on ENCLU or within the enclave
> > > + *  -errno for all other negative values returned by the userspace exit handler
> > > + */
>
> ...
>
> > > +       /* Load the callback pointer to %rax and invoke it via retpoline. */
> > > +       mov     0x20(%rbp), %rax
> > > +       call    .Lretpoline
> > > +
> > > +       /* Restore %rsp to its post-exit value. */
> > > +       mov     %rbx, %rsp
> >
> > ... doesn't seem to match this code.
> >
> > If the handler pops from the stack and then we restore the stack here,
> > the handler had no effect.
> >
> > Also, one difference between this interface and a raw ENCLU[EENTER] is
> > that we can't pass arguments on the untrusted stack during EEXIT. If
> > we want to support that workflow, then we need to allow the ability
> > for the handler to pop "additional" values without restoring the stack
> > pointer to the exact value here.
>
> > Also, one difference between this interface and a raw ENCLU[EENTER] is
> > that we can't pass arguments on the untrusted stack during EEXIT. If
> > we want to support that workflow, then we need to allow the ability
> > for the handler to pop "additional" values without restoring the stack
> > pointer to the exact value here.
>
> The callback shenanigans exist precisely to allow passing arguments on the
> untrusted stack.  The vDSO is very careful to preserve the stack memory
> above RSP, and to snapshot RSP at the time of exit, e.g. the arguments in
> memory and their addresses relative to u_rsp live across EEXIT.  It's the
> same basic concept as regular function calls, e.g. the callee doesn't pop
> params off the stack, it just knows what addresses relative to RSP hold
> the data it wants.  The enclave, being the caller, is responsible for
> cleaning up u_rsp.
>
> FWIW, if the handler reaaaly wanted to pop off the stack, it could do so,
> fixup the stack, and then re-call __vdso_sgx_enter_enclave() instead of
> returning (to the original __vdso_sgx_enter_enclave()).

My understanding from the documentation issue above was that *if* you
wanted to push parameters back on the stack during enclave exit, you
would *have* to supply a handler so it could pop the parameters and
reset the stack. Which is why restoring %rsp from %rbx didn't make
sense to me.

Related to my other message in this thread, if
__vdso_sgx_enter_enclave() preserved %rbx and took @leaf as a stack
parameter, you could call __vdso_sgx_enter_enclave() from C so long as
the enclave didn't push return arguments on the stack. A workaround
for that case would be to fix up the stack in the handler. It would be
enough for the handler to simply set %rbx to the desired stack
location and return (though all of this unclean of course).

> > > +       /*
> > > +        * If the return from callback is zero or negative, return immediately,
> > > +        * else re-execute ENCLU with the postive return value interpreted as
> > > +        * the requested ENCLU leaf.
> > > +        */
> > > +       cmp     $0, %eax
> > > +       jle     .Lout
> > > +       jmp     .Lenter_enclave
> > > +
> > > +.Lretpoline:
> > > +       call    2f
> > > +1:     pause
> > > +       lfence
> > > +       jmp     1b
> > > +2:     mov     %rax, (%rsp)
> > > +       ret
> > > +       .cfi_endproc
> > > +
> > > +_ASM_VDSO_EXTABLE_HANDLE(.Lenclu_eenter_eresume, .Lhandle_exception)
>

Powered by blists - more mailing lists