| Message ID | 20190320162119.4469-25-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel SGX1 support | expand |
> +/** > + * __vdso_sgx_enter_enclave() - Enter an SGX enclave > + * > + * %eax: ENCLU leaf, must be EENTER or ERESUME > + * %rbx: TCS, must be non-NULL > + * %rcx: Optional pointer to 'struct sgx_enclave_exception' > + * > + * Return: > + * 0 on a clean entry/exit to/from the enclave > + * -EINVAL if ENCLU leaf is not allowed or if TCS is NULL > + * -EFAULT if ENCLU or the enclave faults > + * > + * Note that __vdso_sgx_enter_enclave() is not compliant with the x86- > 64 ABI. > + * All registers except RSP must be treated as volatile from the > +caller's > + * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, > FCW, etc... > + * Conversely, the enclave being run must preserve the untrusted RSP > and stack. By requiring preservation of RSP at both AEX and EEXIT, this precludes the possibility of using the untrusted stack as temporary storage by enclaves. While that looks reasonable at first glance, I'm afraid it isn't the case in reality. The untrusted stack is inarguably the most convenient way for data exchange between an enclave and its enclosing process, and is in fact being used for that purpose by almost all existing enclaves to date. Given the expectation that this API will be used by all future SGX application, it looks unwise to ban the most convenient and commonly used approach for data exchange. Given an enclave can touch everything (registers and memory) of the enclosing process, it's reasonable to restrict the enclave by means of "calling convention" to allow the enclosing process to retain its context. And for that purpose, SGX ISA does offer 2 registers (i.e. RSP and RBP) for applications to choose. Instead of preserving RSP, I'd prefer RBP, which will end up with more flexibility in all SGX applications in future. > + * __vdso_sgx_enter_enclave(u32 leaf, void *tcs, > + * struct sgx_enclave_exception *exception_info) > + * { > + * if (leaf != SGX_EENTER && leaf != SGX_ERESUME) > + * return -EINVAL; > + * > + * if (!tcs) > + * return -EINVAL; > + * > + * try { > + * ENCLU[leaf]; > + * } catch (exception) { > + * if (e) > + * *e = exception; > + * return -EFAULT; > + * } > + * > + * return 0; > + * } > + */ > +ENTRY(__vdso_sgx_enter_enclave) > + /* EENTER <= leaf <= ERESUME */ > + cmp $0x2, %eax > + jb bad_input > + > + cmp $0x3, %eax > + ja bad_input > + > + /* TCS must be non-NULL */ > + test %rbx, %rbx > + je bad_input > + > + /* Save @exception_info */ > + push %rcx > + > + /* Load AEP for ENCLU */ > + lea 1f(%rip), %rcx > +1: enclu > + > + add $0x8, %rsp > + xor %eax, %eax > + ret > + > +bad_input: > + mov $(-EINVAL), %rax > + ret > + > +.pushsection .fixup, "ax" > + /* Re-load @exception_info and fill it (if it's non-NULL) */ > +2: pop %rcx > + test %rcx, %rcx > + je 3f > + > + mov %eax, EX_LEAF(%rcx) > + mov %di, EX_TRAPNR(%rcx) > + mov %si, EX_ERROR_CODE(%rcx) > + mov %rdx, EX_ADDRESS(%rcx) > +3: mov $(-EFAULT), %rax > + ret > +.popsection > + > +_ASM_VDSO_EXTABLE_HANDLE(1b, 2b) > + > +ENDPROC(__vdso_sgx_enter_enclave) Rather than preserving RSP, an alternative that preserves RBP will allow more flexibility inside SGX applications. Below is the assembly code based on that idea, that offers a superset of functionality over the current patch, yet at a cost of just 9 more lines of code (23 LOC here vs. 14 LOC in the patch). /** * __vdso_sgx_enter_enclave() - Enter an SGX enclave * * %eax: ENCLU leaf, must be either EENTER or ERESUME * 0x08(%rsp): TCS * 0x10(%rsp): Optional pointer to 'struct sgx_enclave_exception' * 0x18(%rsp): Optional function pointer to 'sgx_exit_handler', defined below * typedef int (*sgx_exit_handler)(struct sgx_enclave_exception *ex_info); * return: Non-negative integer to indicate success, or a negative error * code on failure. * * Note that __vdso_sgx_enter_enclave() is not compatible with x86_64 ABI. * All registers except RBP must be treated as volatile from the caller's * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... * Enclave may decrement RSP, but must not increment it - i.e. existing content * of the stack shall be preserved. */ __vdso_sgx_enter_enclave: push %rbp mov %rsp, %rbp /* EENTER <= leaf <= ERESUME */ 1: cmp $0x2, %eax jb bad_input cmp $0x3, %eax ja bad_leaf /* Load TCS and AEP */ mov 0x10(%rbp), %rbx lea 2f(%rip), %rcx 2: enclu mov 0x18(%rbp), %rcx jrcxz 3f /* Besides leaf, this instruction also zeros trapnr and error_code */ mov %rax, EX_LEAF(%rcx) 3: mov %rcx, %rdi mov 0x20(%rbp), %rcx jrcxz 4f call *%rcx jmp 1b 4: leave ret bad_leaf: cmp $0, %eax jle 4b mov $(-EINVAL), %eax jmp 4b .pushsection .fixup, "ax" 5: mov 0x18(%rbp), %rcx jrcxz 6f mov %eax, EX_LEAF(%rcx) mov %di, EX_TRAPNR(%rcx) mov %si, EX_ERROR_CODE(%rcx) mov %rdx, EX_ADDRESS(%rcx) 6: mov $(-EFAULT), %eax jmp 3b .popsection _ASM_VDSO_EXTABLE_HANDLE(2b, 5b)
On Wed, Mar 20, 2019 at 11:30 AM Xing, Cedric <cedric.xing@intel.com> wrote: > > > +/** > > + * __vdso_sgx_enter_enclave() - Enter an SGX enclave > > + * > > + * %eax: ENCLU leaf, must be EENTER or ERESUME > > + * %rbx: TCS, must be non-NULL > > + * %rcx: Optional pointer to 'struct sgx_enclave_exception' > > + * > > + * Return: > > + * 0 on a clean entry/exit to/from the enclave > > + * -EINVAL if ENCLU leaf is not allowed or if TCS is NULL > > + * -EFAULT if ENCLU or the enclave faults > > + * > > + * Note that __vdso_sgx_enter_enclave() is not compliant with the x86- > > 64 ABI. > > + * All registers except RSP must be treated as volatile from the > > +caller's > > + * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, > > FCW, etc... > > + * Conversely, the enclave being run must preserve the untrusted RSP > > and stack. > > By requiring preservation of RSP at both AEX and EEXIT, this precludes the possibility of using the untrusted stack as temporary storage by enclaves. While that looks reasonable at first glance, I'm afraid it isn't the case in reality. The untrusted stack is inarguably the most convenient way for data exchange between an enclave and its enclosing process, and is in fact being used for that purpose by almost all existing enclaves to date. Given the expectation that this API will be used by all future SGX application, it looks unwise to ban the most convenient and commonly used approach for data exchange. I'm going to go out on a limb and say that this is a good thing. Using the untrusted stack as a way to exchange data is very convenient, but that doesn't mean it's a good idea. Here are some problems it causes: - It prevents using a normal function to wrap enclave entry (as we're seeing with this patch set). - It makes quite a few unfortunate assumptions about the layout of the untrusted stack. It assumes that the untrusted stack is arbitrarily expandable, which is entirely untrue in languages like Go. It assumes that the untrusted stack isn't further constrained by various CFI mechanisms (e.g. CET), and, as of last time I checked, the interaction between CET and SGX was still not specified. It also assumes that the untrusted stack doesn't have ABI-imposed layout restrictions related to unwinding, and, as far as I know, this means that current enclaves with current enclave runtimes can interact quite poorly with debuggers, exception handling, and various crash dumping technologies. - It will make it quite unpleasant to call into an enclave in a coroutine depending on how the host untrusted runtime implements coroutines. So I think it's a *good* thing if the effect is to make enclave SDKs change their memory management so that untrusted buffers are explicitly supplied by the host runtime. Honestly, I would have much preferred if the architecture did not give the enclave access to RSP and RBP at all. (And, for that matter, RIP.)
On 2019-03-20 11:30, Xing, Cedric wrote: >> +/** >> + * __vdso_sgx_enter_enclave() - Enter an SGX enclave >> + * >> + * %eax: ENCLU leaf, must be EENTER or ERESUME >> + * %rbx: TCS, must be non-NULL >> + * %rcx: Optional pointer to 'struct sgx_enclave_exception' >> + * >> + * Return: >> + * 0 on a clean entry/exit to/from the enclave >> + * -EINVAL if ENCLU leaf is not allowed or if TCS is NULL >> + * -EFAULT if ENCLU or the enclave faults >> + * >> + * Note that __vdso_sgx_enter_enclave() is not compliant with the x86- >> 64 ABI. >> + * All registers except RSP must be treated as volatile from the >> +caller's >> + * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, >> FCW, etc... >> + * Conversely, the enclave being run must preserve the untrusted RSP >> and stack. > > By requiring preservation of RSP at both AEX and EEXIT, this precludes the possibility of using the untrusted stack as temporary storage by enclaves. While that looks reasonable at first glance, I'm afraid it isn't the case in reality. The untrusted stack is inarguably the most convenient way for data exchange between an enclave and its enclosing process, and is in fact being used for that purpose by almost all existing enclaves to date. Given the expectation that this API will be used by all future SGX application, it looks unwise to ban the most convenient and commonly used approach for data exchange. For reference, here's the code in the Intel toolchain responsible for this: https://github.com/intel/linux-sgx/blob/6a0b5ac71f8d16f04e0376f3b2168e80c773dd23/sdk/trts/trts.cpp#L125-L140 Regarding "almost all existing enclaves to date", enclaves built with the Fortanix toolchain don't touch the untrusted stack. -- Jethro Beekman | Fortanix
On Wed, Mar 20, 2019 at 11:30:26AM -0700, Xing, Cedric wrote: > > +/** > > + * __vdso_sgx_enter_enclave() - Enter an SGX enclave > > + * > > + * %eax: ENCLU leaf, must be EENTER or ERESUME > > + * %rbx: TCS, must be non-NULL > > + * %rcx: Optional pointer to 'struct sgx_enclave_exception' > > + * > > + * Return: > > + * 0 on a clean entry/exit to/from the enclave > > + * -EINVAL if ENCLU leaf is not allowed or if TCS is NULL > > + * -EFAULT if ENCLU or the enclave faults > > + * > > + * Note that __vdso_sgx_enter_enclave() is not compliant with the x86- > > 64 ABI. > > + * All registers except RSP must be treated as volatile from the > > +caller's > > + * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, > > FCW, etc... > > + * Conversely, the enclave being run must preserve the untrusted RSP > > and stack. > > By requiring preservation of RSP at both AEX and EEXIT, this precludes > the possibility of using the untrusted stack as temporary storage by > enclaves. While that looks reasonable at first glance, I'm afraid it > isn't the case in reality. The untrusted stack is inarguably the most > convenient way for data exchange between an enclave and its enclosing > process, I vehemently disagree with "inarguably". IMO, passing data via registers is much more convenient. Even if you qualify your assertion with "data of arbitrary size unknown at build time", I still disagree. Using the untrusted stack allows for trickery when a debugger is involved, other than that I see no advantages over allocating virtual memory and handing the pointer to the enclave at launch time. Sure, it requires a few more lines of code to setup, but it's literally ~20 LoC out of thousands required to sign, build and launch an enclave, but it doesn't require playing games with the stack. Not to mention that the entire concept of using the untrusted stack is based on the assumption that the enclave is making ocalls, e.g. stateless enclaves or libraries that use a message queue have zero need/benefit for using the untrusted stack. > and is in fact being used for that purpose by almost all existing enclaves > to date. That's a bit misleading, since almost all existing enclaves are built against Intel's SDK, which just so happens to unconditionally use the untrusted stack. It's not like all enclave developers made a concious decision to use the untrusted stack. If Intel rewrote the SDK to use a different method then one could argue that the new approach is the most common method of passing data. > Given the expectation that this API will be used by all future SGX > application, it looks unwise to ban the most convenient and commonly > used approach for data exchange. > > Given an enclave can touch everything (registers and memory) of the > enclosing process, it's reasonable to restrict the enclave by means > of "calling convention" to allow the enclosing process to retain its > context. And for that purpose, SGX ISA does offer 2 registers (i.e. > RSP and RBP) for applications to choose. Instead of preserving RSP, > I'd prefer RBP, which will end up with more flexibility in all SGX > applications in future. I disagree that the SGX ISA intends for applications to choose between preserving RSP and RBP, e.g. the SDM description of SSA.UR{B,S}P states: Non-Enclave (outside) {RBP,stack} pointer. Saved by EENTER, restored on AEX. To me, the "Saved/restored" wording implies that URBP and URSP should *never* be touched by the enclave. Sure, the proposed vDSO interface doesn't require RBP to be preserved, but only because the goal was to deviate from hardware as little as possible, not because anyone wants to encourage enclaves to muck with RBP.
> Using the untrusted stack as a way to exchange data is very convenient, > but that doesn't mean it's a good idea. Here are some problems it > causes: > > - It prevents using a normal function to wrap enclave entry (as we're > seeing with this patch set). It doesn't prevent. It's all about what's agreed between the enclave and its hosting process. With the optional "exit/exception callback" set to null, this will behave exactly the same as in the current patch. That's what I meant by "flexibility" and "superset of functionality". > > - It makes quite a few unfortunate assumptions about the layout of the > untrusted stack. It assumes that the untrusted stack is arbitrarily > expandable, which is entirely untrue in languages like Go. I'm with you that stack is not always good thing, hence I'm NOT ruling out any other approaches for exchanging data. But is stack "bad" enough to be ruled out completely? The point here is flexibility because the stack could be "good" for its convenience. After all, only buffers of "reasonable" sizes will be exchanged in most cases, and in the rare exceptions of stack overflow they'd probably get caught in validation anyway. The point here again is - flexibility. I'd say it's better to leave the choice to the SDK implementers than to force the choice on them. > It assumes that the untrusted stack isn't further constrained by various > CFI mechanisms (e.g. CET), and, as of last time I checked, the > interaction between CET and SGX was still not specified. I was one of the architects participating in the CET ISA definition. The assembly provided was crafted with CET in mind and will be fully compatible with CET. > It also > assumes that the untrusted stack doesn't have ABI-imposed layout > restrictions related to unwinding, and, as far as I know, this means > that current enclaves with current enclave runtimes can interact quite > poorly with debuggers, exception handling, and various crash dumping > technologies. Per comments from the patch set, I guess it's been agreed that this vDSO function will NOT be x86_64 ABI compatible. So I'm not sure why stacking unwinding is relevant here. However, I'm with you that we should take debugging/exception handling/reporting/crash dumping into consideration by making this vDSO API x86_64 ABI compatible. IMO it's trivial and the performance overhead in negligible (dwarfed by ENCLU anyway. I'd be more than happy to provide a x86_64 ABI compatible version if that's also your preference. > - It will make it quite unpleasant to call into an enclave in a > coroutine depending on how the host untrusted runtime implements > coroutines. I'm not sure what you are referring to by "coroutine". But this vDSO API will be (expected to be) the only routine that actually calls into an enclave. Isn't that correct? > > So I think it's a *good* thing if the effect is to make enclave SDKs > change their memory management so that untrusted buffers are explicitly > supplied by the host runtime. Intel SGX SDK will change no matter what. The point is flexibility, is to offer choices and let SDK implementers decide, instead of deciding for them ahead of time. > Honestly, I would have much preferred if > the architecture did not give the enclave access to RSP and RBP at all. > (And, for that matter, RIP.) This reminds me of PUSHA/POPA instructions. We once thought those instruction would be appreciated by compilers but the fact turns out that most compilers prefer a mix of caller-saved/callee-saved GPRs instead of treating all GPRs caller or callee saved. Then when we believed everyone would prefer a mix after so many years, an exception emerged as GO was invented. That said, flexibility is the point and is the most important thing ISA is always trying to offer. The rest is just software convention. So we decided not to enforce RBP/RSP, unless there are security implications - e.g. RIP - EEXIT will be considered an indirect branch instruction and will have to land on an ENDBR once CET comes out.
> > By requiring preservation of RSP at both AEX and EEXIT, this precludes > the possibility of using the untrusted stack as temporary storage by > enclaves. While that looks reasonable at first glance, I'm afraid it > isn't the case in reality. The untrusted stack is inarguably the most > convenient way for data exchange between an enclave and its enclosing > process, and is in fact being used for that purpose by almost all > existing enclaves to date. Given the expectation that this API will be > used by all future SGX application, it looks unwise to ban the most > convenient and commonly used approach for data exchange. > > For reference, here's the code in the Intel toolchain responsible for > this: > https://github.com/intel/linux- > sgx/blob/6a0b5ac71f8d16f04e0376f3b2168e80c773dd23/sdk/trts/trts.cpp#L125 > -L140 > > Regarding "almost all existing enclaves to date", enclaves built with > the Fortanix toolchain don't touch the untrusted stack. > > -- > Jethro Beekman | Fortanix Thanks for providing the references. Yes, not every enclave touches the untrusted stack so I used the word "almost". Everything exists for a reason. By bringing up what is done today, I was trying to inspire thinking on the more important question of "why is it done this way today?".
> > By requiring preservation of RSP at both AEX and EEXIT, this precludes > > the possibility of using the untrusted stack as temporary storage by > > enclaves. While that looks reasonable at first glance, I'm afraid it > > isn't the case in reality. The untrusted stack is inarguably the most > > convenient way for data exchange between an enclave and its enclosing > > process, > > I vehemently disagree with "inarguably". IMO, passing data via > registers is much more convenient. Which is the most convenient approach is always dependent on data size and/or even how the data is produced/consumed. It's kind of a spectrum and we're just talking in the sense of probability. You are right that "inarguably" is arguable if the buffer is small enough to fit in registers, and the producer/consumer also has access to registers. > > Even if you qualify your assertion with "data of arbitrary size unknown > at build time", I still disagree. Using the untrusted stack allows for > trickery when a debugger is involved, other than that I see no > advantages over allocating virtual memory and handing the pointer to the > enclave at launch time. Sure, it requires a few more lines of code to > setup, but it's literally ~20 LoC out of thousands required to sign, > build and launch an enclave, but it doesn't require playing games with > the stack. I'm NOT ruling out your approach. And like you said, the untrusted stack enables certain trickery that helps debugging and also simplifies enclaves (even just a little). Then why are you trying to rule that out? Because of 9 LOC in vDSO? > > Not to mention that the entire concept of using the untrusted stack is > based on the assumption that the enclave is making ocalls, e.g. > stateless enclaves or libraries that use a message queue have zero > need/benefit for using the untrusted stack. Don't get me wrong. I never said enclaves would require untrusted stack to make ocalls, or ocalls would require untrusted stack to make. It's just a generic approach for sharing/exchanging data. Some enclaves my need it, others may not. My question still remains: why do you want to rule it out? > > and is in fact being used for that purpose by almost all existing > > enclaves to date. > > That's a bit misleading, since almost all existing enclaves are built > against Intel's SDK, which just so happens to unconditionally use the > untrusted stack. It's not like all enclave developers made a concious > decision to use the untrusted stack. If Intel rewrote the SDK to use a > different method then one could argue that the new approach is the most > common method of passing data. Everything exists for a reason. It's unimportant what has been done. What matters is why that was done in that particular way. I was trying to inspire thinking. > > > Given the expectation that this API will be used by all future SGX > > application, it looks unwise to ban the most convenient and commonly > > used approach for data exchange. > > > > Given an enclave can touch everything (registers and memory) of the > > enclosing process, it's reasonable to restrict the enclave by means of > > "calling convention" to allow the enclosing process to retain its > > context. And for that purpose, SGX ISA does offer 2 registers (i.e. > > RSP and RBP) for applications to choose. Instead of preserving RSP, > > I'd prefer RBP, which will end up with more flexibility in all SGX > > applications in future. > > I disagree that the SGX ISA intends for applications to choose between > preserving RSP and RBP, e.g. the SDM description of SSA.UR{B,S}P states: > > Non-Enclave (outside) {RBP,stack} pointer. Saved by EENTER, restored > on AEX. > > To me, the "Saved/restored" wording implies that URBP and URSP should > *never* be touched by the enclave. Sure, the proposed vDSO interface > doesn't require RBP to be preserved, but only because the goal was to > deviate from hardware as little as possible, not because anyone wants to > encourage enclaves to muck with RBP. I'm so sorry to tell you that you have misunderstood the SDM. If this is a common misunderstanding, I guess I could talk to the architect responsible for this SDM chapter to see if we could amend the language. The purpose of restoring RSP is because software needs a stack to handle exception. Well, that's not 100% accurate because it's a user mode stack. Anyway, it tells the used part from the unused space in the stack. RBP on the other hand is NEVER required from interrupt/exception handling perspective, but we decided to add it because we'd like to offer a choice, just like I said earlier. The calling thread could then anchor its frame on either RSP or RBP.
On Wed, Mar 20, 2019 at 12:57:52PM -0700, Xing, Cedric wrote: > > Using the untrusted stack as a way to exchange data is very convenient, > > but that doesn't mean it's a good idea. Here are some problems it > > causes: > > > > - It prevents using a normal function to wrap enclave entry (as we're > > seeing with this patch set). > > It doesn't prevent. Yes it does, keyword being "normal". Though I guess we could do a bit of bikeshedding on the definition of normal... > > - It makes quite a few unfortunate assumptions about the layout of the > > untrusted stack. It assumes that the untrusted stack is arbitrarily > > expandable, which is entirely untrue in languages like Go. > > I'm with you that stack is not always good thing, hence I'm NOT ruling > out any other approaches for exchanging data. But is stack "bad" enough > to be ruled out completely? The point here is flexibility because the > stack could be "good" for its convenience. After all, only buffers of > "reasonable" sizes will be exchanged in most cases, and in the rare > exceptions of stack overflow they'd probably get caught in validation > anyway. The point here again is - flexibility. I'd say it's better to > leave the choice to the SDK implementers than to force the choice on them. Actually, this series doesn't force anything on Intel's SDK, as there is nothing in the documentation that states the vDSO *must* be used to enter enclaves. In other words, unless it's expressly forbidden, applications are free to enter enclaves directly and do as they wish with the untrusted stack. The catch being that any such usage will need to deal with enclave exceptions being delivered as signals, i.e. the vDSO implementation is a carrot, not a stick. AIUI, excepting libraries that want to manipulate the untrusted stack, there is a general consensus that the proposed vDSO implementation is the right approach, e.g. full x86_64 ABI compatibility was explored in the past and was deemed to add unnecessary complexity and overhead. The vDSO *could* be written in such a way that it supports preserving RBP or RSP, but for all intents and purposes such an implementation would yield two distinct ABIs that just happen to be implemented in a single function. And *if* we want to deal with maintaining two ABIs, supporting the kernel's existing signal ABI is a lot cleaner (from a kernel perspective) than polluting the vDSO. In other words, if there is a desire to support enclaves which modify the untrusted stack, and it sounds like there is, then IMO our time is better spent discussing whether or not to officially support the signal ABI for enclaves. > > It assumes that the untrusted stack isn't further constrained by various > > CFI mechanisms (e.g. CET), and, as of last time I checked, the > > interaction between CET and SGX was still not specified. > > I was one of the architects participating in the CET ISA definition. > The assembly provided was crafted with CET in mind and will be fully > compatible with CET. > > > It also > > assumes that the untrusted stack doesn't have ABI-imposed layout > > restrictions related to unwinding, and, as far as I know, this means > > that current enclaves with current enclave runtimes can interact quite > > poorly with debuggers, exception handling, and various crash dumping > > technologies. > > Per comments from the patch set, I guess it's been agreed that this > vDSO function will NOT be x86_64 ABI compatible. So I'm not sure why > stacking unwinding is relevant here. I think Andy's point is that a single PUSH (to save %rcx) won't break unwinding, etc..., but unwinders and whantot will have a rough go of it if %rsp points at complete garbage. > However, I'm with you that we > should take debugging/exception handling/reporting/crash dumping into > consideration by making this vDSO API x86_64 ABI compatible. IMO it's > trivial and the performance overhead in negligible (dwarfed by ENCLU > anyway. I'd be more than happy to provide a x86_64 ABI compatible > version if that's also your preference. It's not just the performance cost, making __vdso_sgx_enter_enclave() compatible with the x86_64 ABI adds complexity to both its code and its documentation, e.g. to describe how data is marshalled to/from enclaves.
> On Wed, Mar 20, 2019 at 12:57:52PM -0700, Xing, Cedric wrote: > > > Using the untrusted stack as a way to exchange data is very > > > convenient, but that doesn't mean it's a good idea. Here are some > > > problems it > > > causes: > > > > > > - It prevents using a normal function to wrap enclave entry (as > > > we're seeing with this patch set). > > > > It doesn't prevent. > > Yes it does, keyword being "normal". Though I guess we could do a bit > of bikeshedding on the definition of normal... I don't understand what you mean by "normal". As I said, I tend to have a x86_64 ABI compliant version and by saying that I mean it'd be a 100% "normal" function callable from C. And the version I provided in this thread is a trimmed down version that doesn't preserve any registers except RSP/RBP so a C wrapper will be necessary. Other than that I'm not aware of any anomalies. Could you elaborate on what "abnormal" operations necessary to invoke this vDSO under what circumstances? And it'll be very helpful if you could present a "normal" function to demonstrate how your code could work with it while mine couldn't. > Actually, this series doesn't force anything on Intel's SDK, as there is > nothing in the documentation that states the vDSO *must* be used to > enter enclaves. In other words, unless it's expressly forbidden, > applications are free to enter enclaves directly and do as they wish > with the untrusted stack. The catch being that any such usage will need > to deal with enclave exceptions being delivered as signals, i.e. the > vDSO implementation is a carrot, not a stick. If you want to bike-shedding on *must*, well, no one *must* use *anything* from the *anyone*! But is that the expectation? Or if you don't expect your API to be used then what are you doing here? Intel SDK doesn't have to use this API. But we (the SDK team) are truly willing to use this API because we share the same concern with you over signals and would like to move to something better. > > AIUI, excepting libraries that want to manipulate the untrusted stack, > there is a general consensus that the proposed vDSO implementation is > the right approach, e.g. full x86_64 ABI compatibility was explored in > the past and was deemed to add unnecessary complexity and overhead. > > The vDSO *could* be written in such a way that it supports preserving > RBP or RSP, but for all intents and purposes such an implementation > would yield two distinct ABIs that just happen to be implemented in a > single function. > And *if* we want to deal with maintaining two ABIs, supporting the > kernel's existing signal ABI is a lot cleaner (from a kernel perspective) > than polluting the vDSO. Disagreed! What I'm proposing is one ABI - enclave preserves RBP! No requirements on RSP. Of course RSP is still interpreted as the line between vacant and used parts of the stack, or nothing will work regardless the proposal. The hosting process may have an agreement with the enclave to preserve RSP. But that would be completely between them, and would be just a coincidence instead of a consequence of the ABI from the perspective of this vDSO API. > > In other words, if there is a desire to support enclaves which modify > the untrusted stack, and it sounds like there is, then IMO our time is > better spent discussing whether or not to officially support the signal > ABI for enclaves. Disagreed! We share the same concern over signals so let's work this out! > > > > It assumes that the untrusted stack isn't further constrained by > > > various CFI mechanisms (e.g. CET), and, as of last time I checked, > > > the interaction between CET and SGX was still not specified. > > > > I was one of the architects participating in the CET ISA definition. > > The assembly provided was crafted with CET in mind and will be fully > > compatible with CET. > > > > > It also > > > assumes that the untrusted stack doesn't have ABI-imposed layout > > > restrictions related to unwinding, and, as far as I know, this means > > > that current enclaves with current enclave runtimes can interact > > > quite poorly with debuggers, exception handling, and various crash > > > dumping technologies. > > > > Per comments from the patch set, I guess it's been agreed that this > > vDSO function will NOT be x86_64 ABI compatible. So I'm not sure why > > stacking unwinding is relevant here. > > I think Andy's point is that a single PUSH (to save %rcx) won't break > unwinding, etc..., but unwinders and whantot will have a rough go of it > if %rsp points at complete garbage. The unwanders use CFI directives to determine frame pointers. RSP is the frame point by default but could be easily changed - e.g. by ".cfi_def_cfa_register %rbp". I could add proper CFI directives if so desired. I omitted them because you omitted them too in your code (in case you don't know, CFI directives are needed around push/pop %rcx in your code to stay compliant with ELF/DWARF spec), and I didn't want to confuse those who didn't understand CFI. > > > However, I'm with you that we > > should take debugging/exception handling/reporting/crash dumping into > > consideration by making this vDSO API x86_64 ABI compatible. IMO it's > > trivial and the performance overhead in negligible (dwarfed by ENCLU > > anyway. I'd be more than happy to provide a x86_64 ABI compatible > > version if that's also your preference. > > It's not just the performance cost, making __vdso_sgx_enter_enclave() > compatible with the x86_64 ABI adds complexity to both its code and its > documentation, e.g. to describe how data is marshalled to/from enclaves. Well, technically speaking an ABI compliant API should *reduce* documentation. But given how easy it is to write a C compliant wrapper, I think a custom ABI is perfectly fine.
On Wed, Mar 20, 2019 at 12:57 PM Xing, Cedric <cedric.xing@intel.com> wrote: > > > Using the untrusted stack as a way to exchange data is very convenient, > > but that doesn't mean it's a good idea. Here are some problems it > > causes: > > > > - It prevents using a normal function to wrap enclave entry (as we're > > seeing with this patch set). > > It doesn't prevent. It's all about what's agreed between the enclave and its hosting process. With the optional "exit/exception callback" set to null, this will behave exactly the same as in the current patch. That's what I meant by "flexibility" and "superset of functionality". > > > > > - It makes quite a few unfortunate assumptions about the layout of the > > untrusted stack. It assumes that the untrusted stack is arbitrarily > > expandable, which is entirely untrue in languages like Go. > > I'm with you that stack is not always good thing, hence I'm NOT ruling out any other approaches for exchanging data. But is stack "bad" enough to be ruled out completely? The point here is flexibility because the stack could be "good" for its convenience. After all, only buffers of "reasonable" sizes will be exchanged in most cases, and in the rare exceptions of stack overflow they'd probably get caught in validation anyway. The point here again is - flexibility. I'd say it's better to leave the choice to the SDK implementers than to force the choice on them. > > > It assumes that the untrusted stack isn't further constrained by various > > CFI mechanisms (e.g. CET), and, as of last time I checked, the > > interaction between CET and SGX was still not specified. > > I was one of the architects participating in the CET ISA definition. The assembly provided was crafted with CET in mind and will be fully compatible with CET. > > > It also > > assumes that the untrusted stack doesn't have ABI-imposed layout > > restrictions related to unwinding, and, as far as I know, this means > > that current enclaves with current enclave runtimes can interact quite > > poorly with debuggers, exception handling, and various crash dumping > > technologies. > > Per comments from the patch set, I guess it's been agreed that this vDSO function will NOT be x86_64 ABI compatible. So I'm not sure why stacking unwinding is relevant here. However, I'm with you that we should take debugging/exception handling/reporting/crash dumping into consideration by making this vDSO API x86_64 ABI compatible. IMO it's trivial and the performance overhead in negligible (dwarfed by ENCLU anyway. I'd be more than happy to provide a x86_64 ABI compatible version if that's also your preference. > > > - It will make it quite unpleasant to call into an enclave in a > > coroutine depending on how the host untrusted runtime implements > > coroutines. > > I'm not sure what you are referring to by "coroutine". But this vDSO API will be (expected to be) the only routine that actually calls into an enclave. Isn't that correct? I mean use in languages and runtimes that allow a function and its callees to pause and then resume later. Something like (pseudocode, obviously): void invoke_the_enclave() { do_eenter_through_vdso(); } void some_ocall_handler(void *ptr) { yield; } If the enclave has ptr pointing to the untrusted stack, then this gets quite awkward for the runtime to handle efficiently. IMO a much nicer approach would be: void invoke_the_enclave() { char buffer[1024]; while (true) { eenter (through vdso); if (exit was an ocall request) { some_ocall_handler(buffer); } } } And now there is nothing funny happening behind the runtime's back when some_ocall_handler tries to yield.
Hi Andy, > > > - It will make it quite unpleasant to call into an enclave in a > > > coroutine depending on how the host untrusted runtime implements > > > coroutines. > > > > I'm not sure what you are referring to by "coroutine". But this vDSO > API will be (expected to be) the only routine that actually calls into > an enclave. Isn't that correct? > > I mean use in languages and runtimes that allow a function and its > callees to pause and then resume later. Something like (pseudocode, > obviously): > > void invoke_the_enclave() > { > do_eenter_through_vdso(); > } > > void some_ocall_handler(void *ptr) > { > yield; > } Thank you very much for your detailed explanation. This looks more about whether or not the untrusted stack will remain valid after EEXIT, than whether the ocall will be paused or not. As in your example code above, a problem may occur if "yield" destroys the stack of its caller. But is that a common behavior of "yield" (or any scheduler at all)? Your point is well received though - Not every enclave can/shall assume existence or size of an untrusted stack. Therefore I've made sure my proposal will work no matter the enclave touches the untrusted stack or not. > > If the enclave has ptr pointing to the untrusted stack, then this gets > quite awkward for the runtime to handle efficiently. IMO a much nicer > approach would be: > > void invoke_the_enclave() > { > char buffer[1024]; > while (true) > { > eenter (through vdso); > if (exit was an ocall request) { > some_ocall_handler(buffer); > } > } > } > > And now there is nothing funny happening behind the runtime's back when > some_ocall_handler tries to yield. Agreed. In fact, Mr. Christopherson's API could be implemented using mine as a subroutine (please see below). So your "nicer approach" will continue to work as long as it works with current patch. However, please keep in mind that your "nicer" approach doesn't have to be the "only" approach. The code snippet below shows an equivalent implementation of Mr. Christopherson's API using mine as a subroutine, except that RBP cannot be used as a parameter to the enclave because it will be overwritten before EENTER. To distinguish his API and mine, they are renamed to __vdso_sgx_enter_enclave_Christopherson and __vdso_sgx_enter_enclave_Xing, respectively. __vdso_sgx_enter_enclave_Christopherson: push $0 /* No "exit callback" provided */ push %rcx /* Optional pointer to 'struct sgx_enclave_exception' */ push %rbx /* TCS */ call __vdso_sgx_enter_enclave_Xing add $24, %rsp ret Thanks! -Cedric
On Wed, Mar 20, 2019 at 05:17:53PM -0700, Xing, Cedric wrote: > > On Wed, Mar 20, 2019 at 12:57:52PM -0700, Xing, Cedric wrote: > > > > Using the untrusted stack as a way to exchange data is very > > > > convenient, but that doesn't mean it's a good idea. Here are some > > > > problems it > > > > causes: > > > > > > > > - It prevents using a normal function to wrap enclave entry (as > > > > we're seeing with this patch set). > > > > > > It doesn't prevent. > > > > Yes it does, keyword being "normal". Though I guess we could do a bit > > of bikeshedding on the definition of normal... > > I don't understand what you mean by "normal". My apologies, I missed the code snippet in your original code, I was thinking of a previous idea involving a JMP %rbp. Please ignore this bit of discussion.
On Wed, Mar 20, 2019 at 01:38:04PM -0700, Xing, Cedric wrote: > > > By requiring preservation of RSP at both AEX and EEXIT, this precludes > > > the possibility of using the untrusted stack as temporary storage by > > > enclaves. While that looks reasonable at first glance, I'm afraid it > > > isn't the case in reality. The untrusted stack is inarguably the most > > > convenient way for data exchange between an enclave and its enclosing > > > process, > > > > I vehemently disagree with "inarguably". IMO, passing data via > > registers is much more convenient. > > Which is the most convenient approach is always dependent on data size and/or even how the data is produced/consumed. It's kind of a spectrum and we're just talking in the sense of probability. You are right that "inarguably" is arguable if the buffer is small enough to fit in registers, and the producer/consumer also has access to registers. > > > > > Even if you qualify your assertion with "data of arbitrary size unknown > > at build time", I still disagree. Using the untrusted stack allows for > > trickery when a debugger is involved, other than that I see no > > advantages over allocating virtual memory and handing the pointer to the > > enclave at launch time. Sure, it requires a few more lines of code to > > setup, but it's literally ~20 LoC out of thousands required to sign, > > build and launch an enclave, but it doesn't require playing games with > > the stack. > > I'm NOT ruling out your approach. > > And like you said, the untrusted stack enables certain trickery that helps > debugging and also simplifies enclaves (even just a little). Then why are > you trying to rule that out? Because of 9 LOC in vDSO? Although its just 9 LOC, consider its impact on someone who is looking at the kernel's SGX support for the first time. Questions they may have when looking at the vDSO code/documentation: - What's an exit handler? - Why is an exit handler optional? Don't I always want to handle exits? - What value should my exit handler return? - What should my exit handler do if it detects an error? - Why would I want to preserve %rbp and not %rsp? - Isn't it insecure to use the untrusted stack in my enclave? AFAIK, the only reason to preserve %rbp instead of %rsp, i.e. support an "exit handler" callback, is to be able to implement an o-call scheme using the untrusted stack to pass data. Every idea I came up with for using the callback, e.g. logging, handling stack corruptiong, testing hooks, etc... was at worst no more difficult to implement when using a barebones vDSO. So, given the choice between a) documenting and maintaining all the baggage that comes with the exit handler and b) saying "go use signals", I chose option b.
Hi Sean, > Although its just 9 LOC, consider its impact on someone who is looking > at > the kernel's SGX support for the first time. Questions they may have > when > looking at the vDSO code/documentation: > > - What's an exit handler? > - Why is an exit handler optional? Don't I always want to handle > exits? > - What value should my exit handler return? > - What should my exit handler do if it detects an error? > - Why would I want to preserve %rbp and not %rsp? > - Isn't it insecure to use the untrusted stack in my enclave? > > AFAIK, the only reason to preserve %rbp instead of %rsp, i.e. support an > "exit handler" callback, is to be able to implement an o-call scheme > using > the untrusted stack to pass data. Every idea I came up with for using > the > callback, e.g. logging, handling stack corruptiong, testing hooks, > etc... > was at worst no more difficult to implement when using a barebones vDSO. > > So, given the choice between a) documenting and maintaining all the > baggage > that comes with the exit handler and b) saying "go use signals", I chose > option b. Disagreed! This API is NOT even x86_64 compatible and NOT intended to be used by average developers. Instead, this API will be used by SGX SDK vendors who have all the needed background/expertise. And flexibility is way more important to them than reduced documentation. Just imagine how much one needs to read to understand how SGX works, do you really think a function comprised of 20 or so LOC will be a big deal? Anyway, the documentation needed IMO will not exceed even 1 page, which will be way shorter than most of docs in kernel source tree. I'll be more than happy to help you out if that's out of your competence! Regarding maintenance, I see an API may require maintenance for 2 possible categories of reasons: 1) its interface cannot satisfy emerging applications; or 2) the infrastructure it relies on has changed. Generally speaking, a more generic API with less assumption/dependence on other components will impose lower maintenance cost in the long run. Comparing our proposals, they share the same dependences (i.e. SGX ISA and vDSO extable) but mine is more generic (as yours could be implemented using mine as a subroutine). Thus, I bet your proposal will impose higher maintenance cost in the long run. -Cedric
> On Mar 23, 2019, at 10:36 AM, Xing, Cedric <cedric.xing@intel.com> wrote: > > Hi Sean, > >> Although its just 9 LOC, consider its impact on someone who is looking >> at >> the kernel's SGX support for the first time. Questions they may have >> when >> looking at the vDSO code/documentation: >> >> - What's an exit handler? >> - Why is an exit handler optional? Don't I always want to handle >> exits? >> - What value should my exit handler return? >> - What should my exit handler do if it detects an error? >> - Why would I want to preserve %rbp and not %rsp? >> - Isn't it insecure to use the untrusted stack in my enclave? >> >> AFAIK, the only reason to preserve %rbp instead of %rsp, i.e. support an >> "exit handler" callback, is to be able to implement an o-call scheme >> using >> the untrusted stack to pass data. Every idea I came up with for using >> the >> callback, e.g. logging, handling stack corruptiong, testing hooks, >> etc... >> was at worst no more difficult to implement when using a barebones vDSO. >> >> So, given the choice between a) documenting and maintaining all the >> baggage >> that comes with the exit handler and b) saying "go use signals", I chose >> option b. > > Disagreed! > > This API is NOT even x86_64 compatible and NOT intended to be used by average developers. Instead, this API will be used by SGX SDK vendors who have all the needed background/expertise. And flexibility is way more important to them than reduced documentation. Just imagine how much one needs to read to understand how SGX works, do you really think a function comprised of 20 or so LOC will be a big deal? > > Anyway, the documentation needed IMO will not exceed even 1 page, which will be way shorter than most of docs in kernel source tree. I'll be more than happy to help you out if that's out of your competence! > > Regarding maintenance, I see an API may require maintenance for 2 possible categories of reasons: 1) its interface cannot satisfy emerging applications; or 2) the infrastructure it relies on has changed. Generally speaking, a more generic API with less assumption/dependence on other components will impose lower maintenance cost in the long run. Comparing our proposals, they share the same dependences (i.e. SGX ISA and vDSO extable) but mine is more generic (as yours could be implemented using mine as a subroutine). Thus, I bet your proposal will impose higher maintenance cost in the long run. > > I’m going to put my vDSO maintainer hat on for a minute. Cedric, your proposal has the following issues related specifically to the vDSO: It inherently contains indirect branches. This means that, on retpoline configurations, it probably needs to use retpolines. This is doable, but it’s nasty, and you need to worry about register clobbers. It uses effectively unbounded stack space. The vDSO timing functions are already a problem for Go, and this is worse. And with my vDSO hat back off, I find it disappointing that SGX SDKs seem willing to couple the SGX enclaves so tightly to their host ABIs. An *unmodified* SGX enclave should be able to run, without excessive annoyance, in a Windows process, a Linux process, a C process, a Java process, a Go process, and pretty much any other process. Saying “I’ll just recompile it” is a bad solution — for enclaves that use MRENCLAVE, you can’t, and for enclaves that use MRSIGNER, you need to deal with the fact the protecting the signing key is a big deal. Someone should be able to port the entire host program to a different language without losing secrets and without access to a signing key. Cedric, your proposal allows an enclave to muck with RSP, but not in a way that’s particularly pleasant. Since the ISA is set in stone, we can’t do anything about the enclave’s access to its caller’s registers. I would love to see a straightforward way to run an enclave such that it does not access the main untrusted stack at all — uRSP and uRBP should be arbitrary values passed in the untrusted code, and the values the enclave sets should be relayed back to the caller but otherwise not have any effect. Sadly I see no way to do this short of using GSBASE to store the real untrusted stack pointer. Other than the segment bases, there appear to be literally zero untrusted registers that are reliably preserved across an enclave entry and exit. I suppose we should use a syscall to help. Since the above tricks seem unlikely to make it into the kernel, I think we’re doing everyone a favor if the Linux APIs strongly encourage SDK authors to build enclaves in a way that they don’t make problematic assumptions about the untrusted world. I would really like to see enclaves generated by the Linux SDK work on Windows and vice versa.
Hi Andy, Thank you for your valuable feedbacks! Per what you have been saying, your feedbacks come from different angles - i.e. functionality vs. security, but they are mixed up somehow. As an effort to make the discussion more constructive going forward, I'd like you to acknowledge that, in terms of functionality, my proposal is a superset of the current patch, as I have proven by implementing Sean's API using mine as a subroutine. That said, if you are satisfied with his, you should be satisfied with mine as well, from functional perspective. And because of that, I'll try to interpret/address your concerns from security perspective unless otherwise noted. I'm aware that there's still subtle difference between Sean's API and mine - e.g. my proposal consumes 24 bytes more stack space (for the same functionality, i.e. exit callback is null) than his, due to the additional parameters. But I don't believe that would become a "make it or break it" situation in practice. > I’m going to put my vDSO maintainer hat on for a minute. Cedric, your > proposal has the following issues related specifically to the vDSO: > > It inherently contains indirect branches. This means that, on retpoline > configurations, it probably needs to use retpolines. This is doable, > but it’s nasty, and you need to worry about register clobbers. Only the weakest link matters in security. With dynamic linking in use, this additional indirect CALL can't make things worse. But I'm open to, and in fact also willing to, apply whatever mitigation that you think is satisfactory (or that has been applied to other indirect branches, such as in PLT), such as retpoline. Btw, don't worry about register clobbers because we have at least %rax at our disposal. > > It uses effectively unbounded stack space. The vDSO timing functions are > already a problem for Go, and this is worse. If targeting the same functionality (i.e. no exit callback), my API uses exactly 24 bytes more than Sean's. Is it really the case that those 24 bytes will break Go? > > And with my vDSO hat back off, I find it disappointing that SGX SDKs > seem willing to couple the SGX enclaves so tightly to their host ABIs. > An *unmodified* SGX enclave should be able to run, without excessive > annoyance, in a Windows process, a Linux process, a C process, a Java > process, a Go process, and pretty much any other process. Saying “I’ll > just recompile it” is a bad solution — for enclaves that use MRENCLAVE, > you can’t, and for enclaves that use MRSIGNER, you need to deal with the > fact the protecting the signing key is a big deal. > Someone should be able to port the entire host program to a different > language without losing secrets and without access to a signing key. I'm not sure which SGX SDKs you are referring to. But for Intel SGX SDK, we defined our own ABI that is consistent between Windows and Linux - i.e. there's no technical problem to load on Windows an enclave built on Linux or vice versa. In terms of what programming languages they can work with, I have to say it was designed exclusively for C/C++. Fortunately, there's usually a "native" interface (e.g. JNI, cgo, etc.) supported by a language runtime so it hasn't been a roadblock so far. Alternatively, the enclave vendor could ship an enclave along with an "interface" shared object that encapsulates all of the marshaling specifics, then the combination of that enclave and its "interface" shared object may be able to work "universally", which should be close to what you want. The idea we had, when Intel SGX SDK was designed, was that different SDKs would be developed for different languages to take advantage of specific language features. That is similar to different programming languages were invented to target different usages. As we all know, every programming language has both advantages and disadvantages, hence no single language dominates. And that same idea applies to SGX SDKs. If there existed an SDK that worked with everything, probably it wouldn't work well with anything. > > Cedric, your proposal allows an enclave to muck with RSP, but not in a > way that’s particularly pleasant. From security perspective, it is SGX ISA, but NOT any particular ABI, that allows enclaves "to muck with RSP". > Since the ISA is set in stone, we > can’t do anything about the enclave’s access to its caller’s registers. > I would love to see a straightforward way to run an enclave such that it > does not access the main untrusted stack at all — uRSP and uRBP should > be arbitrary values passed in the untrusted code, and the values the > enclave sets should be relayed back to the caller but otherwise not have > any effect. Sadly I see no way to do this short of using GSBASE to > store the real untrusted stack pointer. I understand your sadness. You are "hoping" SGX to be a sandbox technology (i.e. to prevent enclave from reaching out into the host) but that wasn't the security objective when SGX was defined. Anyway, SGX is what it is. A restrictive ABI only takes away flexibilities from "good" enclaves but can NEVER restrict malicious ones, so Sean's ABI cannot offer what you want. > Other than the segment bases, there appear to be literally zero > untrusted registers that are reliably preserved across an enclave entry > and exit. I suppose we should use a syscall to help. The good news is with CET, there are viable solutions to implement bi-directional protection as you would hope. You are more than welcome to ask me offline for more details. > > Since the above tricks seem unlikely to make it into the kernel, I think > we’re doing everyone a favor if the Linux APIs strongly encourage SDK > authors to build enclaves in a way that they don’t make problematic > assumptions about the untrusted world. I would really like to see > enclaves generated by the Linux SDK work on Windows and vice versa. As said in my previous email, this vDSO API isn't even compliant to x86_64 ABI and is absolutely NOT for average developers. Instead, host/enclave communications are expected to be handled by SDKs and those developers will be very aware of the limitations of their targeted environments, and will need the freedom to deploy optimal solutions. I understand your intention to advocate the programming model that you believe is "right". But there are 7 billion people on this planet and the "right" thing for you could be "wrong" for others, especially in future usages/situations that can't be foreseen today. Software is stacked, with the lower layers being more generic and higher layers being more specific. This vDSO API is sitting at the bottom of the stack, therefore shall be as generic as possible. A better approach to advocate your idea is to wrap it (i.e. to implement it using the more generic vDSO API as a subroutine) in a library for the public to choose (and you can imagine others bearing different ideas will do the same). Then good ideas will stand out! -Cedric
On Sun, Mar 24, 2019 at 01:59:48AM -0700, Xing, Cedric wrote: > As said in my previous email, this vDSO API isn't even compliant to > x86_64 ABI and is absolutely NOT for average developers. Instead, > host/enclave communications are expected to be handled by SDKs and > those developers will be very aware of the limitations of their targeted > environments, and will need the freedom to deploy optimal solutions. This statement epitomizes the difference in philosophies between Intel's SGX SDK and much of the Linux community. The SDK, from edger8r to its stack stitching, believes that it should dumb things down for the so called "average" developer, even if doing so significantly increases the complexity of the implementation. Linux generally favors the opposite approach, preferring simplicitly in its implementations even if it means setting a higher bar for the "average" developer/user. That's not to say that that Linux doesn't have its fair share of complex code or dumbed down interfaces, far from it. But generally speaking, there is an emphasis on making the code itself approachable that I don't see in Intel's SDK. > I understand your intention to advocate the programming model that you > believe is "right". But there are 7 billion people on this planet and > the "right" thing for you could be "wrong" for others, especially in > future usages/situations that can't be foreseen today. IMO modifying SSA.U_R{B,SP} from within the enclave is akin to modifying VMCS fields from the guest, which is technically possible on processors that don't support VMCS caching, but I digress... The above statements aren't intended to fan the flames or send us down a rat hole of philosophical arguments. My intent is to point out that I think we're at an impasse due to philosophical differences, i.e. no amount of arguing will convince me that mucking with the untrusted stack is anything short of insanity, and vice versa I doubt that Andy, I or anyone else will convince you and the SDK team that forcing the SDK to use an alternative form of enclave communication is a good thing. > Software is stacked, with the lower layers being more generic and higher > layers being more specific. This vDSO API is sitting at the bottom of > the stack, therefore shall be as generic as possible. A better approach > to advocate your idea is to wrap it (i.e. to implement it using the more > generic vDSO API as a It's not more generic, just different, e.g. uses %rbp instead of %rsp as the anchor. Or am I missing something? > subroutine) in a library for the public to choose (and you can imagine > others bearing different ideas will do the same). Then good ideas will > stand out! I'd rather support two disparate vDSO implementations than implement the barebones ABI as a wrapper to something heavier. E.g. I really don't want to have to wade through a bunch of conditionals and stack accesses, (or save/restore code if you're talking about making it compliant with the x86_64 ABI) when I inevitably break something during kernel development. Given all above, and that everyone involved wants to see SGX get accepted into mainline sooner rather than later, I propose the following: - Keep the barebones __vdso_sgx_enter_enclave() as is - Explicitly state in the SGX documentation that userspace does *not* have to use the vDSO, e.g. can enter enclaves directly and use signals - Submit a patch to add second vDSO function to support the Intel SDK use case (from Cedric or anyone from the SDK team) I fully realize that the above approach saddles Cedric and the SDK team with the extra task of justifying the need for two vDSO interfaces, and likely reduces the probability of their proposal being accepted. But, we don't *force* the SDK to be rewritten, and we gain a vDSO interface that many people want and is acceptable to the maintainers (unless I've horribly misread Andy's position).
On Sun, Mar 24, 2019 at 1:59 AM Xing, Cedric <cedric.xing@intel.com> wrote: > > Hi Andy, > > Thank you for your valuable feedbacks! > > Per what you have been saying, your feedbacks come from different angles - i.e. functionality vs. security, but they are mixed up somehow. I think you're misunderstanding me. I'm not talking about security at all here. SGX isn't a sandbox, full stop. I'm talking about the degree to which an SGX enclave acts like a well-behaved black box. > > > I’m going to put my vDSO maintainer hat on for a minute. Cedric, your > > proposal has the following issues related specifically to the vDSO: > > > > It inherently contains indirect branches. This means that, on retpoline > > configurations, it probably needs to use retpolines. This is doable, > > but it’s nasty, and you need to worry about register clobbers. > > Only the weakest link matters in security. With dynamic linking in use, this additional indirect CALL can't make things worse. But I'm open to, and in fact also willing to, apply whatever mitigation that you think is satisfactory (or that has been applied to other indirect branches, such as in PLT), such as retpoline. Btw, don't worry about register clobbers because we have at least %rax at our disposal. There is no actual fundamental reason that dynamic linking has to work this way, and in principle, one could even use retpolines to the call the vDSO. In any event, the vDSO is currently compiled with retpolines enabled, and if we decide to turn that off, it would be decision to be made independently of SGX. > > > > > It uses effectively unbounded stack space. The vDSO timing functions are > > already a problem for Go, and this is worse. > > If targeting the same functionality (i.e. no exit callback), my API uses exactly 24 bytes more than Sean's. Is it really the case that those 24 bytes will break Go? You're counting wrong. Your version uses 24 bytes + the stack size of the exit handler + the amount of stack consumed by the enclave, which is effectively unbounded. So this whole scheme becomes unusable on anything other than a stack that is "large" for a totally undefined value of large and that has guard pages. > > > > > Cedric, your proposal allows an enclave to muck with RSP, but not in a > > way that’s particularly pleasant. > > From security perspective, it is SGX ISA, but NOT any particular ABI, that allows enclaves "to muck with RSP". Again, this has nothing to do with security. With your proposal, it's not possible for the caller of an enclave to decide, in an ocall handler, to pause and do something else. This isn't just theoretical. Suppose someone wants to send a network request in an ocall handler. With the current RSP approach, it's difficult to do this in a program that uses poll / select / epoll -- you can't return out from the ocall until you have an answer.
On Mon, Mar 25, 2019 at 11:03 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Sun, Mar 24, 2019 at 01:59:48AM -0700, Xing, Cedric wrote: > > As said in my previous email, this vDSO API isn't even compliant to > > x86_64 ABI and is absolutely NOT for average developers. Instead, > > host/enclave communications are expected to be handled by SDKs and > > those developers will be very aware of the limitations of their targeted > > environments, and will need the freedom to deploy optimal solutions. > I fully realize that the above approach saddles Cedric and the SDK team > with the extra task of justifying the need for two vDSO interfaces, and > likely reduces the probability of their proposal being accepted. But, we > don't *force* the SDK to be rewritten, and we gain a vDSO interface that > many people want and is acceptable to the maintainers (unless I've > horribly misread Andy's position). I don't think you've horribly misread it. I would like to keep the stuff in the vDSO as minimal as possible. If we need to add a fancier interface down the line, then that's fine.
> I think you're misunderstanding me. I'm not talking about security at > all here. SGX isn't a sandbox, full stop. I'm talking about the degree > to which an SGX enclave acts like a well-behaved black box. Any meaningful communication requires an agreement in place. The host and the enclave could be either in agreement, or not in an agreement. In the former case, the enclave will behave while in the latter case it will misbehave. The thing is, if an agreement between them says - "Don't you enclave touch the stack", and if the enclave behaves, then it wouldn't touch the stack; or the enclave misbehaves, then that "agreement" CANNOT stop it from doing so, REGARDLESS what that agreement is. The point is, an agreement must exist for host/enclave communication. The ABI limits what kinds of agreements they can bind to, but can NEVER enforce an agreement. The difference between Sean's ABI and mine is that mine is more relaxing (i.e. allows larger variety of agreements) but otherwise identical functionally. I truly hope you can understand that. > > > > > > I’m going to put my vDSO maintainer hat on for a minute. Cedric, > > > your proposal has the following issues related specifically to the > vDSO: > > > > > > It inherently contains indirect branches. This means that, on > > > retpoline configurations, it probably needs to use retpolines. This > > > is doable, but it’s nasty, and you need to worry about register > clobbers. > > > > Only the weakest link matters in security. With dynamic linking in > use, this additional indirect CALL can't make things worse. But I'm open > to, and in fact also willing to, apply whatever mitigation that you > think is satisfactory (or that has been applied to other indirect > branches, such as in PLT), such as retpoline. Btw, don't worry about > register clobbers because we have at least %rax at our disposal. > > There is no actual fundamental reason that dynamic linking has to work > this way, and in principle, one could even use retpolines to the call > the vDSO. In any event, the vDSO is currently compiled with retpolines > enabled, and if we decide to turn that off, it would be decision to be > made independently of SGX. Don't get me wrong! I'm just saying dynamic linking requires indirect branches so whatever mitigation used there can also apply here. I'm willing to implement the same mechanism as generally accepted in other occasions. If retpoline is the one then I will just do it. Btw, retpoline won't work with CET though. > > > > > > > > > It uses effectively unbounded stack space. The vDSO timing functions > > > are already a problem for Go, and this is worse. > > > > If targeting the same functionality (i.e. no exit callback), my API > uses exactly 24 bytes more than Sean's. Is it really the case that those > 24 bytes will break Go? > > You're counting wrong. Your version uses 24 bytes + the stack size of > the exit handler + the amount of stack consumed by the enclave, which is > effectively unbounded. So this whole scheme becomes unusable on > anything other than a stack that is "large" for a totally undefined > value of large and that has guard pages. You misread. I said "targeting the same functionality", meaning no exit callback is used (because Sean's ABI doesn't support it). And because no callbacks will be made, only 24 more bytes will be needed. And at this point I'm trying to stress the fact that my proposal is a superset of Sean's in terms of functionality - i.e. my proposal can do all that Sean's can do. For example, if the enclave is coded NOT to use the untrusted stack, e.g. in Fortanix's case, then it won't use the stack and the callback is unnecessary (and shall be set to NULL). That is, mine will work exactly the same as Sean's in the case of enclave not touching the stack. My apology for being excessively verbose here but your comment above prompts me that you may NOT have realized that my proposal will work exactly the SAME as Sean's when exit callback is absent (NULL). > > > > > > > > > Cedric, your proposal allows an enclave to muck with RSP, but not in > > > a way that’s particularly pleasant. > > > > From security perspective, it is SGX ISA, but NOT any particular ABI, > that allows enclaves "to muck with RSP". > > Again, this has nothing to do with security. With your proposal, it's > not possible for the caller of an enclave to decide, in an ocall > handler, to pause and do something else. This isn't just theoretical. > Suppose someone wants to send a network request in an ocall handler. > With the current RSP approach, it's difficult to do this in a program > that uses poll / select / epoll -- you can't return out from the ocall > until you have an answer. Andy, your comment here further confirms that you have NOT understood my proposal. In the case the o-call parameters are passed in registers (or separate buffers), exit handler is NOT needed (and should be set to NULL), and the API will just return at EEXIT, and then the caller can dispatch the o-call solely based on register values, which is exactly the SAME as Sean's proposal. An exit handler is NOT necessary to support o-calls. It is needed ONLY if the host/enclave exchanges data on the stack. My apology again for repeating myself, but my proposal is a SUPERSET of Sean's from functional perspective, and it will work in EXACTLY the SAME way as Sean's in ALL aspects when exit callback is absent. So you really have NOTHING to worry about from functional stand point! And as you said you don't worry about security either, then you really have NOTHING to worry about! -Cedric
> On Mon, Mar 25, 2019 at 11:03 AM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Sun, Mar 24, 2019 at 01:59:48AM -0700, Xing, Cedric wrote: > > > As said in my previous email, this vDSO API isn't even compliant to > > > x86_64 ABI and is absolutely NOT for average developers. Instead, > > > host/enclave communications are expected to be handled by SDKs and > > > those developers will be very aware of the limitations of their > > > targeted environments, and will need the freedom to deploy optimal > solutions. > > > I fully realize that the above approach saddles Cedric and the SDK > > team with the extra task of justifying the need for two vDSO > > interfaces, and likely reduces the probability of their proposal being > > accepted. But, we don't *force* the SDK to be rewritten, and we gain > > a vDSO interface that many people want and is acceptable to the > > maintainers (unless I've horribly misread Andy's position). > > I don't think you've horribly misread it. I would like to keep the > stuff in the vDSO as minimal as possible. If we need to add a fancier > interface down the line, then that's fine. Andy, I don't know "many people" is how many in Sean's email. I couldn't tell you how long it took us to settle on the current SGX ISA because you would just LAUGH! Why? Because it took insanely ridiculously long. Why that long? Because the h/w and u-code teams would like to trim down the ISA as much as possible. The fact is, whatever new is released, Intel will have to maintain it on all future processors FOREVER! That means significant/on-going cost to Intel. So any addition to ISA has to undergo extensive reviews that involve all kinds of experts from both within Intel and externally, and would usually take years, before you can see what you are seeing today. As I said in my earlier emails, RBP is NOT needed for interrupt/exception handlers, then how did RBP end up being restored at AEX? You can guess how many people were standing behind it! Sean has no clue! I can assure you! Guess we've talked enough on the technical front. So let's talk about it on the business front. Let me take a step back. Let's say you are right, all enclaves would eventually be coded in the way you want. We (Intel SDK team) were convinced to follow your approach. But there were existing enclaves and a migration path would be needed. Would you like to support us? It'd be only 9 LOC on your side but how much would incur on our side? If you believe you are doing right thing, then acceptance is the next thing you should think of. You should offer an easy path for those who did "wrong" to get on your "right" boat. Don't you think so? -Cedric
On Mon, Mar 25, 2019 at 9:53 PM Xing, Cedric <cedric.xing@intel.com> wrote: > > > On Mon, Mar 25, 2019 at 11:03 AM Sean Christopherson > > <sean.j.christopherson@intel.com> wrote: > > > > > > On Sun, Mar 24, 2019 at 01:59:48AM -0700, Xing, Cedric wrote: > > > > As said in my previous email, this vDSO API isn't even compliant to > > > > x86_64 ABI and is absolutely NOT for average developers. Instead, > > > > host/enclave communications are expected to be handled by SDKs and > > > > those developers will be very aware of the limitations of their > > > > targeted environments, and will need the freedom to deploy optimal > > solutions. > > > > > I fully realize that the above approach saddles Cedric and the SDK > > > team with the extra task of justifying the need for two vDSO > > > interfaces, and likely reduces the probability of their proposal being > > > accepted. But, we don't *force* the SDK to be rewritten, and we gain > > > a vDSO interface that many people want and is acceptable to the > > > maintainers (unless I've horribly misread Andy's position). > > > > I don't think you've horribly misread it. I would like to keep the > > stuff in the vDSO as minimal as possible. If we need to add a fancier > > interface down the line, then that's fine. > > Andy, I don't know "many people" is how many in Sean's email. I couldn't tell you how long it took us to settle on the current SGX ISA because you would just LAUGH! Why? Because it took insanely ridiculously long. Why that long? Because the h/w and u-code teams would like to trim down the ISA as much as possible. The fact is, whatever new is released, Intel will have to maintain it on all future processors FOREVER! That means significant/on-going cost to Intel. So any addition to ISA has to undergo extensive reviews that involve all kinds of experts from both within Intel and externally, and would usually take years, before you can see what you are seeing today. As I said in my earlier emails, RBP is NOT needed for interrupt/exception handlers, then how did RBP end up being restored at AEX? You can guess how many people were standing behind it! Sean has no clue! I can assure you! > > Guess we've talked enough on the technical front. So let's talk about it on the business front. Let me take a step back. Let's say you are right, all enclaves would eventually be coded in the way you want. We (Intel SDK team) were convinced to follow your approach. But there were existing enclaves and a migration path would be needed. Would you like to support us? It'd be only 9 LOC on your side but how much would incur on our side? If you believe you are doing right thing, then acceptance is the next thing you should think of. You should offer an easy path for those who did "wrong" to get on your "right" boat. Don't you think so? > I suppose the real question is: are there a significant number of users who will want to run enclaves created using an old SDK on Linux? And will there actually be support for doing this in the software stack? If the answer to both questions is yes, then it seems like it could be reasonable to support it in the vDSO. But I still think it should probably be a different vDSO entry point so that the normal case doesn't become more complicated.
Hi Andy, > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > owner@vger.kernel.org] On Behalf Of Andy Lutomirski > > I suppose the real question is: are there a significant number of > users who will want to run enclaves created using an old SDK on Linux? > And will there actually be support for doing this in the software > stack? To your first question, I cannot share information of Intel customers or speak for them. But in general, people would like to stay with an old enclave usually because of: 1) attestation, because MRENCLAVE will change after rebuild; and/or 2) the need to support a mix of older and newer Linux kernels. So I'd say it'll be commonly desired, especially when this vDSO API is still "new" (so not available on every platform). To your second question, Intel will support all "legacy" enclaves built with older SGX SDKs on newer kernels. And that's why we are so eager to find a migration path. I can't speak for other companies, but guess backward compatibility is always desirable. > > If the answer to both questions is yes, then it seems like it could be > reasonable to support it in the vDSO. But I still think it should > probably be a different vDSO entry point so that the normal case > doesn't become more complicated. I'll support whatever you think is more appropriate. At the end, I'd like to give out the full version of my proposal, with your feedbacks (i.e. stack unwinder and Spectre variant 2) addressed. I'm a bit concerned by retpoline, which won't work (or be needed) when CET comes online. Are you looking to change it again then? Here's the summary of the changes: - Added CFI directives for proper unwinding. - Defined sgx_ex_callback - the callback function on enclave exit/exception. - Aligned stack properly before calling sgx_ex_callback (per x86_64 ABI). - Used retpoline in place of indirect call. - The block starting at label "4:" captures all the code necessary to support sgx_ex_call. It has grown longer due to retpoline. /** * __vdso_sgx_enter_enclave() - Enter an SGX enclave * * %eax: ENCLU leaf, must be either EENTER or ERESUME * 0x08(%rsp): TCS * 0x10(%rsp): Optional pointer to 'struct sgx_enclave_exception' * 0x18(%rsp): Optional function pointer to 'sgx_ex_callback', whose * definition will be given below. Note that this function, if * present, shall follow x86_64 ABI. * return: 0 (zero) on success, or a negative error code on failure. * * Note that __vdso_sgx_enter_enclave() is not compatible with x86_64 ABI. * All registers except RBP must be treated as volatile from the caller's * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... * Enclave may decrement RSP, but must not increment it - i.e. existing content * of the stack shall be preserved. * * sgx_ex_callback - A callback function to be invoked by * __vdso_sgx_enter_enclave() upon exception or after the enclave exits. * * typedef int (*sgx_ex_callback)(long rdi, long rsi, long rdx, * struct sgx_enclave_exception *ex_info, long r8, long r9, * long rsp, void *tcs); * * Note that sgx_ex_callback shall be x86_64 ABI compliant. * * Note that other GPRs (except %rax, %rbx and %rcx) are also passed through to * sgx_ex_callback, even though accessing them requires assembly code. */ __vdso_sgx_enter_enclave: /* prolog */ .cfi_startproc push %rbp .cfi_adjust_cfa_offset 8 .cfi_rel_offset %rbp, 0 mov %rsp, %rbp .cfi_def_cfa_register %rbp 1: /* EENTER <= leaf <= ERESUME */ cmp $0x2, %eax jb 5f cmp $0x3, %eax ja 5f /* Load TCS and AEP */ mov 0x10(%rbp), %rbx lea 2f(%rip), %rcx 2: enclu /* EEXIT path */ mov 0x18(%rbp), %rcx jrcxz 3f mov %eax, EX_LEAF(%rcx) /* normalize return value */ 3: xor %eax, %eax 4: /* call sgx_ex_callback if supplied */ cmpq $0, 0x20(%rbp) jz 6f /* align stack per x86_64 ABI */ mov %rsp, %rbx and $-0x10, %rsp /* parameters */ push 0x10(%rbp) push %rbx /* call *0x20(%rbp) using retpoline */ mov 0x20(%rbp), %rax call 41f /* stack cleanup */ mov %rbx, %rsp jmp 1b 41: call 43f 42: pause lfence jmp 42b 43: mov %rax, (%rsp) ret 5: /* bad leaf */ cmp $0, %eax jle 6f mov $(-EINVAL), %eax 6: /* epilog */ leave .cfi_def_cfa %rsp, 8 ret .cfi_endproc .pushsection .fixup, "ax" 7: mov 0x18(%rbp), %rcx jrcxz 8f /* fill in ex_info */ mov %eax, EX_LEAF(%rcx) mov %di, EX_TRAPNR(%rcx) mov %si, EX_ERROR_CODE(%rcx) mov %rdx, EX_ADDRESS(%rcx) 8: mov $(-EFAULT), %eax jmp 4b .popsection _ASM_VDSO_EXTABLE_HANDLE(2b, 7b) -Cedric
On Wed, Mar 27, 2019 at 9:23 PM Xing, Cedric <cedric.xing@intel.com> wrote: > > Hi Andy, > > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > > owner@vger.kernel.org] On Behalf Of Andy Lutomirski > > > > I suppose the real question is: are there a significant number of > > users who will want to run enclaves created using an old SDK on Linux? > > And will there actually be support for doing this in the software > > stack? > > To your first question, I cannot share information of Intel customers or speak for them. But in general, people would like to stay with an old enclave usually because of: 1) attestation, because MRENCLAVE will change after rebuild; and/or 2) the need to support a mix of older and newer Linux kernels. So I'd say it'll be commonly desired, especially when this vDSO API is still "new" (so not available on every platform). > > To your second question, Intel will support all "legacy" enclaves built with older SGX SDKs on newer kernels. And that's why we are so eager to find a migration path. I can't speak for other companies, but guess backward compatibility is always desirable. > > > > > If the answer to both questions is yes, then it seems like it could be > > reasonable to support it in the vDSO. But I still think it should > > probably be a different vDSO entry point so that the normal case > > doesn't become more complicated. > > I'll support whatever you think is more appropriate. > > At the end, I'd like to give out the full version of my proposal, with your feedbacks (i.e. stack unwinder and Spectre variant 2) addressed. I'm a bit concerned by retpoline, which won't work (or be needed) when CET comes online. Are you looking to change it again then? The kernel is buildable with and without retpolines. Presumably the addition of CET support will need to address this everywhere, including in the vDSO. > > Here's the summary of the changes: > - Added CFI directives for proper unwinding. > - Defined sgx_ex_callback - the callback function on enclave exit/exception. > - Aligned stack properly before calling sgx_ex_callback (per x86_64 ABI). > - Used retpoline in place of indirect call. > - The block starting at label "4:" captures all the code necessary to support sgx_ex_call. It has grown longer due to retpoline. > > /** > * __vdso_sgx_enter_enclave() - Enter an SGX enclave > * > * %eax: ENCLU leaf, must be either EENTER or ERESUME > * 0x08(%rsp): TCS > * 0x10(%rsp): Optional pointer to 'struct sgx_enclave_exception' > * 0x18(%rsp): Optional function pointer to 'sgx_ex_callback', whose > * definition will be given below. Note that this function, if > * present, shall follow x86_64 ABI. > * return: 0 (zero) on success, or a negative error code on failure. > * > * Note that __vdso_sgx_enter_enclave() is not compatible with x86_64 ABI. > * All registers except RBP must be treated as volatile from the caller's > * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... > * Enclave may decrement RSP, but must not increment it - i.e. existing content > * of the stack shall be preserved. > * > * sgx_ex_callback - A callback function to be invoked by > * __vdso_sgx_enter_enclave() upon exception or after the enclave exits. > * > * typedef int (*sgx_ex_callback)(long rdi, long rsi, long rdx, > * struct sgx_enclave_exception *ex_info, long r8, long r9, > * long rsp, void *tcs); > * > * Note that sgx_ex_callback shall be x86_64 ABI compliant. > * > * Note that other GPRs (except %rax, %rbx and %rcx) are also passed through to > * sgx_ex_callback, even though accessing them requires assembly code. > */ > __vdso_sgx_enter_enclave: > /* prolog */ > .cfi_startproc > push %rbp > .cfi_adjust_cfa_offset 8 > .cfi_rel_offset %rbp, 0 > mov %rsp, %rbp > .cfi_def_cfa_register %rbp > > 1: /* EENTER <= leaf <= ERESUME */ > cmp $0x2, %eax > jb 5f > cmp $0x3, %eax > ja 5f > > /* Load TCS and AEP */ > mov 0x10(%rbp), %rbx > lea 2f(%rip), %rcx > > 2: enclu > > /* EEXIT path */ > mov 0x18(%rbp), %rcx > jrcxz 3f > mov %eax, EX_LEAF(%rcx) > /* normalize return value */ > 3: xor %eax, %eax > > 4: /* call sgx_ex_callback if supplied */ > cmpq $0, 0x20(%rbp) > jz 6f > /* align stack per x86_64 ABI */ > mov %rsp, %rbx > and $-0x10, %rsp > /* parameters */ > push 0x10(%rbp) > push %rbx > /* call *0x20(%rbp) using retpoline */ > mov 0x20(%rbp), %rax > call 41f > /* stack cleanup */ > mov %rbx, %rsp > jmp 1b > 41: call 43f > 42: pause > lfence > jmp 42b > 43: mov %rax, (%rsp) > ret > > 5: /* bad leaf */ > cmp $0, %eax > jle 6f > mov $(-EINVAL), %eax > > 6: /* epilog */ > leave > .cfi_def_cfa %rsp, 8 > ret > .cfi_endproc > > .pushsection .fixup, "ax" > 7: mov 0x18(%rbp), %rcx > jrcxz 8f > /* fill in ex_info */ > mov %eax, EX_LEAF(%rcx) > mov %di, EX_TRAPNR(%rcx) > mov %si, EX_ERROR_CODE(%rcx) > mov %rdx, EX_ADDRESS(%rcx) > 8: mov $(-EFAULT), %eax > jmp 4b > .popsection > > _ASM_VDSO_EXTABLE_HANDLE(2b, 7b) > > It's certainly making progress. I like the fact that the callback is now unconditional (if non-NULL) rather than being used just in case of certain exit types. But, if we go down this route, let's name and document it appropriately -- just call the function "callback" and document it as a function that is called just before __vdso_sgx_enter_enclave returns, to be used if support for legacy enclaves that push data onto the untrusted stack is needed. We should further document that it's safe to longjmp out of it. Also, the tests in tools/testing/selftests/x86/unwind_vdso.c should be augmented to test this code. Finally, why does the vDSO code bother checking whether the leaf is valid?
> It's certainly making progress. I like the fact that the callback is > now unconditional (if non-NULL) rather than being used just in case of > certain exit types. But, if we go down this route, let's name and > document it appropriately -- just call the function "callback" and > document it as a function that is called just before > __vdso_sgx_enter_enclave returns, to be used if support for legacy > enclaves that push data onto the untrusted stack is needed. We should > further document that it's safe to longjmp out of it. > > Also, the tests in tools/testing/selftests/x86/unwind_vdso.c should be > augmented to test this code. > > Finally, why does the vDSO code bother checking whether the leaf is > valid? I can document it. I'll look into unwind_vdso.c to see what kind of selftests will make sense here. And I'll send out a RFC patch with everything included. Or would you prefer to have my changes integrated into Jarkko's patch v20? Different ENCLU leaf has different parameters. This vDSO API knows how to load up parameters only for EENTER and ERESUME so it errs on all other positive values. 0 and negative values are interpreted as return codes.
On Thu, Mar 28, 2019 at 11:19:25PM +0000, Xing, Cedric wrote: > > It's certainly making progress. I like the fact that the callback is > > now unconditional (if non-NULL) rather than being used just in case of > > certain exit types. But, if we go down this route, let's name and > > document it appropriately -- just call the function "callback" and > > document it as a function that is called just before > > __vdso_sgx_enter_enclave returns, to be used if support for legacy > > enclaves that push data onto the untrusted stack is needed. We should > > further document that it's safe to longjmp out of it. > > > > Also, the tests in tools/testing/selftests/x86/unwind_vdso.c should be > > augmented to test this code. > > > > Finally, why does the vDSO code bother checking whether the leaf is > > valid? > > I can document it. I'll look into unwind_vdso.c to see what kind of > selftests will make sense here. And I'll send out a RFC patch with > everything included. Or would you prefer to have my changes integrated > into Jarkko's patch v20? > > Different ENCLU leaf has different parameters. This vDSO API knows how > to load up parameters only for EENTER and ERESUME so it errs on all > other positive values. 0 and negative values are interpreted as return > codes. Not gonna make it to v20. I'm aiming to send it early next week and have already closed the content. What this and also Sean's solution would need is the update to Documentation/x86/sgx.rst explaining how it works in detail. /Jarkko
On Fri, Mar 29, 2019 at 11:48:14AM +0200, Jarkko Sakkinen wrote: Good morning to everyone, I hope the weekend is going well. > Not gonna make it to v20. I'm aiming to send it early next week and have > already closed the content. What is the status of your jarkko-sgx/next and jarkko-sgx/master branches with respect to all of this? We have our SFLC patch series currently staged on top of jarkko-sgx/next and we will re-stage them on top of whatever you are pushing upstream from. > /Jarkko Have a good remainder of the weekend. Dr. Greg As always, Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC. 4206 N. 19th Ave. Specializing in information infra-structure Fargo, ND 58102 development. PH: 701-281-1686 FAX: 701-281-3949 EMAIL: greg@enjellic.com ------------------------------------------------------------------------------ "Can't they? A 64bit number incremented every millisecond can grow for half a billion years. As far as I'm concerned, that is forever." -- Neil Brown linux-raid
On Thu, Mar 28, 2019 at 12:18:40PM -0700, Andy Lutomirski wrote:
> Finally, why does the vDSO code bother checking whether the leaf is valid?
To sanity check the input to ensure the caller is attempting to enter an
enclave, i.e. the function is named __vdso_sgx_enter_enclave(), not
__vsgx_enclu(). And if the caller does pass the wrong leaf, EINVAL will
be easier to debug than EFAULT, especially on SGX1 hardware where the #GP
could be misconstrued as a loss of EPC.
I'm not dead set on the checks by any means, but it does seem appropriate.
And the cost is all of 3 uops and a few bytes of a code.
diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile index 34bcf87d358c..fb5b9960b192 100644 --- a/arch/x86/entry/vdso/Makefile +++ b/arch/x86/entry/vdso/Makefile @@ -18,6 +18,7 @@ VDSO32-$(CONFIG_IA32_EMULATION) := y # files to link into the vdso vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o +vobjs-$(VDSO64-y) += vsgx_enter_enclave.o # files to link into kernel obj-y += vma.o extable.o @@ -85,6 +86,7 @@ CFLAGS_REMOVE_vdso-note.o = -pg CFLAGS_REMOVE_vclock_gettime.o = -pg CFLAGS_REMOVE_vgetcpu.o = -pg CFLAGS_REMOVE_vvar.o = -pg +CFLAGS_REMOVE_vsgx_enter_enclave.o = -pg # # X32 processes use x32 vDSO to access 64bit kernel data. diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S index d3a2dce4cfa9..50952a995a6c 100644 --- a/arch/x86/entry/vdso/vdso.lds.S +++ b/arch/x86/entry/vdso/vdso.lds.S @@ -25,6 +25,7 @@ VERSION { __vdso_getcpu; time; __vdso_time; + __vdso_sgx_enter_enclave; local: *; }; } diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S new file mode 100644 index 000000000000..af572adcd8ed --- /dev/null +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S @@ -0,0 +1,97 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include <linux/linkage.h> +#include <asm/export.h> +#include <asm/errno.h> + +#include "extable.h" + +#define EX_LEAF 0*8 +#define EX_TRAPNR 0*8+4 +#define EX_ERROR_CODE 0*8+6 +#define EX_ADDRESS 1*8 + +.code64 +.section .text, "ax" + +/** + * __vdso_sgx_enter_enclave() - Enter an SGX enclave + * + * %eax: ENCLU leaf, must be EENTER or ERESUME + * %rbx: TCS, must be non-NULL + * %rcx: Optional pointer to 'struct sgx_enclave_exception' + * + * Return: + * 0 on a clean entry/exit to/from the enclave + * -EINVAL if ENCLU leaf is not allowed or if TCS is NULL + * -EFAULT if ENCLU or the enclave faults + * + * Note that __vdso_sgx_enter_enclave() is not compliant with the x86-64 ABI. + * All registers except RSP must be treated as volatile from the caller's + * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... + * Conversely, the enclave being run must preserve the untrusted RSP and stack. + * + * __vdso_sgx_enter_enclave(u32 leaf, void *tcs, + * struct sgx_enclave_exception *exception_info) + * { + * if (leaf != SGX_EENTER && leaf != SGX_ERESUME) + * return -EINVAL; + * + * if (!tcs) + * return -EINVAL; + * + * try { + * ENCLU[leaf]; + * } catch (exception) { + * if (e) + * *e = exception; + * return -EFAULT; + * } + * + * return 0; + * } + */ +ENTRY(__vdso_sgx_enter_enclave) + /* EENTER <= leaf <= ERESUME */ + cmp $0x2, %eax + jb bad_input + + cmp $0x3, %eax + ja bad_input + + /* TCS must be non-NULL */ + test %rbx, %rbx + je bad_input + + /* Save @exception_info */ + push %rcx + + /* Load AEP for ENCLU */ + lea 1f(%rip), %rcx +1: enclu + + add $0x8, %rsp + xor %eax, %eax + ret + +bad_input: + mov $(-EINVAL), %rax + ret + +.pushsection .fixup, "ax" + /* Re-load @exception_info and fill it (if it's non-NULL) */ +2: pop %rcx + test %rcx, %rcx + je 3f + + mov %eax, EX_LEAF(%rcx) + mov %di, EX_TRAPNR(%rcx) + mov %si, EX_ERROR_CODE(%rcx) + mov %rdx, EX_ADDRESS(%rcx) +3: mov $(-EFAULT), %rax + ret +.popsection + +_ASM_VDSO_EXTABLE_HANDLE(1b, 2b) + +ENDPROC(__vdso_sgx_enter_enclave) diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 150a784db395..76834456ab07 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -69,4 +69,22 @@ struct sgx_enclave_set_attribute { __u64 attribute_fd; }; +/** + * struct sgx_enclave_exception - structure to report exceptions encountered in + * __vdso_sgx_enter_enclave() + * + * @leaf: ENCLU leaf from %rax at time of exception + * @trapnr: exception trap number, a.k.a. fault vector + * @error_cdde: exception error code + * @address: exception address, e.g. CR2 on a #PF + * @reserved: reserved for future use + */ +struct sgx_enclave_exception { + __u32 leaf; + __u16 trapnr; + __u16 error_code; + __u64 address; + __u64 reserved[2]; +}; + #endif /* _UAPI_ASM_X86_SGX_H */