Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Aug 25, 2020 at 09:41:19AM +0200, Jethro Beekman wrote:
> On 2020-08-25 09:38, Sean Christopherson wrote:
> > On Tue, Aug 25, 2020 at 09:36:15AM +0200, Jethro Beekman wrote:
> >> On 2020-08-25 01:54, Sean Christopherson wrote:
> >>> On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
> >>>> On 2020-08-18 06:24, Sean Christopherson wrote:
> >>>>>  /**
> >>>>>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> >>>>> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >>>>>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> >>>>>   * @r8:		Pass-through value for R8
> >>>>>   * @r9:		Pass-through value for R9
> >>>>> - * @tcs:	TCS, must be non-NULL
> >>>>> - * @e:		Optional struct sgx_enclave_exception instance
> >>>>> - * @handler:	Optional enclave exit handler
> >>>>> + * @r:		struct sgx_enclave_run, must be non-NULL
> >>>>>   *
> >>>>>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> >>>>> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> >>>>> - * non-volatile general purpose registers, preserving/setting state in
> >>>>> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> >>>>> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> >>>>> - * without careful consideration by both the enclave and its runtime.
> >>>>> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> >>>>> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> >>>>> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> >>>>> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> >>>>> + * code without careful consideration by both the enclave and its runtime.
> >>>>>   *
> >>>>>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
> >>>>>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> >>>>> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >>>>>   * without returning to __vdso_sgx_enter_enclave().
> >>>>>   *
> >>>>>   * Return:
> >>>>> - *  0 on success,
> >>>>> + *  0 on success (ENCLU reached),
> >>>>>   *  -EINVAL if ENCLU leaf is not allowed,
> >>>>> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> >>>>> - *  -errno for all other negative values returned by the userspace exit handler
> >>>>>   */
> >>>>>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> >>>>>  					unsigned long rdx, unsigned int leaf,
> >>>>>  					unsigned long r8,  unsigned long r9,
> >>>>> -					void *tcs,
> >>>>> -					struct sgx_enclave_exception *e,
> >>>>> -					sgx_enclave_exit_handler_t handler);
> >>>>> +					struct sgx_enclave_run *r);
> >>>>>  
> >>>>>  #endif /* _UAPI_ASM_X86_SGX_H */
> >>>>>
> >>>>
> >>>> I think this should return void now, not int? Then, the “return” section of
> >>>> the documentation is also no longer correct.
> >>>
> >>> No, it returns -EINVAL if the leaf is bogus (and if unsupported flags are
> >>> specified, if a @flags param is ever added).
> >>>
> >>
> >> Ok, but if I read the code correctly, contrary to the docs, -EFAULT is not
> >> returned but passed in struct sgx_enclave_run:
> > 
> > The -EFAULT and -errno lines are removed from the "Return:" section of the
> > above documentation.  Did I miss one?
> > 
> 
> No, I'm just bad at reading patches without syntax highlighting. This is fine
> as is. Sorry for the confusion.

No worries.  Removing the -errno line is actually wrong, the vDSO still returns
-errno if the userspace callback returns a negative value.