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

Commit Message

Xing, Cedric July 11, 2019, 4:21 a.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) on the untrusted stack.

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 revised 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 | 214 ++++++++++++++++-------
 arch/x86/include/uapi/asm/sgx.h          |  14 +-
 2 files changed, 157 insertions(+), 71 deletions(-)

Comments

Jarkko Sakkinen July 11, 2019, 9:50 a.m. UTC | #1

On Wed, Jul 10, 2019 at 09:21:32PM -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) on the untrusted stack.
> 
> 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 revised 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 | 214 ++++++++++++++++-------
>  arch/x86/include/uapi/asm/sgx.h          |  14 +-
>  2 files changed, 157 insertions(+), 71 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index fe0bf6671d6d..62f28c01b3c8 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -14,88 +14,174 @@
>  .code64
>  .section .text, "ax"
>  
> -#ifdef SGX_KERNEL_DOC
>  /**
>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
>   *

This was already there but here should not be empty line.

> - * @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

Does not align with kdoc standards.

* @leaf:	ENCLU leaf, must be EENTER or ERESUME
* @tcs:		TCS, must be non-NULL
* @ex_info:	Optional 'struct sgx_enclave_exception' pointer



> + * Parameters:
> + *	@leaf, passed in %eax, must be either EENTER(2) or ERESUME(3)
> + *	@tcs, passed on stack at 8(%rsp), is the linear address of TCS
> + *	@exinfo, passed on stack at 0x10(%rsp), is optional, and if non-NULL,
> + *	shall point to an sgx_enclave_exinfo structure to receive information
> + *	about the enclave exit
> + *	@callback, passed on stack at 0x18(%rsp), is optiona, and if non-NULL,
> + *	points to a callback function that will be invoked after the enclave
> + *	exits
>   *
> - * 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
> + * Returns:
> + *	$0 (zero) on a clean exit from the enclave
> + *	$-EINVAL will be returned if leaf isn't either EENTER or ERESUME
> + *	Other negative values could also be returned as the return value from
> + *	the callback function
>   *
> - * **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.
> + * IMPORTANT! This API is **not** compliant with x86-64 ABI but adopts a
> + * proprietary calling convention, described below:
> + *   · As noted above, input parameters are passed via %eax and the stack.
> + *   · The return value is passed via %eax.
> + *   · %rbx and %rcx must be treated as volatile as they are modified as part
> + *     of enclaves transitions and are used as scratch regs.
> + *   · %rdx, %rdi, %rsi and %r8-%r15 are passed as is and may be freely
> + *     modified by the enclave. Values left in those registers will not be
> + *     altered either, so will be visiable to the callback or the caller (if no
> + *     callback is specified).
> + *   · %rsp is saved/restored across __vdso_sgx_enter_enclave().
> + *
> + * A callback function, if supplied, shall have the following signature:
> + *
> + *	int callback(long rdi, long rsi, long rdx,
> + *		     struct sgx_enclave_exinfo *exinfo, long r8, long r9,
> + *		     void *tcs, long ursp);
> + *
> + * Callback functions shall comply to x86_64 ABI.
> + *   · All registers left off by the enclave are passed as is except %rax, %rbx
> + *     and %rcx. %rdi, %rsi, %r8 and %9 could be accessed as function
> + *     parameters, while other registers could be access in assembly code if
> + *     needed.
> + *   · Positive return values from the callback will be interpreted as ENCLU
> + *     leafs to re-enter the enclave. Currently only EENTER(2) and ERESUME(3)
> + *     are supported, while all other positive return values will result in
> + *     $-EINVAL returned to the caller of __vdso_sgx_enter_enclave().
> + *   · $0 (zero) or negative return values will be passed back to the caller of
> + *     __vdso_sgx_enter_enclave() as is.
> + *
> + * Pseudo-code:
> + *
> + * typedef int (*sgx_callback)(long rdi, long rsi, long rdx,
> + *			       struct sgx_enclave_exinfo *exinfo, long r8,
> + *			       long r9, void *tcs, long ursp);
> + *
> + * int __vdso_sgx_enter_enclave(int leaf, void *tcs,
> + *				struct sgx_enclave_exinfo *exinfo,
> + *				sgx_callback callback)
> + * {
> + *	while (leaf == EENTER || leaf == ERESUME) {
> + *		int rc;
> + *		try {
> + *			ENCLU[leaf];
> + *			rc = 0;
> + *			if (exinfo)
> + *				exinfo->leaf = EEXIT;
> + *		} catch (exception) {
> + *			rc = -EFAULT;
> + *			if (exinfo)
> + *				*exinfo = exception;
> + *		}
> + *
> + *		leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
> + *						   r8, r9, tcs, ursp);
> + *	}
> + *
> + *	return leaf > 0 ? -EINVAL : leaf;
> + * }
>   */

Please remove all of this documentation  or write more punctual
documentation and follow the standard:

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

Not going to maintain the above. Rather do not add documentation
at all than the above.

> -__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
> +	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> +2:	enclu
>  
> -	/* Load AEP for ENCLU */
> -	lea	1f(%rip),  %rcx
> -1:	enclu
> +	/* EEXIT path */
> +	xor	%ebx, %ebx
> +3:	mov	0x18(%rbp), %rcx
> +	jrcxz	4f
> +	mov	%eax, EX_LEAF(%rcx)
> +	jnc	4f
> +	mov	%di, EX_TRAPNR(%rcx)
> +	mov	%si, EX_ERROR_CODE(%rcx)
> +	mov	%rdx, EX_ADDRESS(%rcx)
>  
> -	add	$0x8, %rsp
> -	xor	%eax, %eax
> +4:	/* Call *callback if supplied */
> +	mov	0x20(%rbp), %rax
> +	test	%rax, %rax
> +	/*
> +	 * At this point, %ebx holds the effective return value, which shall be
> +	 * returned if no callback is specified
> +	 */
> +	cmovz	%rbx, %rax
> +	jz	7f
> +	/*
> +	 * Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
> +	 * restored after *callback returns.
> +	 */
> +	mov	%rsp, %rbx
> +	and	$-0x10, %rsp
> +	/* Clear RFLAGS.DF per x86_64 ABI */
> +	cld
> +	/* Parameters for *callback */
> +	push	%rbx
> +	push	0x10(%rbp)
> +	/* Call *%rax via retpoline */
> +	call	40f
> +	/*
> +	 * Restore %rsp to its original value left off by the enclave from last
> +	 * exit
> +	 */
> +	mov	%rbx, %rsp
> +	/*
> +	 * Positive return value from *callback will be interpreted as an ENCLU
> +	 * leaf, while a non-positive value will be interpreted as the return
> +	 * value to be passed back to the caller.
> +	 */
> +	jmp	1b
> +40:	/* retpoline */
> +	call	42f
> +41:	pause
> +	lfence
> +	jmp	41b
> +42:	mov	%rax, (%rsp)
>  	ret
>  
> -bad_input:
> -	mov     $(-EINVAL), %rax
> -	ret
> +5:	/* Exception path */
> +	mov	$-EFAULT, %ebx
> +	stc
> +	jmp	3b
>  
> -.pushsection .fixup, "ax"
> -	/* Re-load @exception_info and fill it (if it's non-NULL) */
> -2:	pop	%rcx
> -	test    %rcx, %rcx
> -	je      3f
> +6:	/* Unsupported ENCLU leaf */
> +	cmp	$0, %eax
> +	jle	7f
> +	mov	$-EINVAL, %eax
>  
> -	mov	%eax, EX_LEAF(%rcx)
> -	mov	%di,  EX_TRAPNR(%rcx)
> -	mov	%si,  EX_ERROR_CODE(%rcx)
> -	mov	%rdx, EX_ADDRESS(%rcx)
> -3:	mov	$(-EFAULT), %rax
> +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

You have bunch of these clutter diff's in your patch. Please get rid of
them.

>   * @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()

If you want to rename a struct it should be its own commit. Anyway, I'd
say that this unnecessary.

>   *
> - * @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;
> -- 
> 2.17.1
> 

Summary: I can live with the general idea but the patch itself is
somewhat half-finished still.

/Jarkko

Jarkko Sakkinen July 11, 2019, 9:53 a.m. UTC | #2

On Wed, Jul 10, 2019 at 09:21:32PM -0700, Cedric Xing wrote:
> -#ifdef SGX_KERNEL_DOC

Why is this removed?

> + * int __vdso_sgx_enter_enclave(int leaf, void *tcs,
> + *				struct sgx_enclave_exinfo *exinfo,
> + *				sgx_callback callback)
> + * {
> + *	while (leaf == EENTER || leaf == ERESUME) {
> + *		int rc;
> + *		try {
> + *			ENCLU[leaf];
> + *			rc = 0;
> + *			if (exinfo)
> + *				exinfo->leaf = EEXIT;
> + *		} catch (exception) {
> + *			rc = -EFAULT;
> + *			if (exinfo)
> + *				*exinfo = exception;
> + *		}
> + *
> + *		leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
> + *						   r8, r9, tcs, ursp);
> + *	}
> + *
> + *	return leaf > 0 ? -EINVAL : leaf;
> + * }
>   */

What is this? C++ and anyway there is already a source code. No need
to duplicate with pseudo-code. Only adds maintenance burde. Please get
rid of this.

/Jarkko

Sean Christopherson July 11, 2019, 3:42 p.m. UTC | #3

On Thu, Jul 11, 2019 at 12:53:17PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jul 10, 2019 at 09:21:32PM -0700, Cedric Xing wrote:
> > -#ifdef SGX_KERNEL_DOC
> 
> Why is this removed?
> 
> > + * int __vdso_sgx_enter_enclave(int leaf, void *tcs,
> > + *				struct sgx_enclave_exinfo *exinfo,
> > + *				sgx_callback callback)
> > + * {
> > + *	while (leaf == EENTER || leaf == ERESUME) {
> > + *		int rc;
> > + *		try {
> > + *			ENCLU[leaf];
> > + *			rc = 0;
> > + *			if (exinfo)
> > + *				exinfo->leaf = EEXIT;
> > + *		} catch (exception) {
> > + *			rc = -EFAULT;
> > + *			if (exinfo)
> > + *				*exinfo = exception;
> > + *		}
> > + *
> > + *		leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
> > + *						   r8, r9, tcs, ursp);
> > + *	}
> > + *
> > + *	return leaf > 0 ? -EINVAL : leaf;
> > + * }
> >   */
> 
> What is this? C++ and anyway there is already a source code. No need
> to duplicate with pseudo-code. Only adds maintenance burde. Please get
> rid of this.

Adding C pseudo-code was my idea, e.g. it already exists in v20.  Declaring
a psuedo-C function coerces kernel-doc into generating documentation for
the asm routine.  IIRC, fully defining the function is not required for
docs, but IMO it's significantly easier to gain an understanding of a blob
of asm if there is higher level pseudocode.

Jarkko Sakkinen July 11, 2019, 5:55 p.m. UTC | #4

On Thu, Jul 11, 2019 at 08:42:31AM -0700, Sean Christopherson wrote:
> On Thu, Jul 11, 2019 at 12:53:17PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jul 10, 2019 at 09:21:32PM -0700, Cedric Xing wrote:
> > > -#ifdef SGX_KERNEL_DOC
> > 
> > Why is this removed?
> > 
> > > + * int __vdso_sgx_enter_enclave(int leaf, void *tcs,
> > > + *				struct sgx_enclave_exinfo *exinfo,
> > > + *				sgx_callback callback)
> > > + * {
> > > + *	while (leaf == EENTER || leaf == ERESUME) {
> > > + *		int rc;
> > > + *		try {
> > > + *			ENCLU[leaf];
> > > + *			rc = 0;
> > > + *			if (exinfo)
> > > + *				exinfo->leaf = EEXIT;
> > > + *		} catch (exception) {
> > > + *			rc = -EFAULT;
> > > + *			if (exinfo)
> > > + *				*exinfo = exception;
> > > + *		}
> > > + *
> > > + *		leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
> > > + *						   r8, r9, tcs, ursp);
> > > + *	}
> > > + *
> > > + *	return leaf > 0 ? -EINVAL : leaf;
> > > + * }
> > >   */
> > 
> > What is this? C++ and anyway there is already a source code. No need
> > to duplicate with pseudo-code. Only adds maintenance burde. Please get
> > rid of this.
> 
> Adding C pseudo-code was my idea, e.g. it already exists in v20.  Declaring
> a psuedo-C function coerces kernel-doc into generating documentation for
> the asm routine.  IIRC, fully defining the function is not required for
> docs, but IMO it's significantly easier to gain an understanding of a blob
> of asm if there is higher level pseudocode.

The way to do this right would be to write a documentation block before
kdoc's for the functions. It is described in the last section of

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt0

I.e. organize the file as

1. Documentation block describing the theory of operation.
2. Functions and their associated documentations.

/Jarkko

Sean Christopherson July 11, 2019, 5:58 p.m. UTC | #5

On Thu, Jul 11, 2019 at 08:55:50PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 08:42:31AM -0700, Sean Christopherson wrote:
> > On Thu, Jul 11, 2019 at 12:53:17PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Jul 10, 2019 at 09:21:32PM -0700, Cedric Xing wrote:
> > > > -#ifdef SGX_KERNEL_DOC
> > > 
> > > Why is this removed?
> > > 
> > > > + * int __vdso_sgx_enter_enclave(int leaf, void *tcs,
> > > > + *				struct sgx_enclave_exinfo *exinfo,
> > > > + *				sgx_callback callback)
> > > > + * {
> > > > + *	while (leaf == EENTER || leaf == ERESUME) {
> > > > + *		int rc;
> > > > + *		try {
> > > > + *			ENCLU[leaf];
> > > > + *			rc = 0;
> > > > + *			if (exinfo)
> > > > + *				exinfo->leaf = EEXIT;
> > > > + *		} catch (exception) {
> > > > + *			rc = -EFAULT;
> > > > + *			if (exinfo)
> > > > + *				*exinfo = exception;
> > > > + *		}
> > > > + *
> > > > + *		leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
> > > > + *						   r8, r9, tcs, ursp);
> > > > + *	}
> > > > + *
> > > > + *	return leaf > 0 ? -EINVAL : leaf;
> > > > + * }
> > > >   */
> > > 
> > > What is this? C++ and anyway there is already a source code. No need
> > > to duplicate with pseudo-code. Only adds maintenance burde. Please get
> > > rid of this.
> > 
> > Adding C pseudo-code was my idea, e.g. it already exists in v20.  Declaring
> > a psuedo-C function coerces kernel-doc into generating documentation for
> > the asm routine.  IIRC, fully defining the function is not required for
> > docs, but IMO it's significantly easier to gain an understanding of a blob
> > of asm if there is higher level pseudocode.
> 
> The way to do this right would be to write a documentation block before
> kdoc's for the functions. It is described in the last section of
> 
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt0
> 
> I.e. organize the file as
> 
> 1. Documentation block describing the theory of operation.
> 2. Functions and their associated documentations.

The kernel doc parser straight up doesn't work on asm functions, hence the
shenanigans to coerce it into thinking it's parsing a normal C function.

Jarkko Sakkinen July 12, 2019, 3:16 a.m. UTC | #6

On Thu, Jul 11, 2019 at 10:58:13AM -0700, Sean Christopherson wrote:
> > The way to do this right would be to write a documentation block before
> > kdoc's for the functions. It is described in the last section of
> > 
> > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt0
> > 
> > I.e. organize the file as
> > 
> > 1. Documentation block describing the theory of operation.
> > 2. Functions and their associated documentations.
> 
> The kernel doc parser straight up doesn't work on asm functions, hence the
> shenanigans to coerce it into thinking it's parsing a normal C function.

Aah. I think I was too hazy with my comment. Looked it from patchwork and
the documentation is mostly just fine.

For this patch this cruft nees to be removed:

- *                           %SGX_IOC_ENCLAVE_INIT ioctl
+ *			     %SGX_IOC_ENCLAVE_INIT ioctl

These make squashing the patch properly nasty.

Also there is one unwanted rename. Did not find anything else obviously
wrong.

/Jarkko

Xing, Cedric July 13, 2019, 7 a.m. UTC | #7

On 7/11/2019 8:16 PM, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 10:58:13AM -0700, Sean Christopherson wrote:
>>> The way to do this right would be to write a documentation block before
>>> kdoc's for the functions. It is described in the last section of
>>>
>>> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt0
>>>
>>> I.e. organize the file as
>>>
>>> 1. Documentation block describing the theory of operation.
>>> 2. Functions and their associated documentations.
>>
>> The kernel doc parser straight up doesn't work on asm functions, hence the
>> shenanigans to coerce it into thinking it's parsing a normal C function.
> 
> Aah. I think I was too hazy with my comment. Looked it from patchwork and
> the documentation is mostly just fine.
> 
> For this patch this cruft nees to be removed:
> 
> - *                           %SGX_IOC_ENCLAVE_INIT ioctl
> + *			     %SGX_IOC_ENCLAVE_INIT ioctl
> 
> These make squashing the patch properly nasty.
> 
> Also there is one unwanted rename. Did not find anything else obviously
> wrong.

Reformatted the comments. Tested with kernel-doc. Now v4 could be 
converted and displayed nicely as man pages.

> /Jarkko
>

Patch

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index fe0bf6671d6d..62f28c01b3c8 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -14,88 +14,174 @@ 
 .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
+ * Parameters:
+ *	@leaf, passed in %eax, must be either EENTER(2) or ERESUME(3)
+ *	@tcs, passed on stack at 8(%rsp), is the linear address of TCS
+ *	@exinfo, passed on stack at 0x10(%rsp), is optional, and if non-NULL,
+ *	shall point to an sgx_enclave_exinfo structure to receive information
+ *	about the enclave exit
+ *	@callback, passed on stack at 0x18(%rsp), is optiona, and if non-NULL,
+ *	points to a callback function that will be invoked after the enclave
+ *	exits
  *
- * 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
+ * Returns:
+ *	$0 (zero) on a clean exit from the enclave
+ *	$-EINVAL will be returned if leaf isn't either EENTER or ERESUME
+ *	Other negative values could also be returned as the return value from
+ *	the callback function
  *
- * **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.
+ * IMPORTANT! This API is **not** compliant with x86-64 ABI but adopts a
+ * proprietary calling convention, described below:
+ *   · As noted above, input parameters are passed via %eax and the stack.
+ *   · The return value is passed via %eax.
+ *   · %rbx and %rcx must be treated as volatile as they are modified as part
+ *     of enclaves transitions and are used as scratch regs.
+ *   · %rdx, %rdi, %rsi and %r8-%r15 are passed as is and may be freely
+ *     modified by the enclave. Values left in those registers will not be
+ *     altered either, so will be visiable to the callback or the caller (if no
+ *     callback is specified).
+ *   · %rsp is saved/restored across __vdso_sgx_enter_enclave().
+ *
+ * A callback function, if supplied, shall have the following signature:
+ *
+ *	int callback(long rdi, long rsi, long rdx,
+ *		     struct sgx_enclave_exinfo *exinfo, long r8, long r9,
+ *		     void *tcs, long ursp);
+ *
+ * Callback functions shall comply to x86_64 ABI.
+ *   · All registers left off by the enclave are passed as is except %rax, %rbx
+ *     and %rcx. %rdi, %rsi, %r8 and %9 could be accessed as function
+ *     parameters, while other registers could be access in assembly code if
+ *     needed.
+ *   · Positive return values from the callback will be interpreted as ENCLU
+ *     leafs to re-enter the enclave. Currently only EENTER(2) and ERESUME(3)
+ *     are supported, while all other positive return values will result in
+ *     $-EINVAL returned to the caller of __vdso_sgx_enter_enclave().
+ *   · $0 (zero) or negative return values will be passed back to the caller of
+ *     __vdso_sgx_enter_enclave() as is.
+ *
+ * Pseudo-code:
+ *
+ * typedef int (*sgx_callback)(long rdi, long rsi, long rdx,
+ *			       struct sgx_enclave_exinfo *exinfo, long r8,
+ *			       long r9, void *tcs, long ursp);
+ *
+ * int __vdso_sgx_enter_enclave(int leaf, void *tcs,
+ *				struct sgx_enclave_exinfo *exinfo,
+ *				sgx_callback callback)
+ * {
+ *	while (leaf == EENTER || leaf == ERESUME) {
+ *		int rc;
+ *		try {
+ *			ENCLU[leaf];
+ *			rc = 0;
+ *			if (exinfo)
+ *				exinfo->leaf = EEXIT;
+ *		} catch (exception) {
+ *			rc = -EFAULT;
+ *			if (exinfo)
+ *				*exinfo = exception;
+ *		}
+ *
+ *		leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
+ *						   r8, r9, tcs, ursp);
+ *	}
+ *
+ *	return leaf > 0 ? -EINVAL : leaf;
+ * }
  */
-__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
+	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
+2:	enclu
 
-	/* Load AEP for ENCLU */
-	lea	1f(%rip),  %rcx
-1:	enclu
+	/* EEXIT path */
+	xor	%ebx, %ebx
+3:	mov	0x18(%rbp), %rcx
+	jrcxz	4f
+	mov	%eax, EX_LEAF(%rcx)
+	jnc	4f
+	mov	%di, EX_TRAPNR(%rcx)
+	mov	%si, EX_ERROR_CODE(%rcx)
+	mov	%rdx, EX_ADDRESS(%rcx)
 
-	add	$0x8, %rsp
-	xor	%eax, %eax
+4:	/* Call *callback if supplied */
+	mov	0x20(%rbp), %rax
+	test	%rax, %rax
+	/*
+	 * At this point, %ebx holds the effective return value, which shall be
+	 * returned if no callback is specified
+	 */
+	cmovz	%rbx, %rax
+	jz	7f
+	/*
+	 * Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
+	 * restored after *callback returns.
+	 */
+	mov	%rsp, %rbx
+	and	$-0x10, %rsp
+	/* Clear RFLAGS.DF per x86_64 ABI */
+	cld
+	/* Parameters for *callback */
+	push	%rbx
+	push	0x10(%rbp)
+	/* Call *%rax via retpoline */
+	call	40f
+	/*
+	 * Restore %rsp to its original value left off by the enclave from last
+	 * exit
+	 */
+	mov	%rbx, %rsp
+	/*
+	 * Positive return value from *callback will be interpreted as an ENCLU
+	 * leaf, while a non-positive value will be interpreted as the return
+	 * value to be passed back to the caller.
+	 */
+	jmp	1b
+40:	/* retpoline */
+	call	42f
+41:	pause
+	lfence
+	jmp	41b
+42:	mov	%rax, (%rsp)
 	ret
 
-bad_input:
-	mov     $(-EINVAL), %rax
-	ret
+5:	/* Exception path */
+	mov	$-EFAULT, %ebx
+	stc
+	jmp	3b
 
-.pushsection .fixup, "ax"
-	/* Re-load @exception_info and fill it (if it's non-NULL) */
-2:	pop	%rcx
-	test    %rcx, %rcx
-	je      3f
+6:	/* Unsupported ENCLU leaf */
+	cmp	$0, %eax
+	jle	7f
+	mov	$-EINVAL, %eax
 
-	mov	%eax, EX_LEAF(%rcx)
-	mov	%di,  EX_TRAPNR(%rcx)
-	mov	%si,  EX_ERROR_CODE(%rcx)
-	mov	%rdx, EX_ADDRESS(%rcx)
-3:	mov	$(-EFAULT), %rax
+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;