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

Commit Message

Xing, Cedric April 22, 2019, 8:42 p.m. UTC

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 <[e-mail]>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 156 ++++++++++++++---------
 arch/x86/include/uapi/asm/sgx.h          |  14 +-
 2 files changed, 100 insertions(+), 70 deletions(-)

Comments

Sean Christopherson April 22, 2019, 10:26 p.m. UTC | #1

On Mon, Apr 22, 2019 at 01:42:58PM -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.

Please resend this series with the new code as a separate function, not a
modification to the existing function.  Andy made it pretty clear that he
prefers a separate vDSO function, and it'll be a lot easier to provide
feedback on the code if we're working from a clean slate.  If you want to
push to get everything lumped into a single function then by all means do
so on LKML, but for the RFC let's keep the focus on the code itself and
simplify life for reviewers.

  On Tue, Mar 26, 2019 at 10:08:31AM -0700, Andy Lutomirski wrote:
  > If the answer to both questions is yes, then it seems like it could be
  > reasonable to support it in the vDSO.  But I still think it should
  > probably be a different vDSO entry point so that the normal case
  > doesn't become more complicated.

> 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.

Again, wrap at 75 chars or earlier.  And incorporate checkpatch into your
git send-email flow if you haven't done so already, e.g. checkpatch should
have warned about lines longer than 75 chars.

> Signed-off-by: Cedric Xing <[e-mail]>
> ---

Andy Lutomirski April 23, 2019, 1:25 a.m. UTC | #2

On Mon, Apr 22, 2019 at 5:37 PM Cedric Xing <[e-mail]> 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 <[e-mail]>
> ---
>  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
>  /**
>   * __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

"to reenter the enclave without popping the extra data pushed by the
enclave off the stack" or similar.  We really don't want a situation
where someone puts all there "keep on going" logic in the callback and
the stack usage grows without bound.

> + * value will be passed back to the caller of __vdso_sgx_enter_enclave().
> + * It is also **safe** to ``longjmp()`` out of ``callback``.

I'm not sure that "safe" needs emphasis.

>   */
> -__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
>  ENTRY(__vdso_sgx_enter_enclave)
> -       /* EENTER <= leaf <= ERESUME */
> +       /* Prolog */
> +       .cfi_startproc
> +       push    %rbp
> +       .cfi_adjust_cfa_offset  8
> +       .cfi_rel_offset         %rbp, 0
> +       mov     %rsp, %rbp
> +       .cfi_def_cfa_register   %rbp
> +
> +1:     /* EENTER <= leaf <= ERESUME */
>         cmp     $0x2, %eax
> -       jb      bad_input
> -
> +       jb      6f
>         cmp     $0x3, %eax
> -       ja      bad_input
> +       ja      6f
>
> -       /* TCS must be non-NULL */
> -       test    %rbx, %rbx
> -       je      bad_input
> +       /* Load TCS and AEP */
> +       mov     0x10(%rbp), %rbx
> +       lea     2f(%rip), %rcx
>
> -       /* Save @exception_info */
> -       push    %rcx
> -
> -       /* Load AEP for ENCLU */
> -       lea     1f(%rip),  %rcx
> -1:     enclu
> -
> -       add     $0x8, %rsp
> -       xor     %eax, %eax
> -       ret
> -
> -bad_input:
> -       mov     $(-EINVAL), %rax
> -       ret
> -
> -.pushsection .fixup, "ax"
> -       /* Re-load @exception_info and fill it (if it's non-NULL) */
> -2:     pop     %rcx
> -       test    %rcx, %rcx
> -       je      3f
> +       /* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> +2:     enclu
>
> +       /* EEXIT path */
> +       xor     %ebx, %ebx
> +3:     mov     0x18(%rbp), %rcx
> +       jrcxz   4f
>         mov     %eax, EX_LEAF(%rcx)
> -       mov     %di,  EX_TRAPNR(%rcx)
> -       mov     %si,  EX_ERROR_CODE(%rcx)
> +       jnc     4f
> +       mov     %di, EX_TRAPNR(%rcx)
> +       mov     %si, EX_ERROR_CODE(%rcx)
>         mov     %rdx, EX_ADDRESS(%rcx)
> -3:     mov     $(-EFAULT), %rax
> +
> +4:     /* Call *callback if supplied */
> +       mov     0x20(%rbp), %rax
> +       test    %rax, %rax

Maybe have a comment like "At this point, the effective return value
is in RBX.  If there is no callback, then return it."

> +       cmovz   %rbx, %rax
> +       jz      7f
> +       /* Align stack and clear RFLAGS.DF per x86_64 ABI */
> +       mov     %rsp, %rbx

Whoa, this is too subtle here.  Can you update the comment to clarify
that the uRSP value set by the enclave needs to be saved so that the
enclave can be resumed if needed?

> +       and     $-0x10, %rsp
> +       cld
> +       /* Parameters for *callback */
> +       push    %rbx
> +       push    0x10(%rbp)
> +       /* Call via retpoline */
> +       call    40f
> +       /* Cleanup stack */
> +       mov     %rbx, %rsp

To me, "Cleanup stack" makes me think that you're restoring the
original RSP, but you're actually just undoing in the stack alignment.
How about "Undo stack alignment"?

But I'm not seeing the code that causes a return value RAX <= 0 to just return.

> +       jmp     1b
> +40:    /* retpoline */
> +       call    42f
> +41:    pause
> +       lfence
> +       jmp     41b
> +42:    mov     %rax, (%rsp)
> +       ret
> +
> +5:     /* Exception path */
> +       mov     $-EFAULT, %ebx
> +       stc
> +       jmp     3b
> +
> +6:     /* Unsupported ENCLU leaf */
> +       cmp     $0, %eax
> +       jle     7f
> +       mov     $-EINVAL, %eax
> +
> +7:     /* Epilog */
> +       leave
> +       .cfi_def_cfa            %rsp, 8
>         ret
> -.popsection
> +       .cfi_endproc
>
> -_ASM_VDSO_EXTABLE_HANDLE(1b, 2b)
> +_ASM_VDSO_EXTABLE_HANDLE(2b, 5b)

--Andy

Sean Christopherson April 23, 2019, 7:26 p.m. UTC | #3

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 <[e-mail]>
> ---
>  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

Andy Lutomirski April 23, 2019, 7:44 p.m. UTC | #4

> On Apr 23, 2019, at 12:26 PM, Sean Christopherson <[e-mail]> wrote:
> 
>> 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 <[e-mail]>
>> ---
>> 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.

That should be explained in a comment :)

—Andy

Xing, Cedric April 24, 2019, 5:56 p.m. UTC | #5

Hi Andy,

I sent out my RFC patch v2 last night, that has your suggestions incorporated, plus a new unwind test to single step through the vDSO API to test out the CFI directives. Hopefully it is able to address all of your concerns. It's worth noting that, given the current patch fixes up #DB and #BP at ENCLU, the unwind test cannot run to completion. I assume Sean will revise the fixup code soon.

Thanks!

-Cedric

> -----Original Message-----
> From: Andy Lutomirski [mailto:[e-mail]]
> Sent: Monday, April 22, 2019 6:26 PM
> To: Xing, Cedric <[e-mail]>
> Cc: LKML <[e-mail]>; X86 ML <[e-mail]>; linux-
> [e-mail]; Andrew Morton <[e-mail]>; Hansen,
> Dave <[e-mail]>; Christopherson, Sean J
> <[e-mail]>; [e-mail];
> [e-mail]; Ayoun, Serge <[e-mail]>; Katz-zamir,
> Shay <[e-mail]>; Huang, Haitao <[e-mail]>;
> Andy Shevchenko <[e-mail]>; Thomas Gleixner
> <[e-mail]>; Svahn, Kai <[e-mail]>; Borislav Petkov
> <[e-mail]>; Josh Triplett <[e-mail]>; Andrew Lutomirski
> <[e-mail]>; Huang, Kai <[e-mail]>; David Rientjes
> <[e-mail]>; Jarkko Sakkinen <[e-mail]>
> Subject: Re: [RFC PATCH v1 2/3] x86/vdso: Modify
> __vdso_sgx_enter_enclave() to allow parameter passing on untrusted stack
> 
> On Mon, Apr 22, 2019 at 5:37 PM Cedric Xing <[e-mail]>
> 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 <[e-mail]>
> > ---
> >  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
> >  /**
> >   * __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
> 
> "to reenter the enclave without popping the extra data pushed by the
> enclave off the stack" or similar.  We really don't want a situation
> where someone puts all there "keep on going" logic in the callback and
> the stack usage grows without bound.
> 
> > + * value will be passed back to the caller of
> __vdso_sgx_enter_enclave().
> > + * It is also **safe** to ``longjmp()`` out of ``callback``.
> 
> I'm not sure that "safe" needs emphasis.
> 
> >   */
> > -__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
> >  ENTRY(__vdso_sgx_enter_enclave)
> > -       /* EENTER <= leaf <= ERESUME */
> > +       /* Prolog */
> > +       .cfi_startproc
> > +       push    %rbp
> > +       .cfi_adjust_cfa_offset  8
> > +       .cfi_rel_offset         %rbp, 0
> > +       mov     %rsp, %rbp
> > +       .cfi_def_cfa_register   %rbp
> > +
> > +1:     /* EENTER <= leaf <= ERESUME */
> >         cmp     $0x2, %eax
> > -       jb      bad_input
> > -
> > +       jb      6f
> >         cmp     $0x3, %eax
> > -       ja      bad_input
> > +       ja      6f
> >
> > -       /* TCS must be non-NULL */
> > -       test    %rbx, %rbx
> > -       je      bad_input
> > +       /* Load TCS and AEP */
> > +       mov     0x10(%rbp), %rbx
> > +       lea     2f(%rip), %rcx
> >
> > -       /* Save @exception_info */
> > -       push    %rcx
> > -
> > -       /* Load AEP for ENCLU */
> > -       lea     1f(%rip),  %rcx
> > -1:     enclu
> > -
> > -       add     $0x8, %rsp
> > -       xor     %eax, %eax
> > -       ret
> > -
> > -bad_input:
> > -       mov     $(-EINVAL), %rax
> > -       ret
> > -
> > -.pushsection .fixup, "ax"
> > -       /* Re-load @exception_info and fill it (if it's non-NULL) */
> > -2:     pop     %rcx
> > -       test    %rcx, %rcx
> > -       je      3f
> > +       /* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> > +2:     enclu
> >
> > +       /* EEXIT path */
> > +       xor     %ebx, %ebx
> > +3:     mov     0x18(%rbp), %rcx
> > +       jrcxz   4f
> >         mov     %eax, EX_LEAF(%rcx)
> > -       mov     %di,  EX_TRAPNR(%rcx)
> > -       mov     %si,  EX_ERROR_CODE(%rcx)
> > +       jnc     4f
> > +       mov     %di, EX_TRAPNR(%rcx)
> > +       mov     %si, EX_ERROR_CODE(%rcx)
> >         mov     %rdx, EX_ADDRESS(%rcx)
> > -3:     mov     $(-EFAULT), %rax
> > +
> > +4:     /* Call *callback if supplied */
> > +       mov     0x20(%rbp), %rax
> > +       test    %rax, %rax
> 
> Maybe have a comment like "At this point, the effective return value is
> in RBX.  If there is no callback, then return it."
> 
> > +       cmovz   %rbx, %rax
> > +       jz      7f
> > +       /* Align stack and clear RFLAGS.DF per x86_64 ABI */
> > +       mov     %rsp, %rbx
> 
> Whoa, this is too subtle here.  Can you update the comment to clarify
> that the uRSP value set by the enclave needs to be saved so that the
> enclave can be resumed if needed?
> 
> > +       and     $-0x10, %rsp
> > +       cld
> > +       /* Parameters for *callback */
> > +       push    %rbx
> > +       push    0x10(%rbp)
> > +       /* Call via retpoline */
> > +       call    40f
> > +       /* Cleanup stack */
> > +       mov     %rbx, %rsp
> 
> To me, "Cleanup stack" makes me think that you're restoring the original
> RSP, but you're actually just undoing in the stack alignment.
> How about "Undo stack alignment"?
> 
> But I'm not seeing the code that causes a return value RAX <= 0 to just
> return.
> 
> > +       jmp     1b
> > +40:    /* retpoline */
> > +       call    42f
> > +41:    pause
> > +       lfence
> > +       jmp     41b
> > +42:    mov     %rax, (%rsp)
> > +       ret
> > +
> > +5:     /* Exception path */
> > +       mov     $-EFAULT, %ebx
> > +       stc
> > +       jmp     3b
> > +
> > +6:     /* Unsupported ENCLU leaf */
> > +       cmp     $0, %eax
> > +       jle     7f
> > +       mov     $-EINVAL, %eax
> > +
> > +7:     /* Epilog */
> > +       leave
> > +       .cfi_def_cfa            %rsp, 8
> >         ret
> > -.popsection
> > +       .cfi_endproc
> >
> > -_ASM_VDSO_EXTABLE_HANDLE(1b, 2b)
> > +_ASM_VDSO_EXTABLE_HANDLE(2b, 5b)
> 
> --Andy

Patch

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
 /**
  * __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
 ENTRY(__vdso_sgx_enter_enclave)
-	/* EENTER <= leaf <= ERESUME */
+	/* Prolog */
+	.cfi_startproc
+	push	%rbp
+	.cfi_adjust_cfa_offset	8
+	.cfi_rel_offset		%rbp, 0
+	mov	%rsp, %rbp
+	.cfi_def_cfa_register	%rbp
+
+1:	/* EENTER <= leaf <= ERESUME */
 	cmp	$0x2, %eax
-	jb	bad_input
-
+	jb	6f
 	cmp	$0x3, %eax
-	ja	bad_input
+	ja	6f
 
-	/* TCS must be non-NULL */
-	test	%rbx, %rbx
-	je	bad_input
+	/* Load TCS and AEP */
+	mov	0x10(%rbp), %rbx
+	lea	2f(%rip), %rcx
 
-	/* Save @exception_info */
-	push	%rcx
-
-	/* Load AEP for ENCLU */
-	lea	1f(%rip),  %rcx
-1:	enclu
-
-	add	$0x8, %rsp
-	xor	%eax, %eax
-	ret
-
-bad_input:
-	mov     $(-EINVAL), %rax
-	ret
-
-.pushsection .fixup, "ax"
-	/* Re-load @exception_info and fill it (if it's non-NULL) */
-2:	pop	%rcx
-	test    %rcx, %rcx
-	je      3f
+	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
+2:	enclu
 
+	/* EEXIT path */
+	xor	%ebx, %ebx
+3:	mov	0x18(%rbp), %rcx
+	jrcxz	4f
 	mov	%eax, EX_LEAF(%rcx)
-	mov	%di,  EX_TRAPNR(%rcx)
-	mov	%si,  EX_ERROR_CODE(%rcx)
+	jnc	4f
+	mov	%di, EX_TRAPNR(%rcx)
+	mov	%si, EX_ERROR_CODE(%rcx)
 	mov	%rdx, EX_ADDRESS(%rcx)
-3:	mov	$(-EFAULT), %rax
+
+4:	/* Call *callback if supplied */
+	mov	0x20(%rbp), %rax
+	test	%rax, %rax
+	cmovz	%rbx, %rax
+	jz	7f
+	/* Align stack and clear RFLAGS.DF per x86_64 ABI */
+	mov	%rsp, %rbx
+	and	$-0x10, %rsp
+	cld
+	/* Parameters for *callback */
+	push	%rbx
+	push	0x10(%rbp)
+	/* Call via retpoline */
+	call	40f
+	/* Cleanup stack */
+	mov	%rbx, %rsp
+	jmp	1b
+40:	/* retpoline */
+	call	42f
+41:	pause
+	lfence
+	jmp	41b
+42:	mov	%rax, (%rsp)
+	ret
+
+5:	/* Exception path */
+	mov	$-EFAULT, %ebx
+	stc
+	jmp	3b
+
+6:	/* Unsupported ENCLU leaf */
+	cmp	$0, %eax
+	jle	7f
+	mov	$-EINVAL, %eax
+
+7:	/* Epilog */
+	leave
+	.cfi_def_cfa		%rsp, 8
 	ret
-.popsection
+	.cfi_endproc
 
-_ASM_VDSO_EXTABLE_HANDLE(1b, 2b)
+_ASM_VDSO_EXTABLE_HANDLE(2b, 5b)
 
 ENDPROC(__vdso_sgx_enter_enclave)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 9ed690a38c70..50d2b5143e5e 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -24,7 +24,7 @@ 
 
 /**
  * struct sgx_enclave_create - parameter structure for the
- *                             %SGX_IOC_ENCLAVE_CREATE ioctl
+ *			       %SGX_IOC_ENCLAVE_CREATE ioctl
  * @src:	address for the SECS page data
  */
 struct sgx_enclave_create  {
@@ -33,7 +33,7 @@  struct sgx_enclave_create  {
 
 /**
  * struct sgx_enclave_add_page - parameter structure for the
- *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
+ *				 %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
  * @addr:	address within the ELRANGE
  * @src:	address for the page data
  * @secinfo:	address for the SECINFO data
@@ -49,7 +49,7 @@  struct sgx_enclave_add_page {
 
 /**
  * struct sgx_enclave_init - parameter structure for the
- *                           %SGX_IOC_ENCLAVE_INIT ioctl
+ *			     %SGX_IOC_ENCLAVE_INIT ioctl
  * @sigstruct:	address for the SIGSTRUCT data
  */
 struct sgx_enclave_init {
@@ -66,16 +66,16 @@  struct sgx_enclave_set_attribute {
 };
 
 /**
- * struct sgx_enclave_exception - structure to report exceptions encountered in
- *				  __vdso_sgx_enter_enclave()
+ * struct sgx_enclave_exinfo - structure to report exceptions encountered in
+ *			       __vdso_sgx_enter_enclave()
  *
- * @leaf:	ENCLU leaf from \%eax at time of exception
+ * @leaf:	ENCLU leaf from \%eax at time of exception/exit
  * @trapnr:	exception trap number, a.k.a. fault vector
  * @error_code:	exception error code
  * @address:	exception address, e.g. CR2 on a #PF
  * @reserved:	reserved for future use
  */
-struct sgx_enclave_exception {
+struct sgx_enclave_exinfo {
 	__u32 leaf;
 	__u16 trapnr;
 	__u16 error_code;