Re: [RFC PATCH v1 2/3] x86/vdso: Modify __vdso_sgx_enter_enclave() to allow parameter passing on untrusted stack

From: Sean Christopherson
Date: Tue Apr 23 2019 - 15:31:59 EST


On Mon, Apr 22, 2019 at 05:37:24PM -0700, Cedric Xing wrote:
> The previous __vdso_sgx_enter_enclave() requires enclaves to preserve %rsp,
> which prohibits enclaves from allocating and passing parameters for
> untrusted function calls (aka. o-calls).
>
> This patch addresses the problem above by introducing a new ABI that preserves
> %rbp instead of %rsp. Then __vdso_sgx_enter_enclave() can anchor its frame
> using %rbp so that enclaves are allowed to allocate space on the untrusted
> stack by decrementing %rsp. Please note that the stack space allocated in such
> way will be part of __vdso_sgx_enter_enclave()'s frame so will be freed after
> __vdso_sgx_enter_enclave() returns. Therefore, __vdso_sgx_enter_enclave() has
> been changed to take a callback function as an optional parameter, which if
> supplied, will be invoked upon enclave exits (both AEX (Asynchronous Enclave
> eXit) and normal exits), with the value of %rsp left
> off by the enclave as a parameter to the callback.
>
> Here's the summary of API/ABI changes in this patch. More details could be
> found in arch/x86/entry/vdso/vsgx_enter_enclave.S.
> * 'struct sgx_enclave_exception' is renamed to 'struct sgx_enclave_exinfo'
> because it is filled upon both AEX (i.e. exceptions) and normal enclave
> exits.
> * __vdso_sgx_enter_enclave() anchors its frame using %rbp (instead of %rsp in
> the previous implementation).
> * __vdso_sgx_enter_enclave() takes one more parameter - a callback function to
> be invoked upon enclave exits. This callback is optional, and if not
> supplied, will cause __vdso_sgx_enter_enclave() to return upon enclave exits
> (same behavior as previous implementation).
> * The callback function is given as a parameter the value of %rsp at enclave
> exit to address data "pushed" by the enclave. A positive value returned by
> the callback will be treated as an ENCLU leaf for re-entering the enclave,
> while a zero or negative value will be passed through as the return
> value of __vdso_sgx_enter_enclave() to its caller. It's also safe to
> leave callback by longjmp() or by throwing a C++ exception.
>
> Signed-off-by: Cedric Xing <cedric.xing@xxxxxxxxx>
> ---
> arch/x86/entry/vdso/vsgx_enter_enclave.S | 156 ++++++++++++++---------
> arch/x86/include/uapi/asm/sgx.h | 14 +-
> 2 files changed, 100 insertions(+), 70 deletions(-)
>
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index fe0bf6671d6d..210f4366374a 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -14,88 +14,118 @@
> .code64
> .section .text, "ax"
>
> -#ifdef SGX_KERNEL_DOC

This #ifdef and the pseudo-C code below has a functional purpose. From
the original commit:

Note, the C-like pseudocode describing the assembly routine is wrapped
in a non-existent macro instead of in a comment to trick kernel-doc into
auto-parsing the documentation and function prototype. This is a double
win as the pseudocode is intended to aid kernel developers, not userland
enclave developers.

We don't need full pseudocode, but a C-like prototype is necessary to get
the kernel-doc comment parsed correctly.

I'll try to look at the actual code later today.

> /**
> * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> *
> * @leaf: **IN \%eax** - ENCLU leaf, must be EENTER or ERESUME
> - * @tcs: **IN \%rbx** - TCS, must be non-NULL
> - * @ex_info: **IN \%rcx** - Optional 'struct sgx_enclave_exception' pointer
> + * @tcs: **IN 0x08(\%rsp)** - TCS, must be non-NULL
> + * @ex_info: **IN 0x10(\%rsp)** - Optional 'struct sgx_enclave_exinfo'
> + * pointer
> + * @callback: **IN 0x18(\%rsp)** - Optional callback function to be called on
> + * enclave exit or exception
> *
> * Return:
> * **OUT \%eax** -
> - * %0 on a clean entry/exit to/from the enclave, %-EINVAL if ENCLU leaf is
> - * not allowed or if TCS is NULL, %-EFAULT if ENCLU or the enclave faults
> + * %0 on a clean entry/exit to/from the enclave, %-EINVAL if ENCLU leaf is not
> + * allowed, %-EFAULT if ENCLU or the enclave faults, or a non-positive value
> + * returned from ``callback`` (if one is supplied).
> *
> * **Important!** __vdso_sgx_enter_enclave() is **NOT** compliant with the
> - * x86-64 ABI, i.e. cannot be called from standard C code. As noted above,
> - * input parameters must be passed via ``%eax``, ``%rbx`` and ``%rcx``, with
> - * the return value passed via ``%eax``. All registers except ``%rsp`` must
> - * be treated as volatile from the caller's perspective, including but not
> - * limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... Conversely, the enclave
> - * being run **must** preserve the untrusted ``%rsp`` and stack.
> + * x86-64 ABI, i.e. cannot be called from standard C code. As noted above,
> + * input parameters must be passed via ``%eax``, ``8(%rsp)``, ``0x10(%rsp)`` and
> + * ``0x18(%rsp)``, with the return value passed via ``%eax``. All other registers
> + * will be passed through to the enclave as is. All registers except ``%rbp``
> + * must be treated as volatile from the caller's perspective, including but not
> + * limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... Conversely, the enclave being
> + * run **must** preserve the untrusted ``%rbp``.
> + *
> + * ``callback`` has the following signature:
> + * int callback(long rdi, long rsi, long rdx,
> + * struct sgx_enclave_exinfo *ex_info, long r8, long r9,
> + * void *tcs, long ursp);
> + * ``callback`` **shall** follow x86_64 ABI. All GPRs **except** ``%rax``, ``%rbx``
> + * and ``rcx`` are passed through to ``callback``. ``%rdi``, ``%rsi``, ``%rdx``,
> + * ``%r8``, ``%r9``, along with the value of ``%rsp`` when the enclave exited/excepted,
> + * can be accessed directly as input parameters, while other GPRs can be
> + * accessed in assembly if needed.
> + * A positive value returned from ``callback`` will be treated as an ENCLU leaf
> + * (e.g. EENTER/ERESUME) to reenter the enclave, while 0 or a negative return
> + * value will be passed back to the caller of __vdso_sgx_enter_enclave().
> + * It is also **safe** to ``longjmp()`` out of ``callback``.
> */
> -__vdso_sgx_enter_enclave(u32 leaf, void *tcs,
> - struct sgx_enclave_exception *ex_info)
> -{
> - if (leaf != SGX_EENTER && leaf != SGX_ERESUME)
> - return -EINVAL;
> -
> - if (!tcs)
> - return -EINVAL;
> -
> - try {
> - ENCLU[leaf];
> - } catch (exception) {
> - if (e)
> - *e = exception;
> - return -EFAULT;
> - }
> -
> - return 0;
> -}
> -#endif