| Message ID | 20201001161642.10500-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vdso: Flatten and clean up struct sgx_enclave_run | expand |
Can you split this into a proper series? There are many different things going on here, reviewing everything at once is difficult. On Thu, Oct 01, 2020 at 07:16:42PM +0300, Jarkko Sakkinen wrote: > Reduce the struct size to 64 bytes. It makes sense to have a naturally > aligning struct size and it leaves 24 bytes of spare space for possible > future expansion. Having 256 bytes is over the top without any reasonable > explanation, which does not exist in the commit for the SGX vDSO. I'm all for dropping the padding, but we should drop reserved fields altogether and instead reserve space for future features by keeping 'flags', enforcing it to be zero, and conditioning consumption of future fields on opt-in flags being set. > Drop struct sgx_exception. It does not serve any semantic purpose in the > context of this vDSO. Doing this is also consistent with user_handler and IMO it does serve a purpose, it helps convey that it's a package deal, i.e. either all fields are valid or none are valid. > user_data fields, as neither are they encapsulated in an embedded data > structure. > > Drop 'flags' from sgx_enclave_run as it is an unused value.'vsgx.S' > literally states this in an inline comment. It's not unused, it's reserved to zero. > Pick more reasonable names for the four fields of the exception. For > example, 'trapnr' should rather be just 'number' because in practice we > deliver only trap and the rest are faults. No, in Intel terminology both faults and traps are delivered. E.g. #DBs are traps (except for code breakpoints and INT1), #PF and #GP are faults. That being said, I'm all for changing the 'trapnr' name. I grabbed it from from the kernel's exception fixup, it wasn't deliberately chosen to imply that only trap-like exceptions are reported to userspace. > Also 'vector' would be an > adequate name for this field. Document the fields properly in order to > better explain the values that they have inherited. My vote is for 'vector'. > Remove 'exit_reason' as it is really a redundant field to save some space. > Instead, have just a field called 'leaf' that tells the last seen ENCLU. This justification contradicts the removal of flags. If you want to shave bytes, keep flags and enforce that flags is zero so that you don't need to have reserved bytes at the end of the struct. IMO, having a separate exit reason provides saner code for userspace, e.g. if (run->exit_reason == SGX_EXCEPTION_EXIT) <do exception stuff> versus if (run->leaf == EENTER || run->leaf == ERESUME) <do exception stuff> I would prefer to be explicit about why the vDSO is transferring control to the caller or its handler. It also gives us line of sight to supporting exit types other than EEXIT and exceptions. Maybe we never end up with another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of ending up with a mess of an API if another exit reason comes along. > The documentation, selftest and the kernel source code use the terms "user > handler" and "exit handler" in somewhat inconsistent manner. Start using > "AEX handler" consistently as this is the term that Intel SDM volume D uses > for these events. No, AEX is specifically for "asynchronus" exits. In this context, that is limited to the exception path. The vDSO invokes the user handler for EEXIT, i.e. the synchrous path, which is not an AEX.
On Thu, Oct 01, 2020 at 09:51:51AM -0700, Sean Christopherson wrote: > Can you split this into a proper series? There are many different things > going on here, reviewing everything at once is difficult. > > On Thu, Oct 01, 2020 at 07:16:42PM +0300, Jarkko Sakkinen wrote: > > Reduce the struct size to 64 bytes. It makes sense to have a naturally > > aligning struct size and it leaves 24 bytes of spare space for possible > > future expansion. Having 256 bytes is over the top without any reasonable > > explanation, which does not exist in the commit for the SGX vDSO. > > I'm all for dropping the padding, but we should drop reserved fields > altogether and instead reserve space for future features by keeping 'flags', > enforcing it to be zero, and conditioning consumption of future fields on > opt-in flags being set. I do not want to maintain ABI where structs change their size based on some field. It is way way too flakky. I fullly get some padding. I rather keep the padding as large as it was than do that. But you did catch a critical bug in my patch: the padding must be zero checked through, and return -EINVAL if not all bytes are zero. Then that space is a future playground. I scaled down to 64 bytes because it is the rounding to the next nicely alignable size (40 -> 64) and because three qwords is a lot of space to play for future expansion. 'flags' wastes qword of space if we end up not needing such field. With a good argument I can grow the struct to 128 or 256 bytes but it has to be a constant. Without argument nearest 2**n is the most logical choice. What I should add vsgx.S, is a zero validation of the reserved area. > > Drop struct sgx_exception. It does not serve any semantic purpose in the > > context of this vDSO. Doing this is also consistent with user_handler and > > IMO it does serve a purpose, it helps convey that it's a package deal, i.e. > either all fields are valid or none are valid. If there is no semantics that require such structure, then it is not important enough to exist. The 'leaf' will tell you whether you need to ignore exception fields or not. And not all fields are valid for all expections (e.g. the info put into stack). > > user_data fields, as neither are they encapsulated in an embedded data > > structure. > > > > Drop 'flags' from sgx_enclave_run as it is an unused value.'vsgx.S' > > literally states this in an inline comment. > > It's not unused, it's reserved to zero. My response above. > > Pick more reasonable names for the four fields of the exception. For > > example, 'trapnr' should rather be just 'number' because in practice we > > deliver only trap and the rest are faults. > > No, in Intel terminology both faults and traps are delivered. E.g. #DBs > are traps (except for code breakpoints and INT1), #PF and #GP are faults. > > That being said, I'm all for changing the 'trapnr' name. I grabbed it from > from the kernel's exception fixup, it wasn't deliberately chosen to imply > that only trap-like exceptions are reported to userspace. I based my information to Table 6-1 in Intel SDM volume 3A. Unfortunately, the quoted sentence above is missing one word, overflow, it is a typo. It should have been: "... we deliver only #OD trap and the rest are faults" The other traps, I think, were not fixed up but I could be wrong if I misread something from the table (was quite tired at that point). If I recheck that table in the SDM, the majority of stuff is categorized as "Fault". From this I deduced that trapnr is not aligned with the SDM terminology too well (being fully aware that kernel uses this internally). Anyway, I guess we go with 'vector', right? Probably there is some miunderstanding but with the name 'vector' we are in the same page :-) > > Also 'vector' would be an > > adequate name for this field. Document the fields properly in order to > > better explain the values that they have inherited. > > My vote is for 'vector'. > > > Remove 'exit_reason' as it is really a redundant field to save some space. > > Instead, have just a field called 'leaf' that tells the last seen ENCLU. > > This justification contradicts the removal of flags. If you want to shave > bytes, keep flags and enforce that flags is zero so that you don't need to > have reserved bytes at the end of the struct. > > IMO, having a separate exit reason provides saner code for userspace, e.g. > > if (run->exit_reason == SGX_EXCEPTION_EXIT) > <do exception stuff> > > versus > > if (run->leaf == EENTER || run->leaf == ERESUME) > <do exception stuff> > > I would prefer to be explicit about why the vDSO is transferring control to > the caller or its handler. It also gives us line of sight to supporting > exit types other than EEXIT and exceptions. Maybe we never end up with > another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of > ending up with a mess of an API if another exit reason comes along. 'exit_reason' is just waste of expensive memory space that we have limited fixed amount of. Using the same field in a *guaranteed* non-conflicting manner saves space. There is absolutely no question that what I did should not be done. And the semantics are intuitive: it will tell what the latest E-thing before arriving here (or subset of E's that transfrom from context to context). Maybe it should be prefixed with something like 'latest_', 'prev_' or perhaps 'last_', Not sure which would be the most adequate prefix. > > The documentation, selftest and the kernel source code use the terms "user > > handler" and "exit handler" in somewhat inconsistent manner. Start using > > "AEX handler" consistently as this is the term that Intel SDM volume D uses > > for these events. > > No, AEX is specifically for "asynchronus" exits. In this context, that is > limited to the exception path. The vDSO invokes the user handler for EEXIT, > i.e. the synchrous path, which is not an AEX. OK, I but this one. I got gray hairs because: - API called it user handler. - Documentation called it exit handler. - Selftest called it exit handler. I think user_handler is too generic and abstract but I'm happy to hear other proposals than 'exit_' to which I will change given your feedback. I think my flattening patch is doing the right thing overally. E.g. packing exception and eexit to leaf came obvious when I did it. It also makes doing all sorts of bindings to all sorts of languages a bit easier, especially if you generate the bindings. It's simply the right thing to do. I only did this patch because I really now concertrated what I'm seeing because of documenting things and came into conclusion that the existing API was such that I simply cannot commit to maintain for years ahead. /Jarkko
On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote: > On Thu, Oct 01, 2020 at 09:51:51AM -0700, Sean Christopherson wrote: > > > Remove 'exit_reason' as it is really a redundant field to save some space. > > > Instead, have just a field called 'leaf' that tells the last seen ENCLU. > > > > This justification contradicts the removal of flags. If you want to shave > > bytes, keep flags and enforce that flags is zero so that you don't need to > > have reserved bytes at the end of the struct. > > > > IMO, having a separate exit reason provides saner code for userspace, e.g. > > > > if (run->exit_reason == SGX_EXCEPTION_EXIT) > > <do exception stuff> > > > > versus > > > > if (run->leaf == EENTER || run->leaf == ERESUME) > > <do exception stuff> > > > > I would prefer to be explicit about why the vDSO is transferring control to > > the caller or its handler. It also gives us line of sight to supporting > > exit types other than EEXIT and exceptions. Maybe we never end up with > > another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of > > ending up with a mess of an API if another exit reason comes along. > > 'exit_reason' is just waste of expensive memory space that we have > limited fixed amount of. Huh? 4 bytes on the user stack can't possibly be a real concern, the default stack size on x86-64 is 8MB. Golang shaves that down to 2kb but dynamically adjusts the stack as needed. The "allocation" is completely free, the caller will already be adjusting RSP to create space for the struct, and it doesn't need to be initialized. > Using the same field in a *guaranteed* > non-conflicting manner saves space. There is absolutely no question > that what I did should not be done. But you can't guarantee that it doesn't conflict with future changes to the SGX architecture. Hell, you can't even guarantee that it doesn't conflict with the current SGX architecture, e.g. see Jethro's request to invoke the handler on AEX's due to interrupts.
On Thu, Oct 01, 2020 at 03:50:29PM -0700, Sean Christopherson wrote: > On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote: > > On Thu, Oct 01, 2020 at 09:51:51AM -0700, Sean Christopherson wrote: > > > > Remove 'exit_reason' as it is really a redundant field to save some space. > > > > Instead, have just a field called 'leaf' that tells the last seen ENCLU. > > > > > > This justification contradicts the removal of flags. If you want to shave > > > bytes, keep flags and enforce that flags is zero so that you don't need to > > > have reserved bytes at the end of the struct. > > > > > > IMO, having a separate exit reason provides saner code for userspace, e.g. > > > > > > if (run->exit_reason == SGX_EXCEPTION_EXIT) > > > <do exception stuff> > > > > > > versus > > > > > > if (run->leaf == EENTER || run->leaf == ERESUME) > > > <do exception stuff> > > > > > > I would prefer to be explicit about why the vDSO is transferring control to > > > the caller or its handler. It also gives us line of sight to supporting > > > exit types other than EEXIT and exceptions. Maybe we never end up with > > > another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of > > > ending up with a mess of an API if another exit reason comes along. > > > > 'exit_reason' is just waste of expensive memory space that we have > > limited fixed amount of. > > Huh? 4 bytes on the user stack can't possibly be a real concern, the default > stack size on x86-64 is 8MB. Golang shaves that down to 2kb but dynamically > adjusts the stack as needed. The "allocation" is completely free, the caller > will already be adjusting RSP to create space for the struct, and it doesn't > need to be initialized. It's unnecessary in the space that is chosen for the run structure. 64 bytes takes only one cacheline and the padding is also fast to validate to contain only zeros (check three qwords). Maybe I'm just weird but I'd just store all the leafs numbers in the field that is designated to store leaf numbers. > > Using the same field in a *guaranteed* > > non-conflicting manner saves space. There is absolutely no question > > that what I did should not be done. > > But you can't guarantee that it doesn't conflict with future changes to the > SGX architecture. Hell, you can't even guarantee that it doesn't conflict > with the current SGX architecture, e.g. see Jethro's request to invoke the > handler on AEX's due to interrupts. Reserved space can be used for any sort of new fields as long as the whole structure is validated, including the padding. With or without 'flags' this should be always done. Then there is no issue to add a new field after the merge. And thus, there is no issue resolving [*], even if the 'flags' is not added right now. You don't simply add stuff that is useless before it gets used. You ensure that your data is in stable state. That's all one hould do. [*] https://lore.kernel.org/linux-sgx/0f88f6d7-fa2d-59b0-c90f-4aa9e04d0af5@fortanix.com/ /Jarkko
On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote: > > > The documentation, selftest and the kernel source code use the terms "user > > > handler" and "exit handler" in somewhat inconsistent manner. Start using > > > "AEX handler" consistently as this is the term that Intel SDM volume D uses > > > for these events. > > > > No, AEX is specifically for "asynchronus" exits. In this context, that is > > limited to the exception path. The vDSO invokes the user handler for EEXIT, > > i.e. the synchrous path, which is not an AEX. > > OK, I but this one. > > I got gray hairs because: > > - API called it user handler. > - Documentation called it exit handler. > - Selftest called it exit handler. > > I think user_handler is too generic and abstract but I'm happy to hear > other proposals than 'exit_' to which I will change given your feedback. OK, so here I use my "too generic" argument pick user handler anyhow because we don't know what the heck the handler is doing. 'exit_handler' implies that not only it would be called for enclave exit resulting from an exception, but also for EEXIT path. /Jarkko
On Fri, Oct 02, 2020 at 07:25:05AM +0300, Jarkko Sakkinen wrote: > On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote: > > > > The documentation, selftest and the kernel source code use the terms "user > > > > handler" and "exit handler" in somewhat inconsistent manner. Start using > > > > "AEX handler" consistently as this is the term that Intel SDM volume D uses > > > > for these events. > > > > > > No, AEX is specifically for "asynchronus" exits. In this context, that is > > > limited to the exception path. The vDSO invokes the user handler for EEXIT, > > > i.e. the synchrous path, which is not an AEX. > > > > OK, I but this one. > > > > I got gray hairs because: > > > > - API called it user handler. > > - Documentation called it exit handler. > > - Selftest called it exit handler. > > > > I think user_handler is too generic and abstract but I'm happy to hear > > other proposals than 'exit_' to which I will change given your feedback. > > OK, so here I use my "too generic" argument pick user handler anyhow > because we don't know what the heck the handler is doing. > > 'exit_handler' implies that not only it would be called for enclave > exit resulting from an exception, but also for EEXIT path. Wile editing the commit message I read what I had written about flags. It was quite terrible, to say the least. I'll send update soon. /Jarkko
On Fri, Oct 02, 2020 at 05:27:56PM +0300, Jarkko Sakkinen wrote: > On Fri, Oct 02, 2020 at 07:25:05AM +0300, Jarkko Sakkinen wrote: > > On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote: > > > > > The documentation, selftest and the kernel source code use the terms "user > > > > > handler" and "exit handler" in somewhat inconsistent manner. Start using > > > > > "AEX handler" consistently as this is the term that Intel SDM volume D uses > > > > > for these events. > > > > > > > > No, AEX is specifically for "asynchronus" exits. In this context, that is > > > > limited to the exception path. The vDSO invokes the user handler for EEXIT, > > > > i.e. the synchrous path, which is not an AEX. > > > > > > OK, I but this one. > > > > > > I got gray hairs because: > > > > > > - API called it user handler. > > > - Documentation called it exit handler. > > > - Selftest called it exit handler. > > > > > > I think user_handler is too generic and abstract but I'm happy to hear > > > other proposals than 'exit_' to which I will change given your feedback. > > > > OK, so here I use my "too generic" argument pick user handler anyhow > > because we don't know what the heck the handler is doing. > > > > 'exit_handler' implies that not only it would be called for enclave > > exit resulting from an exception, but also for EEXIT path. > > Wile editing the commit message I read what I had written about flags. > It was quite terrible, to say the least. > > I'll send update soon. When I added validation, I noticed that the offsets were wrong. Fixing this but I think this is also one good reason for always validate the full struct and not just some flags-field. It helps to catch bugs early on [*]. Not going to do selftest improvements before sending v39 but the selftest needs even more exception test than provision. I think I work on both of these after the patch set has been sent. For exception, wouldn't be easiest to add a 2nd TCS and code path that trigger's #PF / EPCM conflict? I could add a write operation on a read-only page. [*] Proudly admitting that in v1 of this patch everything was wrong as far as flags-field is considered: no flags, no padding validation and no legit reasoning. /Jarkko
diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S index 8f8190ab9ed5..4bbb71253e17 100644 --- a/arch/x86/entry/vdso/vsgx.S +++ b/arch/x86/entry/vdso/vsgx.S @@ -7,25 +7,17 @@ #include "extable.h" -/* Offset of 'struct sgx_enclave_run' relative to %rbp. */ -#define SGX_ENCLAVE_RUN_PTR 2*8 - -/* Offsets into 'struct sgx_enclave_run'. */ -#define SGX_ENCLAVE_RUN_TCS 0*8 -#define SGX_ENCLAVE_RUN_FLAGS 1*8 -#define SGX_ENCLAVE_RUN_EXIT_REASON 1*8 + 4 -#define SGX_ENCLAVE_RUN_USER_HANDLER 2*8 -/* #define SGX_ENCLAVE_RUN_USER_DATA 3*8 */ -#define SGX_ENCLAVE_RUN_EXCEPTION 4*8 - -#define SGX_SYNCHRONOUS_EXIT 0 -#define SGX_EXCEPTION_EXIT 1 - -/* Offsets into sgx_enter_enclave.exception. */ -#define SGX_EX_LEAF 0*8 -#define SGX_EX_TRAPNR 0*8+4 -#define SGX_EX_ERROR_CODE 0*8+6 -#define SGX_EX_ADDRESS 1*8 +/* The offset relative to %rbp. */ +#define SGX_ENCLAVE_RUN_OFFSET 2*8 + +/* The offsets relative to struct sgx_enclave_run. */ +#define SGX_ENCLAVE_RUN_TCS 0*8 +#define SGX_ENCLAVE_RUN_USER_HANDLER 1*8 + +#define SGX_ENCLAVE_RUN_LEAF 3*8 +#define SGX_ENCLAVE_RUN_EXCEPTION_NUMBER 4*8 +#define SGX_ENCLAVE_RUN_EXCEPTION_INFO 5*8 +#define SGX_ENCLAVE_RUN_EXCEPTION_ADDR 6*8 .code64 .section .text, "ax" @@ -49,11 +41,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) cmp $ERESUME, %eax ja .Linvalid_input - mov SGX_ENCLAVE_RUN_PTR(%rbp), %rcx - - /* No flags are currently defined/supported. */ - cmpl $0, SGX_ENCLAVE_RUN_FLAGS(%rcx) - jne .Linvalid_input + mov SGX_ENCLAVE_RUN_OFFSET(%rbp), %rcx /* Load TCS and AEP */ mov SGX_ENCLAVE_RUN_TCS(%rcx), %rbx @@ -65,10 +53,10 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) enclu /* EEXIT jumps here unless the enclave is doing something fancy. */ - mov SGX_ENCLAVE_RUN_PTR(%rbp), %rbx + mov SGX_ENCLAVE_RUN_OFFSET(%rbp), %rbx /* Set exit_reason. */ - movl $SGX_SYNCHRONOUS_EXIT, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx) + movl $EEXIT, SGX_ENCLAVE_RUN_LEAF(%rbx) /* Invoke userspace's exit handler if one was provided. */ .Lhandle_exit: @@ -92,15 +80,13 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) jmp .Lout .Lhandle_exception: - mov SGX_ENCLAVE_RUN_PTR(%rbp), %rbx - - /* Set the exit_reason and exception info. */ - movl $SGX_EXCEPTION_EXIT, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx) + mov SGX_ENCLAVE_RUN_OFFSET(%rbp), %rbx - mov %eax, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_LEAF)(%rbx) - mov %di, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_TRAPNR)(%rbx) - mov %si, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_ERROR_CODE)(%rbx) - mov %rdx, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_ADDRESS)(%rbx) + /* Set the exception info. */ + mov %eax, (SGX_ENCLAVE_RUN_LEAF)(%rbx) + mov %di, (SGX_ENCLAVE_RUN_EXCEPTION_NUMBER)(%rbx) + mov %si, (SGX_ENCLAVE_RUN_EXCEPTION_INFO)(%rbx) + mov %rdx, (SGX_ENCLAVE_RUN_EXCEPTION_ADDR)(%rbx) jmp .Lhandle_exit .Linvoke_userspace_handler: diff --git a/arch/x86/include/asm/enclu.h b/arch/x86/include/asm/enclu.h index 06157b3e9ede..b1314e41a744 100644 --- a/arch/x86/include/asm/enclu.h +++ b/arch/x86/include/asm/enclu.h @@ -4,5 +4,6 @@ #define EENTER 0x02 #define ERESUME 0x03 +#define EEXIT 0x04 #endif /* _ASM_X86_ENCLU_H */ diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 236dd7190431..71e44406da13 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -96,42 +96,25 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, struct sgx_enclave_run *run); /** - * struct sgx_enclave_exception - structure to report exceptions encountered in - * __vdso_sgx_enter_enclave() - * @leaf: ENCLU leaf from \%eax at time of exception - * @trapnr: exception trap number, a.k.a. fault vector - * @error_code: exception error code - * @address: exception address, e.g. CR2 on a #PF - */ -struct sgx_enclave_exception { - __u32 leaf; - __u16 trapnr; - __u16 error_code; - __u64 address; -}; - -/** - * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave() - * @tcs: Thread Control Structure used to enter enclave - * @flags: Control flags - * @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 + * struct sgx_enclave_run - the execution context of __vdso_sgx_enter_enclave() + * @tcs: TCS used to enter the enclave + * @aex_handler: Handler for asynchronous exit (AEX) + * @aex_data: Data passed to the AEX handler + * @leaf: The ENCLU leaf we were at (EENTER/ERESUME/EEXIT) + * @exception_number: The exception number that triggered AEX + * @exception_info: The associated info pulled out from the stack + * @exception_addr: The address that triggered the exception + * @reserved Reserved for possible future use */ struct sgx_enclave_run { __u64 tcs; - __u32 flags; - __u32 exit_reason; - __u64 user_handler; - __u64 user_data; - - union { - struct sgx_enclave_exception exception; - - /* Pad the entire struct to 256 bytes. */ - __u8 pad[256 - 32]; - }; + __u64 aex_handler; + __u64 aex_data; + __u32 leaf; + __u16 exception_number; + __u16 exception_info; + __u64 exception_addr; + __u64 reserved[3]; }; /** @@ -157,10 +140,10 @@ struct sgx_enclave_run { * loaded with @leaf, asynchronous exit pointer, and @run.tcs respectively. * * RBP and the stack are used to anchor __vdso_sgx_enter_enclave() to the - * pre-enclave state, e.g. to retrieve @run.exception and @run.user_handler + * pre-enclave state, e.g. to retrieve @run.exception and @run.aex_handler * after an enclave exit. All other registers are available for use by the * enclave and its runtime, e.g. an enclave can push additional data onto the - * stack (and modify RSP) to pass information to the optional exit handler (see + * stack (and modify RSP) to pass information to the optional AEX handler (see * below). * * Most exceptions reported on ENCLU, including those that occur within the @@ -170,22 +153,22 @@ struct sgx_enclave_run { * reported exceptions, -EFAULT is returned and details about the exception are * recorded in @run.exception, the optional sgx_enclave_exception struct. * - * If an exit handler is provided, the handler will be invoked on synchronous + * If an AEX handler is provided, the handler will be invoked on synchronous * exits from the enclave and for all synchronously reported exceptions. In * latter case, @run.exception is filled prior to invoking the handler. * - * The exit handler's return value is interpreted as follows: + * The AEX handler's return value is interpreted as follows: * >0: continue, restart __vdso_sgx_enter_enclave() with @ret as @leaf * 0: success, return @ret to the caller * <0: error, return @ret to the caller * - * The exit handler may transfer control, e.g. via longjmp() or C++ exception, + * The AEX handler may transfer control, e.g. via longjmp() or C++ exception, * without returning to __vdso_sgx_enter_enclave(). * * Return: * 0 on success (ENCLU reached), * -EINVAL if ENCLU leaf is not allowed, - * -errno for all other negative values returned by the userspace exit handler + * -errno for all other negative values returned by the userspace AEX handler */ typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi, unsigned long rdx, unsigned int leaf, diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 0316701d6bbd..cc9e71b29861 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -129,26 +129,26 @@ int check_result(struct sgx_enclave_run *run, int ret, uint64_t result, if (ret) { printf("FAIL: %s() returned: %d\n", test, ret); return ret; - } else if (run->exit_reason != SGX_SYNCHRONOUS_EXIT) { - printf("FAIL: %s() exit reason, expected: %u, got: %u\n", - test, SGX_SYNCHRONOUS_EXIT, run->exit_reason); + } else if (run->leaf != EEXIT) { + printf("FAIL: %s() leaf, expected: %u, got: %u\n", test, EEXIT, + run->leaf); return -EIO; } else if (result != MAGIC) { - printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n", - test, MAGIC, result); + printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n", test, MAGIC, + result); return -EIO; - } else if (run->user_data) { + } else if (run->aex_data) { printf("FAIL: %s() user data, expected: 0x0, got: 0x%llx\n", - test, run->user_data); + test, run->aex_data); return -EIO; } return 0; } -static int exit_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9, +static int aex_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9, struct sgx_enclave_run *run) { - run->user_data = 0; + run->aex_data = 0; return 0; } @@ -215,11 +215,11 @@ int main(int argc, char *argv[], char *envp[]) goto err; /* And with an exit handler. */ - run.user_handler = (__u64)exit_handler; - run.user_data = 0xdeadbeef; + run.aex_handler = (__u64)aex_handler; + run.aex_data = 0xdeadbeef; ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, 0, 0, &run); - if (check_result(&run, ret, result, "exit_handler")) + if (check_result(&run, ret, result, "aex_handler")) goto err; printf("SUCCESS\n");
Reduce the struct size to 64 bytes. It makes sense to have a naturally aligning struct size and it leaves 24 bytes of spare space for possible future expansion. Having 256 bytes is over the top without any reasonable explanation, which does not exist in the commit for the SGX vDSO. Drop struct sgx_exception. It does not serve any semantic purpose in the context of this vDSO. Doing this is also consistent with user_handler and user_data fields, as neither are they encapsulated in an embedded data structure. Drop 'flags' from sgx_enclave_run as it is an unused value.'vsgx.S' literally states this in an inline comment. Pick more reasonable names for the four fields of the exception. For example, 'trapnr' should rather be just 'number' because in practice we deliver only trap and the rest are faults. Also 'vector' would be an adequate name for this field. Document the fields properly in order to better explain the values that they have inherited. Remove 'exit_reason' as it is really a redundant field to save some space. Instead, have just a field called 'leaf' that tells the last seen ENCLU. The documentation, selftest and the kernel source code use the terms "user handler" and "exit handler" in somewhat inconsistent manner. Start using "AEX handler" consistently as this is the term that Intel SDM volume D uses for these events. Cc: Nathaniel McCallum <npmccallum@redhat.com> Cc: Haitao Huang <haitao.huang@linux.intel.com> CC: Sean Christopherson <sean.j.christopherson@intel.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Andy Lutomirski <luto@amacapital.net> Cc: Cedric Xing <cedric.xing@intel.com> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/entry/vdso/vsgx.S | 54 ++++++++++---------------- arch/x86/include/asm/enclu.h | 1 + arch/x86/include/uapi/asm/sgx.h | 61 +++++++++++------------------- tools/testing/selftests/sgx/main.c | 24 ++++++------ 4 files changed, 55 insertions(+), 85 deletions(-)