Re: [PATCH v19,RESEND 24/27] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions

[Andy Lutomirski <luto@kernel.org>]

> On Mar 23, 2019, at 10:36 AM, Xing, Cedric <cedric.xing@intel.com> wrote:
>
> Hi Sean,
>
>> Although its just 9 LOC, consider its impact on someone who is looking
>> at
>> the kernel's SGX support for the first time.  Questions they may have
>> when
>> looking at the vDSO code/documentation:
>>
>>  - What's an exit handler?
>>  - Why is an exit handler optional?  Don't I always want to handle
>> exits?
>>  - What value should my exit handler return?
>>  - What should my exit handler do if it detects an error?
>>  - Why would I want to preserve %rbp and not %rsp?
>>  - Isn't it insecure to use the untrusted stack in my enclave?
>>
>> AFAIK, the only reason to preserve %rbp instead of %rsp, i.e. support an
>> "exit handler" callback, is to be able to implement an o-call scheme
>> using
>> the untrusted stack to pass data.  Every idea I came up with for using
>> the
>> callback, e.g. logging, handling stack corruptiong, testing hooks,
>> etc...
>> was at worst no more difficult to implement when using a barebones vDSO.
>>
>> So, given the choice between a) documenting and maintaining all the
>> baggage
>> that comes with the exit handler and b) saying "go use signals", I chose
>> option b.
>
> Disagreed!
>
> This API is NOT even x86_64 compatible and NOT intended to be used by average developers. Instead, this API will be used by SGX SDK vendors who have all the needed background/expertise. And flexibility is way more important to them than reduced documentation. Just imagine how much one needs to read to understand how SGX works, do you really think a function comprised of 20 or so LOC will be a big deal?
>
> Anyway, the documentation needed IMO will not exceed even 1 page, which will be way shorter than most of docs in kernel source tree. I'll be more than happy to help you out if that's out of your competence!
>
> Regarding maintenance, I see an API may require maintenance for 2 possible categories of reasons: 1) its interface cannot satisfy emerging applications; or 2) the infrastructure it relies on has changed. Generally speaking, a more generic API with less assumption/dependence on other components will impose lower maintenance cost in the long run. Comparing our proposals, they share the same dependences (i.e. SGX ISA and vDSO extable) but mine is more generic (as yours could be implemented using mine as a subroutine). Thus, I bet your proposal will impose higher maintenance cost in the long run.
>
>

I’m going to put my vDSO maintainer hat on for a minute.  Cedric, your
proposal has the following issues related specifically to the vDSO:

It inherently contains indirect branches.  This means that, on
retpoline configurations, it probably needs to use retpolines.  This
is doable, but it’s nasty, and you need to worry about register
clobbers.

It uses effectively unbounded stack space. The vDSO timing functions
are already a problem for Go, and this is worse.

And with my vDSO hat back off, I find it disappointing that SGX SDKs
seem willing to couple the SGX enclaves so tightly to their host ABIs.
An *unmodified* SGX enclave should be able to run, without excessive
annoyance, in a Windows process, a Linux process, a C process, a Java
process, a Go process, and pretty much any other process.  Saying
“I’ll just recompile it” is a bad solution — for enclaves that use
MRENCLAVE, you can’t, and for enclaves that use MRSIGNER, you need to
deal with the fact the protecting the signing key is a big deal.
Someone should be able to port the entire host program to a different
language without losing secrets and without access to a signing key.

Cedric, your proposal allows an enclave to muck with RSP, but not in a
way that’s particularly pleasant.  Since the ISA is set in stone, we
can’t do anything about the enclave’s access to its caller’s
registers.  I would love to see a straightforward way to run an
enclave such that it does not access the main untrusted stack at all —
uRSP and uRBP should be arbitrary values passed in the untrusted code,
and the values the enclave sets should be relayed back to the caller
but otherwise not have any effect.  Sadly I see no way to do this
short of using GSBASE to store the real untrusted stack pointer.
Other than the segment bases, there appear to be literally zero
untrusted registers that are reliably preserved across an enclave
entry and exit.  I suppose we should use a syscall to help.

Since the above tricks seem unlikely to make it into the kernel, I
think we’re doing everyone a favor if the Linux APIs strongly
encourage SDK authors to build enclaves in a way that they don’t make
problematic assumptions about the untrusted world. I would really like
to see enclaves generated by the Linux SDK work on Windows and vice
versa.