Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?
From: Xing, Cedric
Date: Sat Sep 26 2020 - 19:56:04 EST
On 9/26/2020 12:05 PM, Andy Lutomirski wrote:
On Fri, Sep 25, 2020 at 3:29 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> 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@xxxxxxxxx> 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.