| Message ID | 20190424062623.4345-1-cedric.xing@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | An alternative __vdso_sgx_enter_enclave() to allow enclave/host parameter passing using untrusted stack | expand |
On Tue, Apr 23, 2019 at 11:26:20PM -0700, Cedric Xing wrote: > The current proposed __vdso_sgx_enter_enclave() requires enclaves to preserve > %rsp, which prohibits enclaves from allocating space on the untrusted stack. > However, there are existing enclaves (e.g. those built with current Intel SGX > SDK libraries) relying on the untrusted stack for passing parameters to > untrusted functions (aka. o-calls), which requires allocating space on the > untrusted stack by enclaves. And given its simplicity and convenience, it could > be desired by future SGX applications as well. > > This patchset introduces a new ABI for __vdso_sgx_enter_enclave() to anchor its > stack frame on %rbp (instead of %rsp), so as to allow enclaves to "push" onto > the untrusted stack by decrementing the untrusted %rsp. Additionally, this new > __vdso_sgx_enter_enclave() will take one more parameter - a callback function, > to be invoked upon all enclave exits (both AEX and normal exits). The callback > function will be given the value of %rsp left off by the enclave, so that data > "pushed" by the enclave (if any) could be addressed/accessed. Please note that > the callback function is optional, and if not supplied (i.e. null), > __vdso_sgx_enter_enclave() will just return (i.e. behave the same as the > current implementation) after the enclave exits (or AEX due to exceptions). > > The SGX selftest is augmented by two new tests. One exercises the new callback > interface, and serves as a simple example to showcase how to use it; while the > other validates the hand-crafted CFI directives in __vdso_sgx_enter_enclave() > by single-stepping through it and unwinding call stack at every instruction. Why does the SDK anyway use real enclaves when step debugging? I don't think kernel needs to scale to that. For me it looks more like a bad architectural choice in the SDK implementation than anything else. You should design SDK in a way that it doesn't run the code inside real enclave at first. It is just the sanest development model to use. The current SDK has an unacceptable requirement that the workstation used to develop code destined to run inside enclave must possess an appropriate hardware. I don't think that is acceptable no matter what kind of API kernel provides to invoke enclaves. I think "real" debug enclaves are only good for production testing when you have more or less ready to release/deploy something but still want to be able to get a memory dump of the enclave on occassion. With these conclusions I think the current vDSO API is sufficient for Linux. /Jarkko
On 7/10/2019 4:17 AM, Jarkko Sakkinen wrote: > On Tue, Apr 23, 2019 at 11:26:20PM -0700, Cedric Xing wrote: >> The current proposed __vdso_sgx_enter_enclave() requires enclaves to preserve >> %rsp, which prohibits enclaves from allocating space on the untrusted stack. >> However, there are existing enclaves (e.g. those built with current Intel SGX >> SDK libraries) relying on the untrusted stack for passing parameters to >> untrusted functions (aka. o-calls), which requires allocating space on the >> untrusted stack by enclaves. And given its simplicity and convenience, it could >> be desired by future SGX applications as well. >> >> This patchset introduces a new ABI for __vdso_sgx_enter_enclave() to anchor its >> stack frame on %rbp (instead of %rsp), so as to allow enclaves to "push" onto >> the untrusted stack by decrementing the untrusted %rsp. Additionally, this new >> __vdso_sgx_enter_enclave() will take one more parameter - a callback function, >> to be invoked upon all enclave exits (both AEX and normal exits). The callback >> function will be given the value of %rsp left off by the enclave, so that data >> "pushed" by the enclave (if any) could be addressed/accessed. Please note that >> the callback function is optional, and if not supplied (i.e. null), >> __vdso_sgx_enter_enclave() will just return (i.e. behave the same as the >> current implementation) after the enclave exits (or AEX due to exceptions). >> >> The SGX selftest is augmented by two new tests. One exercises the new callback >> interface, and serves as a simple example to showcase how to use it; while the >> other validates the hand-crafted CFI directives in __vdso_sgx_enter_enclave() >> by single-stepping through it and unwinding call stack at every instruction. > > Why does the SDK anyway use real enclaves when step debugging? I don't > think kernel needs to scale to that. For me it looks more like a bad > architectural choice in the SDK implementation than anything else. Intel's SGX SDK *does* support simulation mode for debugging enclaves outside of SGX mode, or even on non-SGX platforms. But nothing can replace the real SGX environment, hence SGX ISA supports debugging real enclaves. > You should design SDK in a way that it doesn't run the code inside real > enclave at first. It is just the sanest development model to use. The > current SDK has an unacceptable requirement that the workstation used to > develop code destined to run inside enclave must possess an appropriate > hardware. I don't think that is acceptable no matter what kind of API > kernel provides to invoke enclaves. SDK supports simulation on non-SGX platforms. > I think "real" debug enclaves are only good for production testing when > you have more or less ready to release/deploy something but still want > to be able to get a memory dump of the enclave on occassion. That's true. > With these conclusions I think the current vDSO API is sufficient for > Linux. The new vDSO API is to support data exchange on stack. It has nothing to do with debugging. BTW, the community has closed on this. The CFI directives are for stack unwinding. They don't affect what the code does so you can just treat them as NOPs if you don't understand what they do. However, they are useful to not only debuggers but also exception handling code. libunwind also has a setjmp()/longjmp() implementation based on CFI directives. > /Jarkko >
On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: > > With these conclusions I think the current vDSO API is sufficient for > > Linux. > > The new vDSO API is to support data exchange on stack. It has nothing to do > with debugging. BTW, the community has closed on this. And how that is useful? > The CFI directives are for stack unwinding. They don't affect what the code > does so you can just treat them as NOPs if you don't understand what they > do. However, they are useful to not only debuggers but also exception > handling code. libunwind also has a setjmp()/longjmp() implementation based > on CFI directives. Of course I won't merge code of which usefulness I don't understand. /Jarkko
On 7/10/2019 3:46 PM, Jarkko Sakkinen wrote: > On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: >>> With these conclusions I think the current vDSO API is sufficient for >>> Linux. >> >> The new vDSO API is to support data exchange on stack. It has nothing to do >> with debugging. BTW, the community has closed on this. > > And how that is useful? There is a lengthy discussion on its usefulness so I don't want to repeat. In short, it allows using untrusted stack as a convenient method to exchange data with the enclave. It is currently being used by Intel's SGX SDK for e/o-calls parameters. >> The CFI directives are for stack unwinding. They don't affect what the code >> does so you can just treat them as NOPs if you don't understand what they >> do. However, they are useful to not only debuggers but also exception >> handling code. libunwind also has a setjmp()/longjmp() implementation based >> on CFI directives. > > Of course I won't merge code of which usefulness I don't understand. Sure. Any other questions I can help with? > /Jarkko >
On Thu, Jul 11, 2019 at 01:46:28AM +0300, Jarkko Sakkinen wrote: > On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: > > > With these conclusions I think the current vDSO API is sufficient for > > > Linux. > > > > The new vDSO API is to support data exchange on stack. It has nothing to do > > with debugging. BTW, the community has closed on this. > > And how that is useful? > > > The CFI directives are for stack unwinding. They don't affect what the code > > does so you can just treat them as NOPs if you don't understand what they > > do. However, they are useful to not only debuggers but also exception > > handling code. libunwind also has a setjmp()/longjmp() implementation based > > on CFI directives. > > Of course I won't merge code of which usefulness I don't understand. I re-read the cover letter [1] because it usually is the place to "pitch" a feature. It fails to address two things: 1. How and in what circumstances is an untrusted stack is a better vessel for handling exceptions than the register based approach that we already have? 2. How is it simpler approach? There is a strong claim of simplicity and convenience without anything backing it. 3. Why we need both register and stack based approach co-exist? I'd go with one approach for a new API without any legacy whatsoever. This really needs a better pitch before we can consider doing anything to it. Also, in [2] there is talk about the next revision. Maybe the way go forward is to address the three issues I found in the cover letter and fix whatever needed to be fixed in the actual patches? [1] https://lkml.org/lkml/2019/4/24/84 [2] https://lkml.org/lkml/2019/4/25/1170 /Jarkko
On 7/10/2019 4:15 PM, Jarkko Sakkinen wrote: > On Thu, Jul 11, 2019 at 01:46:28AM +0300, Jarkko Sakkinen wrote: >> On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: >>>> With these conclusions I think the current vDSO API is sufficient for >>>> Linux. >>> >>> The new vDSO API is to support data exchange on stack. It has nothing to do >>> with debugging. BTW, the community has closed on this. >> >> And how that is useful? >> >>> The CFI directives are for stack unwinding. They don't affect what the code >>> does so you can just treat them as NOPs if you don't understand what they >>> do. However, they are useful to not only debuggers but also exception >>> handling code. libunwind also has a setjmp()/longjmp() implementation based >>> on CFI directives. >> >> Of course I won't merge code of which usefulness I don't understand. > > I re-read the cover letter [1] because it usually is the place > to "pitch" a feature. > > It fails to address two things: > > 1. How and in what circumstances is an untrusted stack is a better > vessel for handling exceptions than the register based approach > that we already have? We are not judging which vessel is better (or the best) among all possible vessels. We are trying to enable more vessels. Every vessel has its pros and cons so there's *no* single best vessel. > 2. How is it simpler approach? There is a strong claim of simplicity > and convenience without anything backing it. The major benefits in terms of simplicity realize in user mode applications. It's always a trade-off. This vDSO API takes 10-20 more lines than the original one but would save hundreds or even thousands in user applications. Again, I don't want to repeat everything as you can look back at the lengthy discussion to dig out the details. > 3. Why we need both register and stack based approach co-exist? I'd go > with one approach for a new API without any legacy whatsoever. Neither is a legacy to the other. Supporting both approaches is by design. Again, the goal is to enable more vessels because there's *no* single best vessel. > This really needs a better pitch before we can consider doing anything > to it. > > Also, in [2] there is talk about the next revision. Maybe the way go > forward is to address the three issues I found in the cover letter > and fix whatever needed to be fixed in the actual patches? > > [1] https://lkml.org/lkml/2019/4/24/84 > [2] https://lkml.org/lkml/2019/4/25/1170 Let me update the commit message for the vDSO API and send you a patch. > /Jarkko >
On Wed, Jul 10, 2019 at 03:54:20PM -0700, Xing, Cedric wrote: > On 7/10/2019 3:46 PM, Jarkko Sakkinen wrote: > > On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: > > > > With these conclusions I think the current vDSO API is sufficient for > > > > Linux. > > > > > > The new vDSO API is to support data exchange on stack. It has nothing to do > > > with debugging. BTW, the community has closed on this. > > > > And how that is useful? > > There is a lengthy discussion on its usefulness so I don't want to repeat. > In short, it allows using untrusted stack as a convenient method to exchange > data with the enclave. It is currently being used by Intel's SGX SDK for > e/o-calls parameters. > > > > The CFI directives are for stack unwinding. They don't affect what the code > > > does so you can just treat them as NOPs if you don't understand what they > > > do. However, they are useful to not only debuggers but also exception > > > handling code. libunwind also has a setjmp()/longjmp() implementation based > > > on CFI directives. > > > > Of course I won't merge code of which usefulness I don't understand. > > Sure. > > Any other questions I can help with? I dissected my concerns in other email. We can merge this feature after v21 if it makes sense. /Jarkko
On Wed, Jul 10, 2019 at 04:37:41PM -0700, Xing, Cedric wrote: > On 7/10/2019 4:15 PM, Jarkko Sakkinen wrote: > > On Thu, Jul 11, 2019 at 01:46:28AM +0300, Jarkko Sakkinen wrote: > > > On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: > > > > > With these conclusions I think the current vDSO API is sufficient for > > > > > Linux. > > > > > > > > The new vDSO API is to support data exchange on stack. It has nothing to do > > > > with debugging. BTW, the community has closed on this. > > > > > > And how that is useful? > > > > > > > The CFI directives are for stack unwinding. They don't affect what the code > > > > does so you can just treat them as NOPs if you don't understand what they > > > > do. However, they are useful to not only debuggers but also exception > > > > handling code. libunwind also has a setjmp()/longjmp() implementation based > > > > on CFI directives. > > > > > > Of course I won't merge code of which usefulness I don't understand. > > > > I re-read the cover letter [1] because it usually is the place > > to "pitch" a feature. > > > > It fails to address two things: > > > > 1. How and in what circumstances is an untrusted stack is a better > > vessel for handling exceptions than the register based approach > > that we already have? > > We are not judging which vessel is better (or the best) among all possible > vessels. We are trying to enable more vessels. Every vessel has its pros and > cons so there's *no* single best vessel. I think reasonable metric is actually the coverage of the Intel SDK based enclaves. How widely are they in the wild? If the user base is large, it should be reasonable to support this just based on that. /Jarkko
On Thu, Jul 11, 2019 at 12:38:09PM +0300, Jarkko Sakkinen wrote: > On Wed, Jul 10, 2019 at 04:37:41PM -0700, Xing, Cedric wrote: > > We are not judging which vessel is better (or the best) among all possible > > vessels. We are trying to enable more vessels. Every vessel has its pros and > > cons so there's *no* single best vessel. > > I think reasonable metric is actually the coverage of the Intel SDK > based enclaves. How widely are they in the wild? If the user base is > large, it should be reasonable to support this just based on that. Large enough that Andy agreed to take the vDSO code with the optional callback, despite his personal opinion being that mucking with uR{B,S}P from within the enclave is poor form.
On Thu, Jul 11, 2019 at 08:50:37AM -0700, Sean Christopherson wrote: > On Thu, Jul 11, 2019 at 12:38:09PM +0300, Jarkko Sakkinen wrote: > > On Wed, Jul 10, 2019 at 04:37:41PM -0700, Xing, Cedric wrote: > > > We are not judging which vessel is better (or the best) among all possible > > > vessels. We are trying to enable more vessels. Every vessel has its pros and > > > cons so there's *no* single best vessel. > > > > I think reasonable metric is actually the coverage of the Intel SDK > > based enclaves. How widely are they in the wild? If the user base is > > large, it should be reasonable to support this just based on that. > > Large enough that Andy agreed to take the vDSO code with the optional > callback, despite his personal opinion being that mucking with uR{B,S}P > from within the enclave is poor form. OK, the cover letter empahasized things that did not make sense to me, which made me to do my initial conclusions. I don't recall even reading the word "coverage" from it. Anyways, I'm sure we can land this after v21 has been published now that the rationale is clear. /Jarkko
On 7/11/2019 2:36 AM, Jarkko Sakkinen wrote: > On Wed, Jul 10, 2019 at 03:54:20PM -0700, Xing, Cedric wrote: >> On 7/10/2019 3:46 PM, Jarkko Sakkinen wrote: >>> On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: >>>>> With these conclusions I think the current vDSO API is sufficient for >>>>> Linux. >>>> >>>> The new vDSO API is to support data exchange on stack. It has nothing to do >>>> with debugging. BTW, the community has closed on this. >>> >>> And how that is useful? >> >> There is a lengthy discussion on its usefulness so I don't want to repeat. >> In short, it allows using untrusted stack as a convenient method to exchange >> data with the enclave. It is currently being used by Intel's SGX SDK for >> e/o-calls parameters. >> >>>> The CFI directives are for stack unwinding. They don't affect what the code >>>> does so you can just treat them as NOPs if you don't understand what they >>>> do. However, they are useful to not only debuggers but also exception >>>> handling code. libunwind also has a setjmp()/longjmp() implementation based >>>> on CFI directives. >>> >>> Of course I won't merge code of which usefulness I don't understand. >> >> Sure. >> >> Any other questions I can help with? > > I dissected my concerns in other email. We can merge this feature after > v21 if it makes sense. Sent out v3 of vDSO changes last night. I hope your concerns have been properly addressed. The new vDSO API is a community consensus. I can help on whatever technical problems you may have but I don't see a reason you should reject it. > /Jarkko >
On 7/11/2019 2:38 AM, Jarkko Sakkinen wrote: > On Wed, Jul 10, 2019 at 04:37:41PM -0700, Xing, Cedric wrote: >> On 7/10/2019 4:15 PM, Jarkko Sakkinen wrote: >>> On Thu, Jul 11, 2019 at 01:46:28AM +0300, Jarkko Sakkinen wrote: >>>> On Wed, Jul 10, 2019 at 11:08:37AM -0700, Xing, Cedric wrote: >>>>>> With these conclusions I think the current vDSO API is sufficient for >>>>>> Linux. >>>>> >>>>> The new vDSO API is to support data exchange on stack. It has nothing to do >>>>> with debugging. BTW, the community has closed on this. >>>> >>>> And how that is useful? >>>> >>>>> The CFI directives are for stack unwinding. They don't affect what the code >>>>> does so you can just treat them as NOPs if you don't understand what they >>>>> do. However, they are useful to not only debuggers but also exception >>>>> handling code. libunwind also has a setjmp()/longjmp() implementation based >>>>> on CFI directives. >>>> >>>> Of course I won't merge code of which usefulness I don't understand. >>> >>> I re-read the cover letter [1] because it usually is the place >>> to "pitch" a feature. >>> >>> It fails to address two things: >>> >>> 1. How and in what circumstances is an untrusted stack is a better >>> vessel for handling exceptions than the register based approach >>> that we already have? >> >> We are not judging which vessel is better (or the best) among all possible >> vessels. We are trying to enable more vessels. Every vessel has its pros and >> cons so there's *no* single best vessel. > > I think reasonable metric is actually the coverage of the Intel SDK > based enclaves. How widely are they in the wild? If the user base is > large, it should be reasonable to support this just based on that. I don't know how many existing enclaves out there, but definitely larger than 0 (zero), while user base for the old API is definitely 0. What are you worrying, really? > /Jarkko >
The current proposed __vdso_sgx_enter_enclave() requires enclaves to preserve %rsp, which prohibits enclaves from allocating space on the untrusted stack. However, there are existing enclaves (e.g. those built with current Intel SGX SDK libraries) relying on the untrusted stack for passing parameters to untrusted functions (aka. o-calls), which requires allocating space on the untrusted stack by enclaves. And given its simplicity and convenience, it could be desired by future SGX applications as well. This patchset introduces a new ABI for __vdso_sgx_enter_enclave() to anchor its stack frame on %rbp (instead of %rsp), so as to allow enclaves to "push" onto the untrusted stack by decrementing the untrusted %rsp. Additionally, this new __vdso_sgx_enter_enclave() will take one more parameter - a callback function, to be invoked upon all enclave exits (both AEX and normal exits). The callback function will be given the value of %rsp left off by the enclave, so that data "pushed" by the enclave (if any) could be addressed/accessed. Please note that the callback function is optional, and if not supplied (i.e. null), __vdso_sgx_enter_enclave() will just return (i.e. behave the same as the current implementation) after the enclave exits (or AEX due to exceptions). The SGX selftest is augmented by two new tests. One exercises the new callback interface, and serves as a simple example to showcase how to use it; while the other validates the hand-crafted CFI directives in __vdso_sgx_enter_enclave() by single-stepping through it and unwinding call stack at every instruction. v2: - Revised comments in __vdso_sgx_enter_enclave(). See patch 2/3. - Added stack unwind test. See patch 3/3. v1: https://lkml.org/lkml/2019/4/22/871 Note: This patchset is based upon SGX1 patch v20 (https://lkml.org/lkml/2019/4/17/344) by Jarkko Sakkinen Cedric Xing (3): selftests/x86: Fixed Makefile for SGX selftest x86/vdso: Modify __vdso_sgx_enter_enclave() to allow parameter passing on untrusted stack selftests/x86: Augment SGX selftest to test new __vdso_sgx_enter_enclave() and its callback interface arch/x86/entry/vdso/vsgx_enter_enclave.S | 175 +++++++---- arch/x86/include/uapi/asm/sgx.h | 14 +- tools/testing/selftests/x86/Makefile | 12 +- tools/testing/selftests/x86/sgx/Makefile | 49 ++-- tools/testing/selftests/x86/sgx/main.c | 323 ++++++++++++++++++--- tools/testing/selftests/x86/sgx/sgx_call.S | 40 ++- 6 files changed, 471 insertions(+), 142 deletions(-)