[RFC,v3,0/3] x86/sgx: Amend vDSO API to allow enclave/host parameter passing on untrusted stack
mbox series

Message ID cover.1562813643.git.cedric.xing@intel.com
Headers show
Series
  • x86/sgx: Amend vDSO API to allow enclave/host parameter passing on untrusted stack
Related show

Message

Xing, Cedric July 11, 2019, 4:21 a.m. UTC
This patchset is based upon, and can be applied cleanly on SGX1 patch v20
(https://lkml.org/lkml/2019/4/17/344) by Jarkko Sakkinen.

The current proposed __vdso_sgx_enter_enclave() requires enclaves to preserve
%rsp, which prohibits enclaves from allocating space on the untrusted stack.
However, there are existing enclaves (e.g. those built with current Intel SGX
SDK libraries) relying on the untrusted stack for passing parameters to
untrusted functions (aka. o-calls), which requires allocating space on the
untrusted stack by enclaves. After all, passing data via untrusted stack is
very easy to implement (by enclaves), with essentially no overhead, therefore
is very suitable for exchanging data in small amounts, so could be desirable by
future SGX applications as well.  

This patchset introduces a new ABI for __vdso_sgx_enter_enclave() to anchor its
stack frame on %rbp (instead of %rsp), so as to allow enclaves to "push" onto
the untrusted stack by decrementing the untrusted %rsp. And in order to service
o-calls and to preserve the untrusted stack upon exceptions, the new vDSO API
takes one more optional parameter - "callback", which if supplied, will be
invoked on all enclave exits (including normal and asynchronous exits). Ample
details regarding the new ABI have been documented as comments inside the
source code located in arch/x86/entry/vsgx_enter_enclave.S

Please note that there was a lengthy discussion on what is the "best" approach
for passing parameters for trusted/untrusted calls. Unfortunately there's no
single "best" approach that fits all use cases, hence this new ABI has been
designed intentionally to accommodate varieties. Therefore, to those not
interested in using the untrusted stack, whatever worked with the old ABI
proposed by Sean will continue to work with this new ABI.

The SGX selftest has been augmented by two new tests. One exercises the new
callback interface, and serves as a simple example to showcase how to use it;
while the other validates the hand-crafted CFI directives in
__vdso_sgx_enter_enclave() by single-stepping through it and unwinding call
stack at every instruction. Please note that the selftest CANNOT run to
completion yet, as it depends on the vDSO fixup code to signal the process upon
#DB/#BP inside enclaves (rather than the current behavior of branching to the
handler in vDSO).

Changelog:
  · This is version 3 of this patch series with the following changes.
    - Per Andy Lutomirski and Sean Christopherson, revised comments and their
      format in arch/x86/entry/vsgx_enter_enclave.S
    - Per Jarkko Sakkinen, revised the cover letter to articulate motivation
      and objective of this patchset.
  · v2 - https://patchwork.kernel.org/cover/10914161/
  · v1 - https://patchwork.kernel.org/cover/10911615/

Cedric Xing (3):
  selftests/x86: Fixed Makefile for SGX selftest
  x86/vdso: Modify __vdso_sgx_enter_enclave() to allow parameter passing
    on untrusted stack
  selftests/x86: Augment SGX selftest to test new
    __vdso_sgx_enter_enclave() and its callback interface

 arch/x86/entry/vdso/vsgx_enter_enclave.S   | 214 ++++++++++----
 arch/x86/include/uapi/asm/sgx.h            |  14 +-
 tools/testing/selftests/x86/Makefile       |  12 +-
 tools/testing/selftests/x86/sgx/Makefile   |  49 ++--
 tools/testing/selftests/x86/sgx/main.c     | 323 ++++++++++++++++++---
 tools/testing/selftests/x86/sgx/sgx_call.S |  40 ++-
 6 files changed, 500 insertions(+), 152 deletions(-)

Comments

Jarkko Sakkinen July 12, 2019, 3:28 a.m. UTC | #1
On Wed, Jul 10, 2019 at 09:21:30PM -0700, Cedric Xing wrote:
> This patchset is based upon, and can be applied cleanly on SGX1 patch v20
> (https://lkml.org/lkml/2019/4/17/344) by Jarkko Sakkinen.
> 
> The current proposed __vdso_sgx_enter_enclave() requires enclaves to preserve
> %rsp, which prohibits enclaves from allocating space on the untrusted stack.
> However, there are existing enclaves (e.g. those built with current Intel SGX
> SDK libraries) relying on the untrusted stack for passing parameters to
> untrusted functions (aka. o-calls), which requires allocating space on the
> untrusted stack by enclaves. After all, passing data via untrusted stack is
> very easy to implement (by enclaves), with essentially no overhead, therefore
> is very suitable for exchanging data in small amounts, so could be desirable by
> future SGX applications as well.  
> 
> This patchset introduces a new ABI for __vdso_sgx_enter_enclave() to anchor its
> stack frame on %rbp (instead of %rsp), so as to allow enclaves to "push" onto
> the untrusted stack by decrementing the untrusted %rsp. And in order to service
> o-calls and to preserve the untrusted stack upon exceptions, the new vDSO API
> takes one more optional parameter - "callback", which if supplied, will be
> invoked on all enclave exits (including normal and asynchronous exits). Ample
> details regarding the new ABI have been documented as comments inside the
> source code located in arch/x86/entry/vsgx_enter_enclave.S
> 
> Please note that there was a lengthy discussion on what is the "best" approach
> for passing parameters for trusted/untrusted calls. Unfortunately there's no
> single "best" approach that fits all use cases, hence this new ABI has been
> designed intentionally to accommodate varieties. Therefore, to those not
> interested in using the untrusted stack, whatever worked with the old ABI
> proposed by Sean will continue to work with this new ABI.
> 
> The SGX selftest has been augmented by two new tests. One exercises the new
> callback interface, and serves as a simple example to showcase how to use it;
> while the other validates the hand-crafted CFI directives in
> __vdso_sgx_enter_enclave() by single-stepping through it and unwinding call
> stack at every instruction. Please note that the selftest CANNOT run to
> completion yet, as it depends on the vDSO fixup code to signal the process upon
> #DB/#BP inside enclaves (rather than the current behavior of branching to the
> handler in vDSO).
> 
> Changelog:
>   · This is version 3 of this patch series with the following changes.
>     - Per Andy Lutomirski and Sean Christopherson, revised comments and their
>       format in arch/x86/entry/vsgx_enter_enclave.S
>     - Per Jarkko Sakkinen, revised the cover letter to articulate motivation
>       and objective of this patchset.
>   · v2 - https://patchwork.kernel.org/cover/10914161/
>   · v1 - https://patchwork.kernel.org/cover/10911615/

1. I agree with the high level idea.
2. The patches do changes out of scope.

Generally, when doing kernel patches, even for a running patch set,
please do not do anything extra.

It is also stated in the kernel process;

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#separate-your-changes

Once these are fully cleaned up we can merge them.

/Jarkko