| Message ID | 20181210232141.5425-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86: Add exception fixup for SGX ENCLU | expand |
On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > At that point I realized it's a hell of a lot easier to simply provide > an IOCTL via /dev/sgx that allows userspace to register a per-process > ENCLU exception handler. At a high level, the basic idea is the same > as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > attempt to fixup select unhandled exceptions that occurred in user code. So, on the one hand, this is *absolutely* much cleaner than the VDSO approach. On the other hand, this is global process state and has some of the same problems as a signal handler as a result.
On Mon, Dec 10, 2018 at 03:24:50PM -0800, Josh Triplett wrote: Good morning to everyone, I hope the week is progressing well. > On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > > At that point I realized it's a hell of a lot easier to simply provide > > an IOCTL via /dev/sgx that allows userspace to register a per-process > > ENCLU exception handler. At a high level, the basic idea is the same > > as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > > attempt to fixup select unhandled exceptions that occurred in user code. > So, on the one hand, this is *absolutely* much cleaner than the VDSO > approach. On the other hand, this is global process state and has > some of the same problems as a signal handler as a result. Sean's architecture is very simple and straight forward and thus has a lot going for it. As Sean's approach indicates, by linking the exception handler to current->mm, SGX is very much a per memory map concept. The issue is that there can be multiple enclaves loaded and excecuting in a processes memory map, the problem is, execution and thus exception handling, is very much at the per thread level. Based on the responses to my previous e-mail in this thread, the primary driver for addressing the exception handler issue is for shared libraries to implement independent execution of enclaves. So the question for Sean becomes where the 'pull' to address this issue is coming from, is it the Intel SDK/PSW folks? If so it might be helpful for them to weigh in on requirements and needs. To be very frank, there are only 3-4 groups actually working at this level; the Intel SDK/PSW team, Fortanix, us and probably Baidu. Given the complexity of the issues involved with a runtime, the Linux SGX development community, whether it be application or library developers are going to be building on top of runtimes maintained by groups such as these. If we boil issues down to the very basics, the setup of an exception handler is going to have to be wrapped into whatever code is being executed by a thread doing enclave entry for the runtime being used. It thus may be useful to think about this as being the responsibility of whatever loaded and initialized the enclave against which ENCLU[EENTER] is being issued against. To add an additional wrinkle to this, our runtime is already doing what amounts to recursive enclave invocation. We handle remote attestation quote generation and verification almost completely inside of enclaves. This requires that we issue an OCALL in order to exit the enclave being attested in order to load and execute the platform certification and quoting enclaves. So if Linux is going to deal with this correctly, without invoking any global state, the issue will come down conceptually to registering a per-thread/per-EENTER exception handler by whatever mechanism the kernel chooses to provide. Which means we need to be thinking about the ramifications of OCALL's. So whatever we do has to be simple, straight forward and flexible. If not the bike shedding will last until something other then SGX gets invented... :-) I hope the above reflections are useful. Best wishes for a productive day. 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 ------------------------------------------------------------------------------ "If your doing something the same way you have been doing it for ten years, the chances are you are doing it wrong." -- Charles Kettering
On Tue, Dec 11, 2018 at 08:53:39AM -0600, Dr. Greg wrote: > On Mon, Dec 10, 2018 at 03:24:50PM -0800, Josh Triplett wrote: > > Good morning to everyone, I hope the week is progressing well. > > > On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > > > At that point I realized it's a hell of a lot easier to simply provide > > > an IOCTL via /dev/sgx that allows userspace to register a per-process > > > ENCLU exception handler. At a high level, the basic idea is the same > > > as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > > > attempt to fixup select unhandled exceptions that occurred in user code. > > > So, on the one hand, this is *absolutely* much cleaner than the VDSO > > approach. On the other hand, this is global process state and has > > some of the same problems as a signal handler as a result. > > Sean's architecture is very simple and straight forward and thus has a > lot going for it. > > As Sean's approach indicates, by linking the exception handler to > current->mm, SGX is very much a per memory map concept. The issue is > that there can be multiple enclaves loaded and excecuting in a > processes memory map, the problem is, execution and thus exception > handling, is very much at the per thread level. Right, but is there a need to have a per-thread code page? The handler isn't per-process any more than the AEP is per-process.
> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote: > >> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: >> At that point I realized it's a hell of a lot easier to simply provide >> an IOCTL via /dev/sgx that allows userspace to register a per-process >> ENCLU exception handler. At a high level, the basic idea is the same >> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and >> attempt to fixup select unhandled exceptions that occurred in user code. > > So, on the one hand, this is *absolutely* much cleaner than the VDSO > approach. On the other hand, this is global process state and has some > of the same problems as a signal handler as a result. I liked the old version better for this reason and for another reason: while this new one looks very very simple, it still has the hidden complexity that the magic values written to registers in the event of an exception are very much Linux specific. OTOH, the old approach clobbered more regs than needed, but that’s a easy fix.
On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote: > > > > On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote: > > > >> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > >> At that point I realized it's a hell of a lot easier to simply provide > >> an IOCTL via /dev/sgx that allows userspace to register a per-process > >> ENCLU exception handler. At a high level, the basic idea is the same > >> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > >> attempt to fixup select unhandled exceptions that occurred in user code. > > > > So, on the one hand, this is *absolutely* much cleaner than the VDSO > > approach. On the other hand, this is global process state and has some > > of the same problems as a signal handler as a result. > > I liked the old version better for this reason This isn't fundamentally different than forcing all EENTER calls through the vDSO, which is also per-process. Technically this is more flexible in that regard since userspace gets to choose where their one ENCLU gets to reside. Userspace can have per-enclave entry flows so long as the actual ENLU[EENTER] is common, same as vDSO. > and for another reason: > while this new one looks very very simple, it still has the hidden > complexity that the magic values written to registers in the event of an > exception are very much Linux specific. Definitely more magical, but not necessarily more difficult to document. It'd essentially be an extension of hardware's AEE/AEP behavior. > OTOH, the old approach clobbered more regs than needed, but that’s a easy fix.
> On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote: >> >> >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote: >>>> >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: >>>> At that point I realized it's a hell of a lot easier to simply provide >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process >>>> ENCLU exception handler. At a high level, the basic idea is the same >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and >>>> attempt to fixup select unhandled exceptions that occurred in user code. >>> >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO >>> approach. On the other hand, this is global process state and has some >>> of the same problems as a signal handler as a result. >> >> I liked the old version better for this reason > > This isn't fundamentally different than forcing all EENTER calls through > the vDSO, which is also per-process. Technically this is more flexible > in that regard since userspace gets to choose where their one ENCLU gets > to reside. Userspace can have per-enclave entry flows so long as the > actual ENLU[EENTER] is common, same as vDSO. Right. The problem is that user libraries have a remarkably hard time agreeing on where their one copy of anything lives. > >> and for another reason: >> while this new one looks very very simple, it still has the hidden >> complexity that the magic values written to registers in the event of an >> exception are very much Linux specific. > > Definitely more magical, but not necessarily more difficult to document. > It'd essentially be an extension of hardware's AEE/AEP behavior. > >> OTOH, the old approach clobbered more regs than needed, but that’s a easy fix.
On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote: > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote: > >> > >> > >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote: > >>>> > >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > >>>> At that point I realized it's a hell of a lot easier to simply provide > >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process > >>>> ENCLU exception handler. At a high level, the basic idea is the same > >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > >>>> attempt to fixup select unhandled exceptions that occurred in user code. > >>> > >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO > >>> approach. On the other hand, this is global process state and has some > >>> of the same problems as a signal handler as a result. > >> > >> I liked the old version better for this reason > > > > This isn't fundamentally different than forcing all EENTER calls through > > the vDSO, which is also per-process. Technically this is more flexible > > in that regard since userspace gets to choose where their one ENCLU gets > > to reside. Userspace can have per-enclave entry flows so long as the > > actual ENLU[EENTER] is common, same as vDSO. > > Right. The problem is that user libraries have a remarkably hard time > agreeing on where their one copy of anything lives. Ah, so vDSO is the mean parent that takes away their kids' toys because thay haven't learned to share :)
On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote: > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote: > >> > >> > >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote: > >>>> > >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > >>>> At that point I realized it's a hell of a lot easier to simply provide > >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process > >>>> ENCLU exception handler. At a high level, the basic idea is the same > >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > >>>> attempt to fixup select unhandled exceptions that occurred in user code. > >>> > >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO > >>> approach. On the other hand, this is global process state and has some > >>> of the same problems as a signal handler as a result. > >> > >> I liked the old version better for this reason > > > > This isn't fundamentally different than forcing all EENTER calls through > > the vDSO, which is also per-process. Technically this is more flexible > > in that regard since userspace gets to choose where their one ENCLU gets > > to reside. Userspace can have per-enclave entry flows so long as the > > actual ENLU[EENTER] is common, same as vDSO. > > Right. The problem is that user libraries have a remarkably hard time > agreeing on where their one copy of anything lives. Are you concerned about userspace shooting themselves in the foot, e.g. unknowingly overwriting their handler? Requiring unregister->register to change the handler would mitigate that issue for the most part. Or we could even say it's a write-once property. That obviously doesn't solve the issue of a userspace application deliberately using two different libraries to run enclaves in a single process, but I have a hard time envisioning a scenario where someone would want to use two different *SGX* libraries in a single process. Don't most of the signal issue arise due to loading multiple libraries that provide *different* services needing to handle signals?
On Tue, Dec 11, 2018 at 2:23 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote: > > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > > >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote: > > >> > > >> > > >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote: > > >>>> > > >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > > >>>> At that point I realized it's a hell of a lot easier to simply provide > > >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process > > >>>> ENCLU exception handler. At a high level, the basic idea is the same > > >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > > >>>> attempt to fixup select unhandled exceptions that occurred in user code. > > >>> > > >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO > > >>> approach. On the other hand, this is global process state and has some > > >>> of the same problems as a signal handler as a result. > > >> > > >> I liked the old version better for this reason > > > > > > This isn't fundamentally different than forcing all EENTER calls through > > > the vDSO, which is also per-process. Technically this is more flexible > > > in that regard since userspace gets to choose where their one ENCLU gets > > > to reside. Userspace can have per-enclave entry flows so long as the > > > actual ENLU[EENTER] is common, same as vDSO. > > > > Right. The problem is that user libraries have a remarkably hard time > > agreeing on where their one copy of anything lives. > > Are you concerned about userspace shooting themselves in the foot, e.g. > unknowingly overwriting their handler? Requiring unregister->register > to change the handler would mitigate that issue for the most part. Or > we could even say it's a write-once property. > > That obviously doesn't solve the issue of a userspace application > deliberately using two different libraries to run enclaves in a single > process, but I have a hard time envisioning a scenario where someone > would want to use two different *SGX* libraries in a single process. > Don't most of the signal issue arise due to loading multiple libraries > that provide *different* services needing to handle signals? I can easily imagine two SGX libraries that know nothing about each other running in the same process. One or both could be PKCS#11 modules, for example. I suspect that Linux will eventually want the ability for libraries to register exception handlers, but that's not going to get designed and implemented quickly enough for SGX's initial Linux rollout. A vDSO helper like in your earlier series should solve most of the problem without any contention issues.
On Tue, Dec 11, 2018 at 03:10:52PM -0800, Andy Lutomirski wrote: > On Tue, Dec 11, 2018 at 2:23 PM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote: > > > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > > > > >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote: > > > >> > > > >> > > > >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote: > > > >>>> > > > >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote: > > > >>>> At that point I realized it's a hell of a lot easier to simply provide > > > >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process > > > >>>> ENCLU exception handler. At a high level, the basic idea is the same > > > >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and > > > >>>> attempt to fixup select unhandled exceptions that occurred in user code. > > > >>> > > > >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO > > > >>> approach. On the other hand, this is global process state and has some > > > >>> of the same problems as a signal handler as a result. > > > >> > > > >> I liked the old version better for this reason > > > > > > > > This isn't fundamentally different than forcing all EENTER calls through > > > > the vDSO, which is also per-process. Technically this is more flexible > > > > in that regard since userspace gets to choose where their one ENCLU gets > > > > to reside. Userspace can have per-enclave entry flows so long as the > > > > actual ENLU[EENTER] is common, same as vDSO. > > > > > > Right. The problem is that user libraries have a remarkably hard time > > > agreeing on where their one copy of anything lives. > > > > Are you concerned about userspace shooting themselves in the foot, e.g. > > unknowingly overwriting their handler? Requiring unregister->register > > to change the handler would mitigate that issue for the most part. Or > > we could even say it's a write-once property. > > > > That obviously doesn't solve the issue of a userspace application > > deliberately using two different libraries to run enclaves in a single > > process, but I have a hard time envisioning a scenario where someone > > would want to use two different *SGX* libraries in a single process. > > Don't most of the signal issue arise due to loading multiple libraries > > that provide *different* services needing to handle signals? > > I can easily imagine two SGX libraries that know nothing about each > other running in the same process. One or both could be PKCS#11 > modules, for example. Argh, wasn't thinking about loading other libraries that would also be using SGX.
On Tue, Dec 11, 2018 at 03:10:52PM -0800, Andy Lutomirski wrote: Good evening, I hope the day has gone well for everyone. > > > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > > > > > This isn't fundamentally different than forcing all EENTER > > > > calls through the vDSO, which is also per-process. > > > > Technically this is more flexible in that regard since > > > > userspace gets to choose where their one ENCLU gets to reside. > > > > Userspace can have per-enclave entry flows so long as the > > > > actual ENLU[EENTER] is common, same as vDSO. > > > Right. The problem is that user libraries have a remarkably hard > > > time agreeing on where their one copy of anything lives. > > Are you concerned about userspace shooting themselves in the foot, > > e.g. unknowingly overwriting their handler? Requiring > > unregister->register to change the handler would mitigate that > > issue for the most part. Or we could even say it's a write-once > > property. > > > > That obviously doesn't solve the issue of a userspace application > > deliberately using two different libraries to run enclaves in a > > single process, but I have a hard time envisioning a scenario > > where someone would want to use two different *SGX* libraries in a > > single process. Don't most of the signal issue arise due to > > loading multiple libraries that provide *different* services > > needing to handle signals? > I can easily imagine two SGX libraries that know nothing about each > other running in the same process. One or both could be PKCS#11 > modules, for example. Very good, I see that Sean agreed with this down thread. I was concerned that our discussion was lacking precision and we were talking past one another. > I suspect that Linux will eventually want the ability for libraries > to register exception handlers, but that's not going to get designed > and implemented quickly enough for SGX's initial Linux rollout. A > vDSO helper like in your earlier series should solve most of the > problem without any contention issues. Let me see if I can impart some framework for additional clarity as discussions proceed forward. I believe it would be helpful if we could agree to refer to a body of code, possibly in library form, that loads, initializes and executes an enclave as an 'SGX runtime'. In this framework, the term 'library' refers to code that an application links to for domain specific functionality, ie. libpkcs11, libkrb5, libsasl. These 'libraries' may implement enclaves, using 'SGX runtimes' of their choice, to improve their security guarantees. In this model it is the 'SGX runtime' that is responsible for registering SGX exception handlers under their management. In order for mainline Linux SGX support to be relevant, it must admit mutually distrusting 'SGX runtimes' in the same process context. The SGX exception handler architecture must also support the notion of 'nested enclave' invocation where an enclave may execute an OCALL and then go on to execute an enclave, possibly based on a different 'SGX runtime', before returning into its previous enclave. Hopefully the above will help assist further discussions. Have a good evening. 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 ------------------------------------------------------------------------------ "If you think nobody cares if you're alive, try missing a couple of car payments." -- Earl Wilson
This is effectively v3 of the "x86: Add vDSO exception fixup for SGX" series, but as you might of noticed, there are no vDSO changes here. Andy's comment on Spectre/retpoline[1] and Jethro's comment on making the vDSO as barebones as possible[2] got me wondering if we could let userspace dynamically set the "return" target of the SGX vDSO function, taking advantage of the fact that most (all?) SGX processes will have a single entry point for all enclaves. The initial idea was to make __vdso_sgx_enter_enclave() a barebones "function" consisting of ENCLU and UD2. SGX would provide an IOCTL via /dev/sgx that could be used to patch the UD2 in the current vDSO image to a JMP rel32, with rel32 pointing at an address provided by the user. Aside from the issues of rel32 in a 64-bit memory space, patching the vDSO image added a lot of overhead just so that the kernel could know the address of ENCLU. At that point I realized it's a hell of a lot easier to simply provide an IOCTL via /dev/sgx that allows userspace to register a per-process ENCLU exception handler. At a high level, the basic idea is the same as the vDSO approach: provide a hardcoded fixup handler for ENCLU and attempt to fixup select unhandled exceptions that occurred in user code. Pros: - No vDSO function. - Minimal userspace changes. - Smaller ABI, i.e. less bikeshedding (fingers crossed). - Kernel doesn't prevent userspace from doing stupid things, e.g. modifying the process' stack from within the enclave. - Kernel can proactively enforce the ENCLU handler as the only officialy supported ABI for enclave exception handling. Cons: - Userspace can only register a single ENCLU handler per process. - ABI is more esoteric than a standard function call. This series is based on Jarkko's current master branch, which moves all of the SGX code into arch/x86. The full code can be found here: https://github.com/sean-jc/linux/tree/2538adcece15447b988d93bf677af48073c7d219 I have not actually tested this *exact* commit beyond compiling as I do not know the health status of Jarkko's code. To test, I cherry-picked the patches into an older stable version of the code. Git was able to handle the file renaming. v1: https://lkml.kernel.org/r/20181205232012.28920-1-sean.j.christopherson@intel.com v2: https://lkml.kernel.org/r/20181206221922.31012-1-sean.j.christopherson@intel.com v3: - Replace the vDSO fixup with SGX specific fixup. - Patches 2/4 and 3/4 are essentially identical, the only difference being the name of the function that is called. The changelogs for these patches still need a lot of attention. [1] https://lkml.kernel.org/r/CALCETrVBR+2HjTqX=W4r9GOq69Xg36v4gmCKqK0wUjzAqBJnrw@mail.gmail.com [2] https://lkml.kernel.org/r/f595c046-682c-0d4a-ce68-44d4634cedf2@fortanix.com Sean Christopherson (4): x86/sgx: Add a per-mm ENCLU exception fixup handler x86/fault: Attempt to fixup unhandled #PF on ENCLU before signaling x86/traps: Attempt to fixup exceptions in vDSO before signaling x86/sgx: Add an SGX IOCTL to register a per-mm ENCLU exception handler arch/x86/include/asm/mmu.h | 4 ++++ arch/x86/include/asm/sgx.h | 13 +++++++++++++ arch/x86/include/uapi/asm/sgx.h | 23 ++++++++++++++++++----- arch/x86/kernel/cpu/sgx/driver/encl.c | 6 ++++++ arch/x86/kernel/cpu/sgx/driver/ioctl.c | 20 ++++++++++++++++++++ arch/x86/kernel/cpu/sgx/main.c | 18 ++++++++++++++++++ arch/x86/kernel/traps.c | 15 +++++++++++++++ arch/x86/mm/fault.c | 7 +++++++ 8 files changed, 101 insertions(+), 5 deletions(-)