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

["Xing, Cedric" <cedric.xing@intel.com>]

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.