On Wed, Mar 18, 2020 at 7:41 PM Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote: > > 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. You're right. __vdso() will always restore the caller's stack via the leave instruction at .Lout. So no change is necessary. > > > > 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. > > > > > > > > > > > > > > >
