| Message ID | 20200818042405.12871-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vdso: x86/sgx: Rework SGX vDSO API | expand |
On Mon, Aug 17, 2020 at 09:24:03PM -0700, Sean Christopherson wrote: > Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and > output params. In the new struct, add an opaque "user_data" that can be > used to pass context across the vDSO, and an explicit "exit_reason" to > avoid overloading the return value. > > Moving the params into a struct will also make it less painful to use > dedicated exit reasons, and to support exiting on interrupts in future > patches. > > Cc: Nathaniel McCallum <npmccallum@redhat.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++------- > arch/x86/include/uapi/asm/sgx.h | 90 ++++++++++++++++-------- > 2 files changed, 107 insertions(+), 55 deletions(-) > > diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S > index 2d88acd408d4e..aaae6d6e28ac3 100644 > --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S > +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S > @@ -7,9 +7,21 @@ > > #include "extable.h" > > -#define EX_LEAF 0*8 > -#define EX_TRAPNR 0*8+4 > -#define EX_ERROR_CODE 0*8+6 > +/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */ > +#define RUN_OFFSET 2*8 Some better name please. > + > +/* Offsets into 'struct sgx_enter_enclave'. */ > +#define TCS_OFFEST 0*8 > +#define FLAGS_OFFSET 1*8 > +#define EXIT_LEAF_OFFSET 2*8 > +#define EXIT_REASON_OFFSET 2*8 + 4 > +#define USER_HANDLER_OFFSET 3*8 > +/* #define USER_DATA_OFFSET 4*8 */ > +#define EXCEPTION_OFFSET 5*8 These non-prefixed constants make the code really stressing to read given the complexity of it. Please, just write them like SGX_ENTER_ENCLAVE_TCS and so forth. /Jarkko
Tested-by: Jethro Beekman <jethro@fortanix.com> -- Jethro Beekman | Fortanix On 2020-08-18 06:24, Sean Christopherson wrote: > Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and > output params. In the new struct, add an opaque "user_data" that can be > used to pass context across the vDSO, and an explicit "exit_reason" to > avoid overloading the return value. > > Moving the params into a struct will also make it less painful to use > dedicated exit reasons, and to support exiting on interrupts in future > patches. > > Cc: Nathaniel McCallum <npmccallum@redhat.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++------- > arch/x86/include/uapi/asm/sgx.h | 90 ++++++++++++++++-------- > 2 files changed, 107 insertions(+), 55 deletions(-) > > diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S > index 2d88acd408d4e..aaae6d6e28ac3 100644 > --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S > +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S > @@ -7,9 +7,21 @@ > > #include "extable.h" > > -#define EX_LEAF 0*8 > -#define EX_TRAPNR 0*8+4 > -#define EX_ERROR_CODE 0*8+6 > +/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */ > +#define RUN_OFFSET 2*8 > + > +/* Offsets into 'struct sgx_enter_enclave'. */ > +#define TCS_OFFEST 0*8 > +#define FLAGS_OFFSET 1*8 > +#define EXIT_LEAF_OFFSET 2*8 > +#define EXIT_REASON_OFFSET 2*8 + 4 > +#define USER_HANDLER_OFFSET 3*8 > +/* #define USER_DATA_OFFSET 4*8 */ > +#define EXCEPTION_OFFSET 5*8 > + > +/* Offsets into sgx_enter_enclave.exception. */ > +#define EX_TRAPNR 0*8 > +#define EX_ERROR_CODE 0*8+2 > #define EX_ADDRESS 1*8 > > .code64 > @@ -30,12 +42,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > .Lenter_enclave: > /* EENTER <= leaf <= ERESUME */ > cmp $EENTER, %eax > - jb .Linvalid_leaf > + jb .Linvalid_input > cmp $ERESUME, %eax > - ja .Linvalid_leaf > + ja .Linvalid_input > + > + mov RUN_OFFSET(%rbp), %rcx > + > + /* No flags are currently defined/supported. */ > + cmpq $0, FLAGS_OFFSET(%rcx) > + jne .Linvalid_input > > /* Load TCS and AEP */ > - mov 0x10(%rbp), %rbx > + mov TCS_OFFEST(%rcx), %rbx > lea .Lasync_exit_pointer(%rip), %rcx > > /* Single ENCLU serving as both EENTER and AEP (ERESUME) */ > @@ -44,13 +62,21 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > enclu > > /* EEXIT jumps here unless the enclave is doing something fancy. */ > - xor %eax, %eax > + mov RUN_OFFSET(%rbp), %rbx > + > + /* Set exit_reason. */ > + movl $0, EXIT_REASON_OFFSET(%rbx) > > /* Invoke userspace's exit handler if one was provided. */ > .Lhandle_exit: > - cmpq $0, 0x20(%rbp) > + mov %eax, EXIT_LEAF_OFFSET(%rbx) > + > + cmpq $0, USER_HANDLER_OFFSET(%rbx) > jne .Linvoke_userspace_handler > > + /* Success, in the sense that ENCLU was attempted. */ > + xor %eax, %eax > + > .Lout: > pop %rbx > leave > @@ -60,28 +86,28 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > /* The out-of-line code runs with the pre-leave stack frame. */ > .cfi_def_cfa %rbp, 16 > > -.Linvalid_leaf: > +.Linvalid_input: > mov $(-EINVAL), %eax > jmp .Lout > > .Lhandle_exception: > - mov 0x18(%rbp), %rcx > - test %rcx, %rcx > - je .Lskip_exception_info > + mov RUN_OFFSET(%rbp), %rbx > > - /* Fill optional exception info. */ > - mov %eax, EX_LEAF(%rcx) > - mov %di, EX_TRAPNR(%rcx) > - mov %si, EX_ERROR_CODE(%rcx) > - mov %rdx, EX_ADDRESS(%rcx) > -.Lskip_exception_info: > - mov $(-EFAULT), %eax > + /* Set the exit_reason and exception info. */ > + movl $(-EFAULT), EXIT_REASON_OFFSET(%rbx) > + > + mov %di, (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx) > + mov %si, (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx) > + mov %rdx, (EXCEPTION_OFFSET + EX_ADDRESS)(%rbx) > jmp .Lhandle_exit > > .Linvoke_userspace_handler: > /* Pass the untrusted RSP (at exit) to the callback via %rcx. */ > mov %rsp, %rcx > > + /* Save @e, %rbx is about to be clobbered. */ > + mov %rbx, %rax > + > /* Save the untrusted RSP offset in %rbx (non-volatile register). */ > mov %rsp, %rbx > and $0xf, %rbx > @@ -93,20 +119,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > and $-0x10, %rsp > push %rax > > - /* Push @e, the "return" value and @tcs as params to the callback. */ > - push 0x18(%rbp) > + /* Push @e as a param to the callback. */ > push %rax > - push 0x10(%rbp) > > /* Clear RFLAGS.DF per x86_64 ABI */ > cld > > /* Load the callback pointer to %rax and invoke it via retpoline. */ > - mov 0x20(%rbp), %rax > + mov USER_HANDLER_OFFSET(%rax), %rax > call .Lretpoline > > /* Undo the post-exit %rsp adjustment. */ > - lea 0x20(%rsp, %rbx), %rsp > + lea 0x10(%rsp, %rbx), %rsp > > /* > * If the return from callback is zero or negative, return immediately, > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h > index 3760e5d5dc0c7..d3b107aac279d 100644 > --- a/arch/x86/include/uapi/asm/sgx.h > +++ b/arch/x86/include/uapi/asm/sgx.h > @@ -74,6 +74,28 @@ struct sgx_enclave_set_attribute { > __u64 attribute_fd; > }; > > +struct sgx_enclave_run; > + > +/** > + * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by > + * __vdso_sgx_enter_enclave() > + * > + * @rdi: RDI at the time of enclave exit > + * @rsi: RSI at the time of enclave exit > + * @rdx: RDX at the time of enclave exit > + * @ursp: RSP at the time of enclave exit (untrusted stack) > + * @r8: R8 at the time of enclave exit > + * @r9: R9 at the time of enclave exit > + * @r: Pointer to struct sgx_enclave_run (as provided by caller) > + * > + * Return: > + * 0 or negative to exit vDSO > + * positive to re-enter enclave (must be EENTER or ERESUME leaf) > + */ > +typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, > + long ursp, long r8, long r9, > + struct sgx_enclave_run *r); > + > /** > * struct sgx_enclave_exception - structure to report exceptions encountered in > * __vdso_sgx_enter_enclave() > @@ -85,31 +107,43 @@ struct sgx_enclave_set_attribute { > * @reserved: reserved for future use > */ > struct sgx_enclave_exception { > - __u32 leaf; > __u16 trapnr; > __u16 error_code; > + __u32 reserved; > __u64 address; > - __u64 reserved[2]; > }; > > /** > - * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by > - * __vdso_sgx_enter_enclave() > + * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave() > * > - * @rdi: RDI at the time of enclave exit > - * @rsi: RSI at the time of enclave exit > - * @rdx: RDX at the time of enclave exit > - * @ursp: RSP at the time of enclave exit (untrusted stack) > - * @r8: R8 at the time of enclave exit > - * @r9: R9 at the time of enclave exit > - * @tcs: Thread Control Structure used to enter enclave > - * @ret: 0 on success (EEXIT), -EFAULT on an exception > - * @e: Pointer to struct sgx_enclave_exception (as provided by caller) > + * @tcs: Thread Control Structure used to enter enclave > + * @flags: Control flags > + * @exit_leaf: ENCLU leaf from \%eax at time of exit > + * @exit_reason: Cause of exit from enclave, e.g. EEXIT vs. exception > + * @user_handler: User provided exit handler (optional) > + * @user_data: User provided opaque value (optional) > + * @exception: Valid on exit due to exception > */ > -typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, > - long ursp, long r8, long r9, > - void *tcs, int ret, > - struct sgx_enclave_exception *e); > +struct sgx_enclave_run { > + __u64 tcs; > + __u64 flags; > + > + __u32 exit_leaf; > + __u32 exit_reason; > + > + union { > + sgx_enclave_exit_handler_t user_handler; > + __u64 __user_handler; > + }; > + __u64 user_data; > + > + union { > + struct sgx_enclave_exception exception; > + > + /* Pad the entire struct to 256 bytes. */ > + __u8 pad[256 - 40]; > + }; > +}; > > /** > * __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 */ >
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. -- Jethro Beekman | Fortanix
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. This documentation should be moved to Documentation/x86/sgx.rst. It is easier to read from there and then it will be included by kdoc to the kernel documentation. In here it is not addressed by kdoc and it is unnecessarily hard to read. > -- > Jethro Beekman | Fortanix /Jarkko
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).
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: -.Lskip_exception_info: - mov $(-EFAULT), %eax + /* Set the exit_reason and exception info. */ + movl $(-EFAULT), EXIT_REASON_OFFSET(%rbx) -- Jethro Beekman | Fortanix
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? > > -.Lskip_exception_info: > - mov $(-EFAULT), %eax > + /* Set the exit_reason and exception info. */ > + movl $(-EFAULT), EXIT_REASON_OFFSET(%rbx) > > -- > Jethro Beekman | Fortanix >
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. -- Jethro Beekman | Fortanix
On 8/17/2020 9:24 PM, Sean Christopherson wrote: > Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and > output params. In the new struct, add an opaque "user_data" that can be > used to pass context across the vDSO, and an explicit "exit_reason" to > avoid overloading the return value. > In order to pass additional parameters to the exit handler, the exinfo structure could be embedded in a user-defined structure while the handler could pick it up using "container_of" macro. IMO the original interface was neat and suffcient, and we are over-engineering it. > Moving the params into a struct will also make it less painful to use > dedicated exit reasons, and to support exiting on interrupts in future > patches. > My apology for not following discussion lately. What did you mean by "dedicated exit reasons" and why was it "painful"? According to another thread, I guess we wouldn't have "exiting on interrupts".
On Wed, Aug 26, 2020 at 12:27:54PM -0700, Xing, Cedric wrote: > On 8/17/2020 9:24 PM, Sean Christopherson wrote: > > Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and > > output params. In the new struct, add an opaque "user_data" that can be > > used to pass context across the vDSO, and an explicit "exit_reason" to > > avoid overloading the return value. > > In order to pass additional parameters to the exit handler, the exinfo > structure could be embedded in a user-defined structure while the handler > could pick it up using "container_of" macro. IMO the original interface was > neat and suffcient, and we are over-engineering it. container_of/offsetof shenanigans were my original suggestion as well. Nathaniel's argument is that using such tricks is less than pleasent in a Rust environment. https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com > > Moving the params into a struct will also make it less painful to use > > dedicated exit reasons, and to support exiting on interrupts in future > > patches. > > > My apology for not following discussion lately. What did you mean by > "dedicated exit reasons" and why was it "painful"? According to another > thread, I guess we wouldn't have "exiting on interrupts". Currently the vDSO returns -EFAULT if the enclave encounters an exception, which is kludgy for two reasons: 1. EFAULT traditionally means "bad address", whereas an exception could be a #UD in the enclave. 2. Returning -EFAULT provides weird semantics as it implies the vDSO call failed, which is not the case. The vDSO itself was successful, i.e. it did the requested EENTER/ERESUME operation, and should really return '0'. The proposed solution for #1 is to define arbitrary return values to differentiate between synchronous (EEXIT) exits and asynchronous exits due to exceptions. But that doesn't address #2, and gets especially awkward when dealing with the call back return, which is also overloaded to use positive return values for ENCLU leafs. Passing a "run" struct makes it trivially easy to different the return value of the vDSO function from the enclave exit reason, and to avoid collisions with the return value from the userspace callback.
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.
Andy, Any thoughts on using a struct versus a plethora of params? If you don't have a strong opinion, I'd like to push for the struct option as it fixes the -EFAULT weirdness, satisfies Nathaniel's request, and gives some flexibility for the future. The code impact, both to the vDSO and to the caller, is largely a lateral move, i.e. it's different but in the end doesn't require any more or less work. On Mon, Aug 17, 2020 at 09:24:03PM -0700, Sean Christopherson wrote: > Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and > output params. In the new struct, add an opaque "user_data" that can be > used to pass context across the vDSO, and an explicit "exit_reason" to > avoid overloading the return value. > > Moving the params into a struct will also make it less painful to use > dedicated exit reasons, and to support exiting on interrupts in future > patches. > > Cc: Nathaniel McCallum <npmccallum@redhat.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++------- > arch/x86/include/uapi/asm/sgx.h | 90 ++++++++++++++++-------- > 2 files changed, 107 insertions(+), 55 deletions(-) > > diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S > index 2d88acd408d4e..aaae6d6e28ac3 100644 > --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S > +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S > @@ -7,9 +7,21 @@ > > #include "extable.h" > > -#define EX_LEAF 0*8 > -#define EX_TRAPNR 0*8+4 > -#define EX_ERROR_CODE 0*8+6 > +/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */ > +#define RUN_OFFSET 2*8 > + > +/* Offsets into 'struct sgx_enter_enclave'. */ > +#define TCS_OFFEST 0*8 > +#define FLAGS_OFFSET 1*8 > +#define EXIT_LEAF_OFFSET 2*8 > +#define EXIT_REASON_OFFSET 2*8 + 4 > +#define USER_HANDLER_OFFSET 3*8 > +/* #define USER_DATA_OFFSET 4*8 */ > +#define EXCEPTION_OFFSET 5*8 > + > +/* Offsets into sgx_enter_enclave.exception. */ > +#define EX_TRAPNR 0*8 > +#define EX_ERROR_CODE 0*8+2 > #define EX_ADDRESS 1*8 > > .code64 > @@ -30,12 +42,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > .Lenter_enclave: > /* EENTER <= leaf <= ERESUME */ > cmp $EENTER, %eax > - jb .Linvalid_leaf > + jb .Linvalid_input > cmp $ERESUME, %eax > - ja .Linvalid_leaf > + ja .Linvalid_input > + > + mov RUN_OFFSET(%rbp), %rcx > + > + /* No flags are currently defined/supported. */ > + cmpq $0, FLAGS_OFFSET(%rcx) > + jne .Linvalid_input > > /* Load TCS and AEP */ > - mov 0x10(%rbp), %rbx > + mov TCS_OFFEST(%rcx), %rbx > lea .Lasync_exit_pointer(%rip), %rcx > > /* Single ENCLU serving as both EENTER and AEP (ERESUME) */ > @@ -44,13 +62,21 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > enclu > > /* EEXIT jumps here unless the enclave is doing something fancy. */ > - xor %eax, %eax > + mov RUN_OFFSET(%rbp), %rbx > + > + /* Set exit_reason. */ > + movl $0, EXIT_REASON_OFFSET(%rbx) > > /* Invoke userspace's exit handler if one was provided. */ > .Lhandle_exit: > - cmpq $0, 0x20(%rbp) > + mov %eax, EXIT_LEAF_OFFSET(%rbx) > + > + cmpq $0, USER_HANDLER_OFFSET(%rbx) > jne .Linvoke_userspace_handler > > + /* Success, in the sense that ENCLU was attempted. */ > + xor %eax, %eax > + > .Lout: > pop %rbx > leave > @@ -60,28 +86,28 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > /* The out-of-line code runs with the pre-leave stack frame. */ > .cfi_def_cfa %rbp, 16 > > -.Linvalid_leaf: > +.Linvalid_input: > mov $(-EINVAL), %eax > jmp .Lout > > .Lhandle_exception: > - mov 0x18(%rbp), %rcx > - test %rcx, %rcx > - je .Lskip_exception_info > + mov RUN_OFFSET(%rbp), %rbx > > - /* Fill optional exception info. */ > - mov %eax, EX_LEAF(%rcx) > - mov %di, EX_TRAPNR(%rcx) > - mov %si, EX_ERROR_CODE(%rcx) > - mov %rdx, EX_ADDRESS(%rcx) > -.Lskip_exception_info: > - mov $(-EFAULT), %eax > + /* Set the exit_reason and exception info. */ > + movl $(-EFAULT), EXIT_REASON_OFFSET(%rbx) > + > + mov %di, (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx) > + mov %si, (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx) > + mov %rdx, (EXCEPTION_OFFSET + EX_ADDRESS)(%rbx) > jmp .Lhandle_exit > > .Linvoke_userspace_handler: > /* Pass the untrusted RSP (at exit) to the callback via %rcx. */ > mov %rsp, %rcx > > + /* Save @e, %rbx is about to be clobbered. */ > + mov %rbx, %rax > + > /* Save the untrusted RSP offset in %rbx (non-volatile register). */ > mov %rsp, %rbx > and $0xf, %rbx > @@ -93,20 +119,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > and $-0x10, %rsp > push %rax > > - /* Push @e, the "return" value and @tcs as params to the callback. */ > - push 0x18(%rbp) > + /* Push @e as a param to the callback. */ > push %rax > - push 0x10(%rbp) > > /* Clear RFLAGS.DF per x86_64 ABI */ > cld > > /* Load the callback pointer to %rax and invoke it via retpoline. */ > - mov 0x20(%rbp), %rax > + mov USER_HANDLER_OFFSET(%rax), %rax > call .Lretpoline > > /* Undo the post-exit %rsp adjustment. */ > - lea 0x20(%rsp, %rbx), %rsp > + lea 0x10(%rsp, %rbx), %rsp > > /* > * If the return from callback is zero or negative, return immediately, > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h > index 3760e5d5dc0c7..d3b107aac279d 100644 > --- a/arch/x86/include/uapi/asm/sgx.h > +++ b/arch/x86/include/uapi/asm/sgx.h > @@ -74,6 +74,28 @@ struct sgx_enclave_set_attribute { > __u64 attribute_fd; > }; > > +struct sgx_enclave_run; > + > +/** > + * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by > + * __vdso_sgx_enter_enclave() > + * > + * @rdi: RDI at the time of enclave exit > + * @rsi: RSI at the time of enclave exit > + * @rdx: RDX at the time of enclave exit > + * @ursp: RSP at the time of enclave exit (untrusted stack) > + * @r8: R8 at the time of enclave exit > + * @r9: R9 at the time of enclave exit > + * @r: Pointer to struct sgx_enclave_run (as provided by caller) > + * > + * Return: > + * 0 or negative to exit vDSO > + * positive to re-enter enclave (must be EENTER or ERESUME leaf) > + */ > +typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, > + long ursp, long r8, long r9, > + struct sgx_enclave_run *r); > + > /** > * struct sgx_enclave_exception - structure to report exceptions encountered in > * __vdso_sgx_enter_enclave() > @@ -85,31 +107,43 @@ struct sgx_enclave_set_attribute { > * @reserved: reserved for future use > */ > struct sgx_enclave_exception { > - __u32 leaf; > __u16 trapnr; > __u16 error_code; > + __u32 reserved; > __u64 address; > - __u64 reserved[2]; > }; > > /** > - * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by > - * __vdso_sgx_enter_enclave() > + * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave() > * > - * @rdi: RDI at the time of enclave exit > - * @rsi: RSI at the time of enclave exit > - * @rdx: RDX at the time of enclave exit > - * @ursp: RSP at the time of enclave exit (untrusted stack) > - * @r8: R8 at the time of enclave exit > - * @r9: R9 at the time of enclave exit > - * @tcs: Thread Control Structure used to enter enclave > - * @ret: 0 on success (EEXIT), -EFAULT on an exception > - * @e: Pointer to struct sgx_enclave_exception (as provided by caller) > + * @tcs: Thread Control Structure used to enter enclave > + * @flags: Control flags > + * @exit_leaf: ENCLU leaf from \%eax at time of exit > + * @exit_reason: Cause of exit from enclave, e.g. EEXIT vs. exception > + * @user_handler: User provided exit handler (optional) > + * @user_data: User provided opaque value (optional) > + * @exception: Valid on exit due to exception > */ > -typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, > - long ursp, long r8, long r9, > - void *tcs, int ret, > - struct sgx_enclave_exception *e); > +struct sgx_enclave_run { > + __u64 tcs; > + __u64 flags; > + > + __u32 exit_leaf; > + __u32 exit_reason; > + > + union { > + sgx_enclave_exit_handler_t user_handler; > + __u64 __user_handler; > + }; > + __u64 user_data; > + > + union { > + struct sgx_enclave_exception exception; > + > + /* Pad the entire struct to 256 bytes. */ > + __u8 pad[256 - 40]; > + }; > +}; > > /** > * __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 */ > -- > 2.28.0 >
> On Aug 26, 2020, at 1:20 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > Andy, > > Any thoughts on using a struct versus a plethora of params? If you don't > have a strong opinion, I'd like to push for the struct option as it fixes > the -EFAULT weirdness, satisfies Nathaniel's request, and gives some > flexibility for the future. The code impact, both to the vDSO and to the > caller, is largely a lateral move, i.e. it's different but in the end > doesn't require any more or less work. I have no preference.
On 8/26/2020 1:15 PM, Sean Christopherson wrote: > On Wed, Aug 26, 2020 at 12:27:54PM -0700, Xing, Cedric wrote: >> On 8/17/2020 9:24 PM, Sean Christopherson wrote: >>> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and >>> output params. In the new struct, add an opaque "user_data" that can be >>> used to pass context across the vDSO, and an explicit "exit_reason" to >>> avoid overloading the return value. >>> In order to pass additional parameters to the exit handler, the exinfo >> structure could be embedded in a user-defined structure while the handler >> could pick it up using "container_of" macro. IMO the original interface was >> neat and suffcient, and we are over-engineering it. > > container_of/offsetof shenanigans were my original suggestion as well. > Nathaniel's argument is that using such tricks is less than pleasent in a > Rust environment. > > https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com > I don't know much about rust but there seem to be offsetof/container_of solutions in rust. Here's one: https://gist.github.com/resilar/baefbfbf0d733c69d70970d829938875. After countless discussions along this upstream journey, I thought we had agreed that simplicity was the priority. And for simplicity we were even willing to give up compliance to x86_64 ABI. Then how do you justify treating rust better than other programming languages? This looks a hard sell to me. >>> Moving the params into a struct will also make it less painful to use >>> dedicated exit reasons, and to support exiting on interrupts in future >>> patches. >>> >> My apology for not following discussion lately. What did you mean by >> "dedicated exit reasons" and why was it "painful"? According to another >> thread, I guess we wouldn't have "exiting on interrupts". > > Currently the vDSO returns -EFAULT if the enclave encounters an exception, > which is kludgy for two reasons: > > 1. EFAULT traditionally means "bad address", whereas an exception could be > a #UD in the enclave. > > 2. Returning -EFAULT provides weird semantics as it implies the vDSO call > failed, which is not the case. The vDSO itself was successful, i.e. it > did the requested EENTER/ERESUME operation, and should really return '0'. > EFAULT is descriptive enough for me. And more importantly detailed/specific info is always available in exinfo. IMO, the purpose of return code is for the caller/handler to branch on. If 2 cases share the same execution path, then there's no need to distinguish them. In the case of SGX, most (if not all) exceptions are handled in the same way - nested EENTER, then what's point of distinguishing them? Please note that detailed info is always available in exinfo to cover corner cases. Moreover, even the vDSO API itself cannot tell between faults at EENTER/ERESUME (the vDSO itself failes) vs. faults inside the enclave (vDSO succeeded but the enclave fails). I don't think you can be accurate without complicating the code significantly. > The proposed solution for #1 is to define arbitrary return values to > differentiate between synchronous (EEXIT) exits and asynchronous exits due > to exceptions. But that doesn't address #2, and gets especially awkward when > dealing with the call back return, which is also overloaded to use positive > return values for ENCLU leafs. > > Passing a "run" struct makes it trivially easy to different the return value > of the vDSO function from the enclave exit reason, and to avoid collisions > with the return value from the userspace callback. > When a callback is configured, the vDSO NEVER returns to caller directly - i.e. all return codes must be from the callback but NOT the vDSO. I don't see any collisions here.
On 2020-08-26 22:15, Sean Christopherson wrote: > On Wed, Aug 26, 2020 at 12:27:54PM -0700, Xing, Cedric wrote: >> On 8/17/2020 9:24 PM, Sean Christopherson wrote: >>> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and >>> output params. In the new struct, add an opaque "user_data" that can be >>> used to pass context across the vDSO, and an explicit "exit_reason" to >>> avoid overloading the return value. >>> In order to pass additional parameters to the exit handler, the exinfo >> structure could be embedded in a user-defined structure while the handler >> could pick it up using "container_of" macro. IMO the original interface was >> neat and suffcient, and we are over-engineering it. > > container_of/offsetof shenanigans were my original suggestion as well. > Nathaniel's argument is that using such tricks is less than pleasent in a > Rust environment. > > https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com Just for the record I'm a Rust user and I have nothing against a solution that requires computing field offsets. As others have mentioned, assembly doesn't have facilities for this but it hasn't been a problem there. -- Jethro Beekman | Fortanix
On Wed, Aug 26, 2020 at 01:20:29PM -0700, Sean Christopherson wrote: > Andy, > > Any thoughts on using a struct versus a plethora of params? If you don't > have a strong opinion, I'd like to push for the struct option as it fixes > the -EFAULT weirdness, satisfies Nathaniel's request, and gives some > flexibility for the future. The code impact, both to the vDSO and to the > caller, is largely a lateral move, i.e. it's different but in the end > doesn't require any more or less work. If we ended up ever considering eBPF idea that I throwed a while ago in the air, most likely the same struct could be use as the context for the BPF program and larger portion of the existing vDSO code would remain intact. I'm not trying gonzo market BPF. I'm just pointing out that a struct is a great vessel to travel between kernel and user space (e.g. a BPF program would be executed by the kernel by using the vDSO provided context struct). /Jarkko
On Wed, Aug 26, 2020 at 04:26:11PM -0700, Xing, Cedric wrote: > After countless discussions along this upstream journey, I thought we had > agreed that simplicity was the priority. And for simplicity we were even > willing to give up compliance to x86_64 ABI. Then how do you justify > treating rust better than other programming languages? This looks a hard > sell to me. If you view using a struct instead of a multiple params as a separate cost, adding a pass-through param to throw a bone to Rust adds zero complexity. The flags field has nothing to do with Rust whatsoever. As for the struct itself, I suppose you could argue it's gratuitous, but I don't buy that it adds complexity. The impact on the vDSO is something like four extra instructions, two of which are a CMP+Jcc to enforce the flags, and the others are the additional moves need to chase pointers through the struct. Unless someone really dislikes structs, there is no added complexity to the caller, it's just slightly different code, e.g. the selftest update for the struct variant actually removed code. > > > > Moving the params into a struct will also make it less painful to use > > > > dedicated exit reasons, and to support exiting on interrupts in future > > > > patches. > > > > > > > My apology for not following discussion lately. What did you mean by > > > "dedicated exit reasons" and why was it "painful"? According to another > > > thread, I guess we wouldn't have "exiting on interrupts". > > > > Currently the vDSO returns -EFAULT if the enclave encounters an exception, > > which is kludgy for two reasons: > > > > 1. EFAULT traditionally means "bad address", whereas an exception could be > > a #UD in the enclave. > > > > 2. Returning -EFAULT provides weird semantics as it implies the vDSO call > > failed, which is not the case. The vDSO itself was successful, i.e. it > > did the requested EENTER/ERESUME operation, and should really return '0'. > > > EFAULT is descriptive enough for me. And more importantly detailed/specific > info is always available in exinfo. > > IMO, the purpose of return code is for the caller/handler to branch on. If 2 > cases share the same execution path, then there's no need to distinguish > them. In the case of SGX, most (if not all) exceptions are handled in the > same way - nested EENTER, then what's point of distinguishing them? Please > note that detailed info is always available in exinfo to cover corner cases. The primary motiviation is to adhere to the general rules of the kernel, specifically that -EFAULT isn't intended as a catchall for exceptions. > Moreover, even the vDSO API itself cannot tell between faults at > EENTER/ERESUME (the vDSO itself failes) vs. faults inside the enclave (vDSO > succeeded but the enclave fails). I don't think you can be accurate without > complicating the code significantly. And I didn't even try. The proposed documentation explicitly states that "success" is defined as attempting ENCLU. > > The proposed solution for #1 is to define arbitrary return values to > > differentiate between synchronous (EEXIT) exits and asynchronous exits due > > to exceptions. But that doesn't address #2, and gets especially awkward when > > dealing with the call back return, which is also overloaded to use positive > > return values for ENCLU leafs. > > > > Passing a "run" struct makes it trivially easy to different the return value > > of the vDSO function from the enclave exit reason, and to avoid collisions > > with the return value from the userspace callback. > > > When a callback is configured, the vDSO NEVER returns to caller directly - > i.e. all return codes must be from the callback but NOT the vDSO. I don't > see any collisions here. Unless the user provides a bad leaf, in which case -EINVAL is returned. But that wasn't what I was talking about. If we want to use arbitrary values instead of hijacking -EFAULT (and possibly -EINTR), then we run into problems with choosing an arbitrary value that won't collide with the value returned from the callback to the _vDSO_. The callback essentially gets dibs on all negative values and 0 is off limits. That leaves positive values, but those are interpreted as the new ENCLU leaf by the vDSO. That means the vDSO can "return" a value to the callback that the callback can't safely reflect back to the vDSO. Using an exit reason side steps that by virtue of it not being the return value in either direction.
On Mon, Aug 24, 2020 at 10:49:41PM +0300, Jarkko Sakkinen 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. > > This documentation should be moved to Documentation/x86/sgx.rst. > > It is easier to read from there and then it will be included by kdoc > to the kernel documentation. In here it is not addressed by kdoc and > it is unnecessarily hard to read. I'm confused, this is all picked up by kernel-doc. What am I missing?
On Fri, Sep 04, 2020 at 03:25:07AM -0700, Sean Christopherson wrote: > On Mon, Aug 24, 2020 at 10:49:41PM +0300, Jarkko Sakkinen 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. > > > > This documentation should be moved to Documentation/x86/sgx.rst. > > > > It is easier to read from there and then it will be included by kdoc > > to the kernel documentation. In here it is not addressed by kdoc and > > it is unnecessarily hard to read. > > I'm confused, this is all picked up by kernel-doc. What am I missing? Right of course, it is in the .h file now. Sorry, it is me being blind. Was it before in the .S file? /Jarkko
On Fri, Sep 04, 2020 at 04:36:31PM +0300, Jarkko Sakkinen wrote: > On Fri, Sep 04, 2020 at 03:25:07AM -0700, Sean Christopherson wrote: > > On Mon, Aug 24, 2020 at 10:49:41PM +0300, Jarkko Sakkinen wrote: > > > This documentation should be moved to Documentation/x86/sgx.rst. > > > > > > It is easier to read from there and then it will be included by kdoc > > > to the kernel documentation. In here it is not addressed by kdoc and > > > it is unnecessarily hard to read. > > > > I'm confused, this is all picked up by kernel-doc. What am I missing? > > Right of course, it is in the .h file now. Sorry, it is me being blind. No worries. > Was it before in the .S file? Yeah, it was originally in the .S file with an #ifdef hack to trick kernel-doc into parsing the comment. It got moved to the .h when the vDSO was changed to be directly callable from C.
diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S index 2d88acd408d4e..aaae6d6e28ac3 100644 --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S @@ -7,9 +7,21 @@ #include "extable.h" -#define EX_LEAF 0*8 -#define EX_TRAPNR 0*8+4 -#define EX_ERROR_CODE 0*8+6 +/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */ +#define RUN_OFFSET 2*8 + +/* Offsets into 'struct sgx_enter_enclave'. */ +#define TCS_OFFEST 0*8 +#define FLAGS_OFFSET 1*8 +#define EXIT_LEAF_OFFSET 2*8 +#define EXIT_REASON_OFFSET 2*8 + 4 +#define USER_HANDLER_OFFSET 3*8 +/* #define USER_DATA_OFFSET 4*8 */ +#define EXCEPTION_OFFSET 5*8 + +/* Offsets into sgx_enter_enclave.exception. */ +#define EX_TRAPNR 0*8 +#define EX_ERROR_CODE 0*8+2 #define EX_ADDRESS 1*8 .code64 @@ -30,12 +42,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) .Lenter_enclave: /* EENTER <= leaf <= ERESUME */ cmp $EENTER, %eax - jb .Linvalid_leaf + jb .Linvalid_input cmp $ERESUME, %eax - ja .Linvalid_leaf + ja .Linvalid_input + + mov RUN_OFFSET(%rbp), %rcx + + /* No flags are currently defined/supported. */ + cmpq $0, FLAGS_OFFSET(%rcx) + jne .Linvalid_input /* Load TCS and AEP */ - mov 0x10(%rbp), %rbx + mov TCS_OFFEST(%rcx), %rbx lea .Lasync_exit_pointer(%rip), %rcx /* Single ENCLU serving as both EENTER and AEP (ERESUME) */ @@ -44,13 +62,21 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) enclu /* EEXIT jumps here unless the enclave is doing something fancy. */ - xor %eax, %eax + mov RUN_OFFSET(%rbp), %rbx + + /* Set exit_reason. */ + movl $0, EXIT_REASON_OFFSET(%rbx) /* Invoke userspace's exit handler if one was provided. */ .Lhandle_exit: - cmpq $0, 0x20(%rbp) + mov %eax, EXIT_LEAF_OFFSET(%rbx) + + cmpq $0, USER_HANDLER_OFFSET(%rbx) jne .Linvoke_userspace_handler + /* Success, in the sense that ENCLU was attempted. */ + xor %eax, %eax + .Lout: pop %rbx leave @@ -60,28 +86,28 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) /* The out-of-line code runs with the pre-leave stack frame. */ .cfi_def_cfa %rbp, 16 -.Linvalid_leaf: +.Linvalid_input: mov $(-EINVAL), %eax jmp .Lout .Lhandle_exception: - mov 0x18(%rbp), %rcx - test %rcx, %rcx - je .Lskip_exception_info + mov RUN_OFFSET(%rbp), %rbx - /* Fill optional exception info. */ - mov %eax, EX_LEAF(%rcx) - mov %di, EX_TRAPNR(%rcx) - mov %si, EX_ERROR_CODE(%rcx) - mov %rdx, EX_ADDRESS(%rcx) -.Lskip_exception_info: - mov $(-EFAULT), %eax + /* Set the exit_reason and exception info. */ + movl $(-EFAULT), EXIT_REASON_OFFSET(%rbx) + + mov %di, (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx) + mov %si, (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx) + mov %rdx, (EXCEPTION_OFFSET + EX_ADDRESS)(%rbx) jmp .Lhandle_exit .Linvoke_userspace_handler: /* Pass the untrusted RSP (at exit) to the callback via %rcx. */ mov %rsp, %rcx + /* Save @e, %rbx is about to be clobbered. */ + mov %rbx, %rax + /* Save the untrusted RSP offset in %rbx (non-volatile register). */ mov %rsp, %rbx and $0xf, %rbx @@ -93,20 +119,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) and $-0x10, %rsp push %rax - /* Push @e, the "return" value and @tcs as params to the callback. */ - push 0x18(%rbp) + /* Push @e as a param to the callback. */ push %rax - push 0x10(%rbp) /* Clear RFLAGS.DF per x86_64 ABI */ cld /* Load the callback pointer to %rax and invoke it via retpoline. */ - mov 0x20(%rbp), %rax + mov USER_HANDLER_OFFSET(%rax), %rax call .Lretpoline /* Undo the post-exit %rsp adjustment. */ - lea 0x20(%rsp, %rbx), %rsp + lea 0x10(%rsp, %rbx), %rsp /* * If the return from callback is zero or negative, return immediately, diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 3760e5d5dc0c7..d3b107aac279d 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -74,6 +74,28 @@ struct sgx_enclave_set_attribute { __u64 attribute_fd; }; +struct sgx_enclave_run; + +/** + * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by + * __vdso_sgx_enter_enclave() + * + * @rdi: RDI at the time of enclave exit + * @rsi: RSI at the time of enclave exit + * @rdx: RDX at the time of enclave exit + * @ursp: RSP at the time of enclave exit (untrusted stack) + * @r8: R8 at the time of enclave exit + * @r9: R9 at the time of enclave exit + * @r: Pointer to struct sgx_enclave_run (as provided by caller) + * + * Return: + * 0 or negative to exit vDSO + * positive to re-enter enclave (must be EENTER or ERESUME leaf) + */ +typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, + long ursp, long r8, long r9, + struct sgx_enclave_run *r); + /** * struct sgx_enclave_exception - structure to report exceptions encountered in * __vdso_sgx_enter_enclave() @@ -85,31 +107,43 @@ struct sgx_enclave_set_attribute { * @reserved: reserved for future use */ struct sgx_enclave_exception { - __u32 leaf; __u16 trapnr; __u16 error_code; + __u32 reserved; __u64 address; - __u64 reserved[2]; }; /** - * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by - * __vdso_sgx_enter_enclave() + * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave() * - * @rdi: RDI at the time of enclave exit - * @rsi: RSI at the time of enclave exit - * @rdx: RDX at the time of enclave exit - * @ursp: RSP at the time of enclave exit (untrusted stack) - * @r8: R8 at the time of enclave exit - * @r9: R9 at the time of enclave exit - * @tcs: Thread Control Structure used to enter enclave - * @ret: 0 on success (EEXIT), -EFAULT on an exception - * @e: Pointer to struct sgx_enclave_exception (as provided by caller) + * @tcs: Thread Control Structure used to enter enclave + * @flags: Control flags + * @exit_leaf: ENCLU leaf from \%eax at time of exit + * @exit_reason: Cause of exit from enclave, e.g. EEXIT vs. exception + * @user_handler: User provided exit handler (optional) + * @user_data: User provided opaque value (optional) + * @exception: Valid on exit due to exception */ -typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, - long ursp, long r8, long r9, - void *tcs, int ret, - struct sgx_enclave_exception *e); +struct sgx_enclave_run { + __u64 tcs; + __u64 flags; + + __u32 exit_leaf; + __u32 exit_reason; + + union { + sgx_enclave_exit_handler_t user_handler; + __u64 __user_handler; + }; + __u64 user_data; + + union { + struct sgx_enclave_exception exception; + + /* Pad the entire struct to 256 bytes. */ + __u8 pad[256 - 40]; + }; +}; /** * __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 */
Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and output params. In the new struct, add an opaque "user_data" that can be used to pass context across the vDSO, and an explicit "exit_reason" to avoid overloading the return value. Moving the params into a struct will also make it less painful to use dedicated exit reasons, and to support exiting on interrupts in future patches. Cc: Nathaniel McCallum <npmccallum@redhat.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++------- arch/x86/include/uapi/asm/sgx.h | 90 ++++++++++++++++-------- 2 files changed, 107 insertions(+), 55 deletions(-)