Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Andy Lutomirski]

vdso_sgx_enter_enclave() sucks. I don't think anyone seriously likes
it, but maybe it's the best we can do.

I'm wondering if it's worth trying to do better.  Here's what I'd like
if I could wave a magic wand:

struct sgx_enclave_run {
       __u64 tcs;
       __u32 flags;
       __u32 exit_reason;

    /*
     * These values are exposed to the enclave on entry, and the values
     * left behind by the enclave are returned here.
     * Some enclaves might write to memory pointed to by rsp.
     */
       __u64 rsp, rbp, r8, r9, r10, r11, r12, r13, r14, r15;
       /* Maybe other regs too? */

       union {
               struct sgx_enclave_exception exception;

               /* Pad the entire struct to 256 bytes. */
               __u8 pad[256 - 32];
       };
};

long vdso_sgx_enter_enclave(unsigned int leaf, struct sgx_enclave_run *r);

No callback, no asm wrapper needed, no nastiness from the perspective
of the caller.

So here are my questions.  First, do people agree with me that this
would be better?  Second, could this be implemented in a way that
doesn't utterly suck?  The best I've come up with so far is abusing
WRFSBASE to shove a little data structure containing the real user RSP
or RBP along with the old FSBASE into FSBASE, do EENTER, and then undo
the FSBASE dance.  We'd also need some additional exception fixup
magic to prevent a signal or ptrace() from observing the intermediate
states and getting extremely confused.

--Andy

Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Sean Christopherson]

+Nathaniel, Cedric and linux-sgx

On Fri, Sep 25, 2020 at 09:55:00AM -0700, Andy Lutomirski wrote:
> vdso_sgx_enter_enclave() sucks. I don't think anyone seriously likes
> it, but maybe it's the best we can do.

The code itself sucks, or the API sucks?

For the code, given the constraints of SGX and the number of runtimes we're
enabling, I don't think it's bad at all.  It's not hard to maintain, there are
no horrendous hacks, and it doesn't play games with the caller's state, i.e.
there's no additional magic required.  In other words, I really like that we
have in hand _works_, and works for a variety of runtimes and their ABIs.

The API isn't glorious, but it's not awful either.

> I'm wondering if it's worth trying to do better.  Here's what I'd like
> if I could wave a magic wand:
> 
> struct sgx_enclave_run {
>        __u64 tcs;
>        __u32 flags;
>        __u32 exit_reason;
> 
>     /*
>      * These values are exposed to the enclave on entry, and the values
>      * left behind by the enclave are returned here.
>      * Some enclaves might write to memory pointed to by rsp.
>      */
>        __u64 rsp, rbp, r8, r9, r10, r11, r12, r13, r14, r15;
>        /* Maybe other regs too? */
> 
>        union {
>                struct sgx_enclave_exception exception;
> 
>                /* Pad the entire struct to 256 bytes. */
>                __u8 pad[256 - 32];
>        };
> };
> 
> long vdso_sgx_enter_enclave(unsigned int leaf, struct sgx_enclave_run *r);
> 
> No callback, no asm wrapper needed, no nastiness from the perspective
> of the caller.
> 
> So here are my questions.  First, do people agree with me that this
> would be better?

No?

From a user perspective, I don't find the callback particularly onerous.
Avoiding the callback would yield prettier code for the caller, but IMO it
doesn't fundamentally make the code easier to maintain.  Implementing the
callback is a one-time cost for a relatively small number of people, and it's
not even all that difficult to write, especially compared to all the other crud
that needs to be done to enable SGX.

As for requiring assembly to pass state in r10-r15, IMO that's a non-issue as
it's trivially easy for the runtime/enclave ABI to pass its own params struct,
e.g. my runtime for testing basically does:

int enter_enclave(const long fn, void *params, tcs_t *tcs)
{
	...

	ret = __enter_enclave(fn, (unsigned long)params, 0, EENTER, 0, 0, &run);

	...
}

where __enter_enclave is a function pointer to the vDSO.  The enclave side
looks very similar, e.g. there's a small amount of asm trampoline to invoke
a C function that looks like:

int eenter_handler(const long fn, void *params, void *tcs, int ssa)
{
	...
}

where @params is eventually passed down to the function referenced by @fn.

Actually consuming r10-r15 without assembly of some form is impossible (unless
there's compiler magic of which I'm unaware).  Not to mention that r10-r15 are
especially painful to use because they don't have dedicated input/output
constraints for inline asm.

In other words, I don't see any value in making it easier for the caller to
pass state in r10-r15 (I'm intentionally omitting r8 and r9, because those can
be passed as parameters to the vDSO function, I have no idea why we'd want to
move them into sgx_enclave_run).

So it really comes down to context switching versus using a callback.  Not that
this is exactly a hot path, but for me having to implement a callback versus
bloating the vDSO with state save and restore is a mostly a wash.

> Second, could this be implemented in a way that doesn't utterly suck?  The
> best I've come up with so far is abusing WRFSBASE to shove a little data
> structure containing the real user RSP or RBP along with the old FSBASE into
> FSBASE, do EENTER, and then undo the FSBASE dance.  We'd also need some
> additional exception fixup magic to prevent a signal or ptrace() from
> observing the intermediate states and getting extremely confused.

But where would the vDSO get memory for that little data structure?  It can't
be percpu because the current task can get preempted.  It can't be per instance
of the vDSO because a single mm/process can have multiple tasks entering an
enclave.  Per task might work, but how would the vDSO get that info?  E.g.
via a syscall, which seems like complete overkill?

So, no?

If we truly want to provide a "better" vDSO, my vote would be to disallow using
the runtime's stack from within the enclave.  But I'm guessing that would cause
more than a few aneurysms for the folks that are cc'd :-)

Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Andy Lutomirski]

On Fri, Sep 25, 2020 at 12:09 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> +Nathaniel, Cedric and linux-sgx
>
> On Fri, Sep 25, 2020 at 09:55:00AM -0700, Andy Lutomirski wrote:
> > vdso_sgx_enter_enclave() sucks. I don't think anyone seriously likes
> > it, but maybe it's the best we can do.
>
> The code itself sucks, or the API sucks?

I'm referring to the API.

>
> For the code, given the constraints of SGX and the number of runtimes we're
> enabling, I don't think it's bad at all.  It's not hard to maintain, there are
> no horrendous hacks, and it doesn't play games with the caller's state, i.e.
> there's no additional magic required.  In other words, I really like that we
> have in hand _works_, and works for a variety of runtimes and their ABIs.

Well, it _works_, but I see at least two implementation issues.
There's the CET problem that Andrew Cooper spotted (solvable without
too much work).  Also, the code really ought to have .cfi annotations
because it's user code and userspace expects libunwind, gdb, etc to
work.

>
> The API isn't glorious, but it's not awful either.
>
> > I'm wondering if it's worth trying to do better.  Here's what I'd like
> > if I could wave a magic wand:
> >
> > struct sgx_enclave_run {
> >        __u64 tcs;
> >        __u32 flags;
> >        __u32 exit_reason;
> >
> >     /*
> >      * These values are exposed to the enclave on entry, and the values
> >      * left behind by the enclave are returned here.
> >      * Some enclaves might write to memory pointed to by rsp.
> >      */
> >        __u64 rsp, rbp, r8, r9, r10, r11, r12, r13, r14, r15;
> >        /* Maybe other regs too? */
> >
> >        union {
> >                struct sgx_enclave_exception exception;
> >
> >                /* Pad the entire struct to 256 bytes. */
> >                __u8 pad[256 - 32];
> >        };
> > };
> >
> > long vdso_sgx_enter_enclave(unsigned int leaf, struct sgx_enclave_run *r);
> >
> > No callback, no asm wrapper needed, no nastiness from the perspective
> > of the caller.
> >
> > So here are my questions.  First, do people agree with me that this
> > would be better?
>
> No?
>
> From a user perspective, I don't find the callback particularly onerous.
> Avoiding the callback would yield prettier code for the caller, but IMO it
> doesn't fundamentally make the code easier to maintain.  Implementing the
> callback is a one-time cost for a relatively small number of people, and it's
> not even all that difficult to write, especially compared to all the other crud
> that needs to be done to enable SGX.

[lots of discussion about r8 .. r15]

r8 through r15 are kind of red herrings.  I think that supporting them
as C-callable input/output args in the vDSO would be polite but isn't
really important. The actual important part I'm thinking of is RSP,
RBP, and the callback.

>
> So it really comes down to context switching versus using a callback.  Not that
> this is exactly a hot path, but for me having to implement a callback versus
> bloating the vDSO with state save and restore is a mostly a wash.
>
> > Second, could this be implemented in a way that doesn't utterly suck?  The
> > best I've come up with so far is abusing WRFSBASE to shove a little data
> > structure containing the real user RSP or RBP along with the old FSBASE into
> > FSBASE, do EENTER, and then undo the FSBASE dance.  We'd also need some
> > additional exception fixup magic to prevent a signal or ptrace() from
> > observing the intermediate states and getting extremely confused.
>
> But where would the vDSO get memory for that little data structure?  It can't
> be percpu because the current task can get preempted.  It can't be per instance
> of the vDSO because a single mm/process can have multiple tasks entering an
> enclave.  Per task might work, but how would the vDSO get that info?  E.g.
> via a syscall, which seems like complete overkill?

The stack.  The vDSO could, logically, do:

struct sgx_entry_state {
  unsigned long real_rbp;
  unsigned long real_rsp;
  unsigned long orig_fsbase;
};

...

  struct sgx_entry_state state;
  state.rbp = rbp;  [ hey, this is pseudocode.  the real code would be in asm.]
  state.rsp = rsp;
  state.fsbase = __rdfsbase();
  rbp = arg->rbp;

  /* set up all other regs */
  wrfsbase %rsp
  movq enclave_rsp(%rsp), %rsp
  enclu
  rdfsbase %rsp

Expressing this abomination using DWARF is surely entirely impossible,
so instead we would want to make sure that user code simply cannot
observe the state between the wrfsbase and the rdfsbase.  This would
require three exception hooks, which is more than we have now but is
not actually unmanageable.  And it would be possible to write a test
case to make sure we got it right.  A downside is that single-stepping
this mess would probably not work in any sensible way.

This could also be handled more slowly by introducing an actual
syscall so the kernel could help out.  This would also be pretty ugly.

On the other hand, if we had some confidence that the existing corpus
of enclaves plays games with RSP but not RBP, we could handle this
much more straightforwardly and even likely perserve DWARF support,
although we'd still have strange artifacts if we got a signal in the
wrong place.

(If we have CET, we could do truly unspeakable things involving saving
our state on the shadow stack.  Don't think too hard about this.)

>
> So, no?
>
> If we truly want to provide a "better" vDSO, my vote would be to disallow using
> the runtime's stack from within the enclave.  But I'm guessing that would cause
> more than a few aneurysms for the folks that are cc'd :-)

Indeed.

My real problem with the callback is that it forces the untrusted
runtime to use a C-like stack model and to keep the vDSO entry on the
stack while handling the OCALL or whatever you want to call it.  This
is fine for those of us who exclusively program in C, aren't doing
anything fancy, and want to think of enclaves as just more C code.
But enclaves aren't just more C code -- they're more-or-less-immutable
blobs, and they can't easily evolve with the untrusted code that
invokes them.  And the world is slowly but surely moving toward
languages that aren't quite as C-like as C.  In a language with
stackless coroutines or async/await or continuations or goroutines,
this could all get quite awkward.  Sure, a really nice Rust or Go SGX
untrusted runtime could just declare that it won't support enclaves
that touch the stack, but that's a bit of an unfortunate restriction
given that removing stack access from an existing enclave will
inevitably change MRENCLAVE.

If I thought there was a straightforward way to support all existing
enclaves without callbacks in the vDSO, I would be all for it.  I'm
wondering if doing so even unstraightforwardly would still be
worthwhile.

If everyone wants to tell me that what we have now (plus .cfi
annotations and perhaps a CET fix) is good enough, then so be it.  But
I figured I'd ask.

--Andy

Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Sean Christopherson]

On Fri, Sep 25, 2020 at 01:20:03PM -0700, Andy Lutomirski wrote:
> On Fri, Sep 25, 2020 at 12:09 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > But where would the vDSO get memory for that little data structure?  It can't
> > be percpu because the current task can get preempted.  It can't be per instance
> > of the vDSO because a single mm/process can have multiple tasks entering an
> > enclave.  Per task might work, but how would the vDSO get that info?  E.g.
> > via a syscall, which seems like complete overkill?
> 
> The stack.

Duh.

> The vDSO could, logically, do:
> 
> struct sgx_entry_state {
>   unsigned long real_rbp;
>   unsigned long real_rsp;
>   unsigned long orig_fsbase;
> };
> 
> ...
> 
>   struct sgx_entry_state state;
>   state.rbp = rbp;  [ hey, this is pseudocode.  the real code would be in asm.]
>   state.rsp = rsp;
>   state.fsbase = __rdfsbase();
>   rbp = arg->rbp;
> 
>   /* set up all other regs */
>   wrfsbase %rsp
>   movq enclave_rsp(%rsp), %rsp

I think this is where there's a disconnect with what is being requested by the
folks writing run times.  IIUC, they want to use the untrusted runtime's stack
to pass params because it doesn't require additional memory allocations and
automagically grows as necessary (obviously to a certain limit).  I.e. forcing
the caller to provide an alternative "stack" defeats the purpose of using the
untrusted stack.

>   enclu
>   rdfsbase %rsp
>
> Expressing this abomination using DWARF is surely entirely impossible,
> so instead we would want to make sure that user code simply cannot
> observe the state between the wrfsbase and the rdfsbase.  This would
> require three exception hooks, which is more than we have now but is
> not actually unmanageable.  And it would be possible to write a test
> case to make sure we got it right.  A downside is that single-stepping
> this mess would probably not work in any sensible way.
> 
> This could also be handled more slowly by introducing an actual
> syscall so the kernel could help out.  This would also be pretty ugly.
> 
> On the other hand, if we had some confidence that the existing corpus
> of enclaves plays games with RSP but not RBP, we could handle this
> much more straightforwardly and even likely perserve DWARF support,
> although we'd still have strange artifacts if we got a signal in the
> wrong place.

Heh, I had exactly that typed out, then I reread this paragraph :-)

We do have this confidence, because it's required by the current version
of the VDSO.

> (If we have CET, we could do truly unspeakable things involving saving
> our state on the shadow stack.  Don't think too hard about this.)
> 
> >
> > So, no?
> >
> > If we truly want to provide a "better" vDSO, my vote would be to disallow using
> > the runtime's stack from within the enclave.  But I'm guessing that would cause
> > more than a few aneurysms for the folks that are cc'd :-)
> 
> Indeed.
> 
> My real problem with the callback is that it forces the untrusted
> runtime to use a C-like stack model and to keep the vDSO entry on the
> stack while handling the OCALL or whatever you want to call it.

But the callback is optional...  It's effectively mandatory if you want to
invoke the vDSO from anything but assembly, but it's not required.

> This is fine for those of us who exclusively program in C, aren't doing
> anything fancy, and want to think of enclaves as just more C code.  But
> enclaves aren't just more C code -- they're more-or-less-immutable blobs, and
> they can't easily evolve with the untrusted code that invokes them.  And the
> world is slowly but surely moving toward languages that aren't quite as
> C-like as C.  In a language with stackless coroutines or async/await or
> continuations or goroutines, this could all get quite awkward.  Sure, a
> really nice Rust or Go SGX untrusted runtime could just declare that it won't
> support enclaves that touch the stack, but that's a bit of an unfortunate
> restriction given that removing stack access from an existing enclave will
> inevitably change MRENCLAVE.

But if these use cases come along (using a modern runtime to run existing
enclaves that do stupid things), and we didn't want to foist the effort of
writing an assembly wrapper onto the runtimes, couldn't we simply add a flag to
opt into saving non-volatile registers and/or loading an alternative RSP?  E.g.

test	$1,	SGX_ENCLAVE_RUN_FLAGS(%rcx)
	jz	.Lload_tsc
	mov	%rsp, SGX_SAVED_RSP(%rbp)
	mov	SGX_ENCLAVE_RUN_RSP(%rcx), %rsp

	/* Load TCS and AEP */
.Lload_tsc:
	mov	SGX_ENCLAVE_RUN_TCS(%rcx), %rbx
	lea	.Lasync_exit_pointer(%rip), %rcx

	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
.Lasync_exit_pointer:
.Lenclu_eenter_eresume:
	enclu

	/* EEXIT jumps here unless the enclave is doing something fancy. */
	mov	SGX_ENCLAVE_RUN_PTR(%rbp), %rbx
	test	$1, SGX_ENCLAVE_RUN_FLAGS(%rbx)
	jz	.Lblahblahblah

	mov	%rsp, SGX_ENCLAVE_RUN_RSP(%rbx)
	mov	SGX_SAVED_RSP(%rbp), %rsp

.Lblahblahblah:

> If I thought there was a straightforward way to support all existing
> enclaves without callbacks in the vDSO, I would be all for it.  I'm
> wondering if doing so even unstraightforwardly would still be
> worthwhile.

Hmm, so IIUC, your pie in the sky goal is providing a vDSO that:

  a) is callable from C
  b) supports all existing enclaves
  c) doesn't require a callback

The problem is (c) conflicts the aforementioned desire to use the runtime's 
_stack_, not simply whatever SSA.RSP points at.  We can play games to allow
using the stack as is without a callback, but that would add some non-trivial
nastiness in the caller to avoid clobbering the "enclave's" stack, especially
for runtimes/enclaves that are reentrant.

> If everyone wants to tell me that what we have now (plus .cfi
> annotations and perhaps a CET fix) is good enough, then so be it.  But
> I figured I'd ask.

Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Andy Lutomirski]

On Fri, Sep 25, 2020 at 3:29 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, Sep 25, 2020 at 01:20:03PM -0700, Andy Lutomirski wrote:
> > On Fri, Sep 25, 2020 at 12:09 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > > But where would the vDSO get memory for that little data structure?  It can't
> > > be percpu because the current task can get preempted.  It can't be per instance
> > > of the vDSO because a single mm/process can have multiple tasks entering an
> > > enclave.  Per task might work, but how would the vDSO get that info?  E.g.
> > > via a syscall, which seems like complete overkill?
> >
> > The stack.
>
> Duh.
>
> > The vDSO could, logically, do:
> >
> > struct sgx_entry_state {
> >   unsigned long real_rbp;
> >   unsigned long real_rsp;
> >   unsigned long orig_fsbase;
> > };
> >
> > ...
> >
> >   struct sgx_entry_state state;
> >   state.rbp = rbp;  [ hey, this is pseudocode.  the real code would be in asm.]
> >   state.rsp = rsp;
> >   state.fsbase = __rdfsbase();
> >   rbp = arg->rbp;
> >
> >   /* set up all other regs */
> >   wrfsbase %rsp
> >   movq enclave_rsp(%rsp), %rsp
>
> I think this is where there's a disconnect with what is being requested by the
> folks writing run times.  IIUC, they want to use the untrusted runtime's stack
> to pass params because it doesn't require additional memory allocations and
> automagically grows as necessary (obviously to a certain limit).  I.e. forcing
> the caller to provide an alternative "stack" defeats the purpose of using the
> untrusted stack.

I personally find this concept rather distasteful.  Sure, it might
save a couple cycles, but it means that the enclave has hardcoded some
kind of assumption about the outside-the-enclave stack.

Given that RBP seems reasonably likely to be stable across enclave
executions, I suppose we could add a flag and an RSP value in the
sgx_enclave_run structure.  If set, the vDSO would swap out RSP (but
not RBP) with the provided value on entry and record the new RSP on
exit.  I don't know if this would be useful to people.

I do think we need to add at least minimal CFI annotations no matter what we do.

Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Xing, Cedric]

On 9/26/2020 12:05 PM, Andy Lutomirski wrote:
> On Fri, Sep 25, 2020 at 3:29 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
>>
>> On Fri, Sep 25, 2020 at 01:20:03PM -0700, Andy Lutomirski wrote:
>>> On Fri, Sep 25, 2020 at 12:09 PM Sean Christopherson
>>> <sean.j.christopherson@intel.com> wrote:
>>>> But where would the vDSO get memory for that little data structure?  It can't
>>>> be percpu because the current task can get preempted.  It can't be per instance
>>>> of the vDSO because a single mm/process can have multiple tasks entering an
>>>> enclave.  Per task might work, but how would the vDSO get that info?  E.g.
>>>> via a syscall, which seems like complete overkill?
>>>
>>> The stack.
>>
>> Duh.
>>
>>> The vDSO could, logically, do:
>>>
>>> struct sgx_entry_state {
>>>    unsigned long real_rbp;
>>>    unsigned long real_rsp;
>>>    unsigned long orig_fsbase;
>>> };
>>>
>>> ...
>>>
>>>    struct sgx_entry_state state;
>>>    state.rbp = rbp;  [ hey, this is pseudocode.  the real code would be in asm.]
>>>    state.rsp = rsp;
>>>    state.fsbase = __rdfsbase();
>>>    rbp = arg->rbp;
>>>
>>>    /* set up all other regs */
>>>    wrfsbase %rsp
>>>    movq enclave_rsp(%rsp), %rsp
>>
>> I think this is where there's a disconnect with what is being requested by the
>> folks writing run times.  IIUC, they want to use the untrusted runtime's stack
>> to pass params because it doesn't require additional memory allocations and
>> automagically grows as necessary (obviously to a certain limit).  I.e. forcing
>> the caller to provide an alternative "stack" defeats the purpose of using the
>> untrusted stack.
> 
> I personally find this concept rather distasteful.  Sure, it might
> save a couple cycles, but it means that the enclave has hardcoded some
> kind of assumption about the outside-the-enclave stack.
> 

It's more than just a couple of cycles. It's convenience. Yes, an 
enclave may overflow the caller's stack with big allocations but those 
are rare. In more common cases less than the red zone size (128 bytes) 
are required. And we should optimize for the more common cases.

And yes again, the enclave has to assume something about the stack. But 
please note that the vDSO has to save its "context" somewhere so that it 
can switch back to it. The "context" currently is anchored at RBP so the 
enclave has to preserve it. If not RBP, the "context" has to anchor 
"something else", and we have to assume the enclave preserve that 
"something else". That said, we can't get rid of assumptions. RBP is a 
reasonable choice because it is simple without obvious side effects, 
i.e. most compilers/ABIs preserve RBP so developers don't have to pay 
extra attention to it generally.

If I were asked to opine on the API, I'd say I like the most the initial 
version with callback support. The stack parameters were easier to 
set/retrieve than struct members (requiring hand-crafted offset macros) 
in asm, and didn't need any padding. The callback was easy to use 
(non-NULL pointer) or skip (NULL pointer). Standard/unified error codes 
were easier to handle than separate error/exit_reason. Additional data 
for callback could be captured in a structure enclosing 
sgx_enclave_exception so no need to be explicitly passed (languages that 
don't support offsetof/container_of can always employ an asm wrapper). 
The current API looks confusing and overly complicated to me, even 
though it still works.

> Given that RBP seems reasonably likely to be stable across enclave
> executions, I suppose we could add a flag and an RSP value in the
> sgx_enclave_run structure.  If set, the vDSO would swap out RSP (but
> not RBP) with the provided value on entry and record the new RSP on
> exit.  I don't know if this would be useful to people.
> 

I would say, if one wants to use a different untrusted stack for calling 
the enclave, he/she could switch stack before calling vDSO. Given this 
isn't commonly required, I vote NO here.


> I do think we need to add at least minimal CFI annotations no matter what we do.
> 

Can't agree more.

Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 3597 bytes --]

On 2020-09-25 18:55, Andy Lutomirski wrote:
> vdso_sgx_enter_enclave() sucks. I don't think anyone seriously likes
> it, but maybe it's the best we can do.

I don't agree that it sucks. I think it's pretty good. My goal is to 1) support existing enclaves and 2) have it as close to ENCLU as possible. Goal 2 since I want to use it in cross-platform code and other platforms don't have this kind of call, so I'm forced to use assembly anyway.

The only concerns I have are that I need 256 bytes of memory (but I can put that on the stack I suppose) and the automatic restarting, but I've agreed to postpone the latter issue post-merge.

Now I'm speaking from a perspective of someone who's not planning to use the callback/user stack, so perhaps people using those features might think the current implementation is not that great.

> 
> I'm wondering if it's worth trying to do better.  Here's what I'd like
> if I could wave a magic wand:
> 
> struct sgx_enclave_run {
>        __u64 tcs;
>        __u32 flags;
>        __u32 exit_reason;
> 
>     /*
>      * These values are exposed to the enclave on entry, and the values
>      * left behind by the enclave are returned here.
>      * Some enclaves might write to memory pointed to by rsp.
>      */
>        __u64 rsp, rbp, r8, r9, r10, r11, r12, r13, r14, r15;
>        /* Maybe other regs too? */

I think it's fine to add a mechanism to pass rsp & rbp, and I support innovation in this space. But we should aim to keep the other register passing as close as possible to the instruction/what the current implementation does. For the other state, it certainly encompasses more than just the 16 GPRs. There's also rflags, XMM, etc. Future processors might add even more state. Capturing all state in this structure seems unnecessary.

On 2020-09-25 21:09, Sean Christopherson wrote:
> For the code, given the constraints of SGX and the number of runtimes we're
> enabling, I don't think it's bad at all.  It's not hard to maintain, there are
> no horrendous hacks, and it doesn't play games with the caller's state, i.e.
> there's no additional magic required.  In other words, I really like that we
> have in hand _works_, and works for a variety of runtimes and their ABIs.
> 
> The API isn't glorious, but it's not awful either.

Yup.

On 2020-09-25 22:20, Andy Lutomirski wrote:
> On the other hand, if we had some confidence that the existing corpus
> of enclaves plays games with RSP but not RBP

Pretty sure this is the case.

> languages that aren't quite as C-like as C.  In a language with
> stackless coroutines or async/await or continuations or goroutines,
> this could all get quite awkward.  Sure, a really nice Rust or Go SGX
> untrusted runtime could just declare that it won't support enclaves
> that touch the stack, but that's a bit of an unfortunate restriction
> given that removing stack access from an existing enclave will
> inevitably change MRENCLAVE.

I can say with confidence that v38 proposal can be used by async Rust code if the enclave doesn't use the user stack.

> If everyone wants to tell me that what we have now (plus .cfi
> annotations and perhaps a CET fix) is good enough, then so be it.

Good enough for me.

On 2020-09-26 00:29, Sean Christopherson wrote:
> We do have this confidence, because it's required by the current version
> of the VDSO.

Do you mean “it's required by the current version of the VDSO” and nobody has complained about it?

> But the callback is optional...

Yup.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]