On Sat, Mar 14, 2020 at 10:10:26AM -0400, Nathaniel McCallum wrote: > On Fri, Mar 13, 2020 at 6:08 PM Sean Christopherson > <sean.j.christopherson@xxxxxxxxx> wrote: > > > > > 4. sub/add to %rsp rather than save/restore > > > > > > > > Can you elaborate on why you want to sub/add to %rsp instead of having the > > > > enclave unwind the stack? Preserving %rsp across EEXIT/ERESUME seems more > > > > in line with function call semantics, which I assume is desirable? E.g. > > > > > > > > push param3 > > > > push param2 > > > > push param1 > > > > > > > > enclu[EEXIT] > > > > > > > > add $0x18, %rsp > > > > > > Before enclave EEXIT, the enclave restores %rsp to the value it had > > > before EENTER was called. Then it pushes additional output arguments > > > onto the stack. The enclave calls EENCLU[EEXIT]. > > > > > > We are now in __vdso...() on the way back to the caller. However, %rsp > > > has a different value than we entered the function with. This breaks > > > x86_64 ABI, obviously. The handler needs to fix this up: how does it > > > do so? Circling back to this request, because I just realized that the above is handled by saving %rsp into %rbp and requiring the enclave and handler to preserve %rbp at all times. So the below discussion on making the %rsp adjustment relative is moot, at least with respect to getting out of __vdso() if the enclave has mucked with the untrusted stack. > > > In the current code, __vdso..() saves the value of %rsp, calls the > > > handler and then restores %rsp. The handler can fix up the stack by > > > setting the correct value to %rbx and returning without restoring it. > > > > Ah, you're referring to the patch where the handler decides to return all > > the way back to the caller of __vdso...(). > > > > > But this requires internal knowledge of the __vdso...() function, > > > which could theoretically change in the future. > > > > > > If instead the __vdso...() only did add/sub, then the handler could do: > > > 1. pop return address > > > 2. pop handler stack params > > > 3. pop enclave additional output stack params > > > 4. push handler stack params > > > 5. push return address Per above, this is unnecessary when returning to the caller of __vdso(). It would be necessary if the enclave wasn't smart enough to do it's own stack cleanup, but that seems like a very bizarre contract between the enclave and its runtime. The caveat is if %rbx is saved/restored by __vdso(). If we want a traditional frame pointer, then %rbx would be restored from the stack before %rsp itself is restored (from %rbp), in which case the exit handler would need to adjust %rsp using a sequence similar to what you listed above. If __vdso() uses a non-standard frame pointer, e.g. push %rbp push %rbx mov %rsp, %rbp then %rbx would come off the stack after %rsp is restored from %rbp, i.e. would be guaranteed to be restored to the pre-EENTER value (unless the enclave or handler mucks with %rbp). Anyways, we can discuss how to implement the frame pointer in the context of the patches, just wanted to point this out here for completeness. > > > While this is more work, it is standard calling convention work that > > > doesn't require internal knowledge of __vdso..(). Alternatively, if we > > > don't like the extra work, we can document the %rbx hack explicitly > > > into the handler documentation and make it part of the interface. But > > > we need some explicit way for the handler to pop enclave output stack > > > params that doesn't depend on internal knowledge of the __vdso...() > > > invariants. > > > > IIUC, this is what you're suggesting? Having to align the stack makes this > > a bit annoying, but it's not bad by any means. > > > > diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S > > index 94a8e5f99961..05d54f79b557 100644 > > --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S > > +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S > > @@ -139,8 +139,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > > /* Pass the untrusted RSP (at exit) to the callback via %rcx. */ > > mov %rsp, %rcx > > > > - /* Save the untrusted RSP in %rbx (non-volatile register). */ > > + /* Save the untrusted RSP offset in %rbx (non-volatile register). */ > > mov %rsp, %rbx > > + and $0xf, %rbx > > > > /* > > * Align stack per x86_64 ABI. Note, %rsp needs to be 16-byte aligned > > @@ -161,8 +162,8 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave) > > mov 0x20(%rbp), %rax > > call .Lretpoline > > > > - /* Restore %rsp to its post-exit value. */ > > - mov %rbx, %rsp > > + /* Undo the post-exit %rsp adjustment. */ > > + lea 0x20(%rsp,%rbx), %rsp > > > > > > That's reasonable, let's the handler play more games with minimal overhead. > > Yes, exactly! > > > > > > That would make this a very usable and fast interface without > > > > > sacrificing any of its current power. > > > > > > > > > >
