Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 23 Apr 2019 12:26:54 -0700
From:   Sean Christopherson <sean.j.christopherson@intel.com>
To:     Cedric Xing <cedric.xing@intel.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
        Hansen@linux.intel.com, Dave <dave.hansen@intel.com>,
        Christopherson@linux.intel.com, nhorman@redhat.com,
        npmccallum@redhat.com, Ayoun@linux.intel.com,
        Serge <serge.ayoun@intel.com>, Katz-zamir@linux.intel.com,
        Shay <shay.katz-zamir@intel.com>, Huang@linux.intel.com,
        Haitao <haitao.huang@intel.com>,
        andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
        Svahn@linux.intel.com, Kai <kai.svahn@intel.com>, bp@alien8.de,
        josh@joshtriplett.org, luto@kernel.org, Kai <kai.huang@intel.com>,
        rientjes@google.com,
        Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Subject: Re: [RFC PATCH v1 2/3] x86/vdso: Modify __vdso_sgx_enter_enclave()
 to allow parameter passing on untrusted stack
Message-ID: <20190423192654.GA12691@linux.intel.com>
References: <20190417103938.7762-1-jarkko.sakkinen@linux.intel.com>
 <cover.1555965327.git.cedric.xing@intel.com>
 <ba2a51568f3adaf74994d33ea3cbee570e20c6f6.1555965327.git.cedric.xing@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ba2a51568f3adaf74994d33ea3cbee570e20c6f6.1555965327.git.cedric.xing@intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@intel.com>
> ---
>  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