Re: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Tue, Dec 19, 2017 at 11:24:55PM +, Christopherson, Sean J wrote: > Exposing the token generated by the in-kernel LE doesn't affect the > kernel's power in the slightest, e.g. the kernel doesn't need a LE > to refuse to run an enclave and a privileged user can always load > an out-of-tree driver if they really want to circumvent the kernel's > policies, which is probably easier than stealing the LE's private key. If the MSRs are read-only, kernel does need an LE in order to launch enclaves if it only has the SIGSTRUCT. User with abilities to load out-of-tree driver or otherwise modify the running kernel code does not really work as an argument in to any direction. /Jarkko
RE: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Tuesday, December 19, 2017 Jarkko Sakkinen wrote: > On Tue, 2017-12-19 at 18:52 +, Christopherson, Sean J wrote: > > > We can cache tokens in future in the kernel space, can't we? > > > > Yes, but why? Deferring to userspace is less complex and likely > > more performant. > > That's quite strong argument especially if you are making that for > systems running multiple independent workloads and not just a single > application. > > > Tokens are large enough that there would need to be some form of > > limit on the number of tokens, which brings up questions about > > how to account tokens, the cache eviction scheme, whether or not > > the size of the cache should be controllable from userspace, etc... > > Leaving caching decisions to the kernel also gives more freedoms to > do global decisions. > > > Userspace caching can likely provide better performance because > > the user/application knows the usage model and life expectancy of > > its tokens, i.e. userspace can make informed decisions about when > > to discard a token, how much memory to dedicate to caching tokens, > > etc... And in the case of VMs, userspace can reuse tokens across > > reboots (of the VM), e.g. by saving tokens to disk. > > I'm not really convinced that your argument is sound if you consider the > whole range of x86 systems that can run enclaves especially if the > system is running multiple irrelated applications. > > And you are ignoring everything else but the performance, which is does > not make any sense. The current design governs the Linux kernel to have > the ultimate power, which enclaves to run with the minimized proprietary > risk. I think that is something worth of emphasizing too. Exposing the token generated by the in-kernel LE doesn't affect the kernel's power in the slightest, e.g. the kernel doesn't need a LE to refuse to run an enclave and a privileged user can always load an out-of-tree driver if they really want to circumvent the kernel's policies, which is probably easier than stealing the LE's private key. > > Whether the token caching is left to kernel or user space will most > definitely introduce some non-trivial performance problems to solve > with some unexpected workloads that we cannot imagine right now. That's > why the governance should be the driver. Not the performance. Those > issues can and must be sorted out in any case. >
Re: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Tue, 2017-12-19 at 18:52 +, Christopherson, Sean J wrote: > > We can cache tokens in future in the kernel space, can't we? > > Yes, but why? Deferring to userspace is less complex and likely > more performant. That's quite strong argument especially if you are making that for systems running multiple independent workloads and not just a single application. > Tokens are large enough that there would need to be some form of > limit on the number of tokens, which brings up questions about > how to account tokens, the cache eviction scheme, whether or not > the size of the cache should be controllable from userspace, etc... Leaving caching decisions to the kernel also gives more freedoms to do global decisions. > Userspace caching can likely provide better performance because > the user/application knows the usage model and life expectancy of > its tokens, i.e. userspace can make informed decisions about when > to discard a token, how much memory to dedicate to caching tokens, > etc... And in the case of VMs, userspace can reuse tokens across > reboots (of the VM), e.g. by saving tokens to disk. I'm not really convinced that your argument is sound if you consider the whole range of x86 systems that can run enclaves especially if the system is running multiple irrelated applications. And you are ignoring everything else but the performance, which is does not make any sense. The current design governs the Linux kernel to have the ultimate power, which enclaves to run with the minimized proprietary risk. I think that is something worth of emphasizing too. Whether the token caching is left to kernel or user space will most definitely introduce some non-trivial performance problems to solve with some unexpected workloads that we cannot imagine right now. That's why the governance should be the driver. Not the performance. Those issues can and must be sorted out in any case. /Jarkko
RE: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Friday, 2017-12-15, Jarkko Sakkinen wrote: > > Resurrecting this thread now that I have a system with launch control > > and have been able to measure the performance impact... > > > > Regenerating the EINIT token every time adds somewhere in the vicinity > > of ~5% overhead to creating an enclave, versus generating a token once > > and reusing it in each EINIT call. This isn't a huge issue since real > > world usage models likely won't be re-launching enclaves at a high rate, > > but it is measurable. > > We can cache tokens in future in the kernel space, can't we? Yes, but why? Deferring to userspace is less complex and likely more performant. Tokens are large enough that there would need to be some form of limit on the number of tokens, which brings up questions about how to account tokens, the cache eviction scheme, whether or not the size of the cache should be controllable from userspace, etc... Userspace caching can likely provide better performance because the user/application knows the usage model and life expectancy of its tokens, i.e. userspace can make informed decisions about when to discard a token, how much memory to dedicate to caching tokens, etc... And in the case of VMs, userspace can reuse tokens across reboots (of the VM), e.g. by saving tokens to disk.
Re: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Wed, Dec 13, 2017 at 11:18:29PM +, Christopherson, Sean J wrote: > Resurrecting this thread now that I have a system with launch control > and have been able to measure the performance impact... > > Regenerating the EINIT token every time adds somewhere in the vicinity > of ~5% overhead to creating an enclave, versus generating a token once > and reusing it in each EINIT call. This isn't a huge issue since real > world usage models likely won't be re-launching enclaves at a high rate, > but it is measurable. We can cache tokens in future in the kernel space, can't we? /Jarkko
RE: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Wed, Nov 15, 2017 at 10:20:27AM -0800, Sean Christopherson wrote: > On Tue, 2017-11-14 at 22:28 +0200, Jarkko Sakkinen wrote: > > On Tue, Nov 14, 2017 at 09:55:06AM -0800, Sean Christopherson wrote: > > > > > > What do you mean by bottlenecks? Assuming you're referring to performance > > > bottlenecks, this statement is flat out false. Moving the launch enclave > > > into > > > the kernel introduces performance bottlenecks, e.g. as implemented, a > > > single > > > LE > > > services all EINIT requests and is protected by a mutex. That is the very > > > definition of a bottleneck. > > I guess the text does not do a good job describing what I meant. Maybe I > > should refine it? Your argument about mutex is correct. > > > > The use of "bottleneck" does not specifically refer to performance. I'm > > worried about splitting the tasks needed to launch an enclave between > > kernel and user space. It could become difficult to manage when more > > SGX features are added. That is what I was referring when I used the > > word "bottleneck". > > > > I suppose you think I should refine the commit message? > > > > About the perf bottleneck. Given that all the data is already in > > sgx_le_ctx the driver could for example have own LE process for every > > opened /dev/sgx. Is your comment also suggesting to refine this or > > could it be postponed? > > More that I don't understand why the driver doesn't allow userspace to provide > an EINIT token, and reciprocally, doesn't provide the token back to > userspace. > IMO, the act of generating an EINIT token is orthogonal to deciding whether or > not to run the enclave. Running code in a kernel-owned enclave is not > specific > to SGX, e.g. paranoid kernels could run other sensitive tasks in an enclave. > Being forced to run an enclave to generate an EINIT token is an unfortunate > speed bump that exists purely because hardware doesn't provide the option to > disable launch control entirely. > > In other words, accepting a token via the IOCTL doesn't mean the driver has to > use it, e.g. it can always ignore the token, enforce periodic reverification, > check that the token was created by the driver, etc... And using the token > doesn't preclude the driver from re-running its verification checks outside of > the launch enclave. Resurrecting this thread now that I have a system with launch control and have been able to measure the performance impact... Regenerating the EINIT token every time adds somewhere in the vicinity of ~5% overhead to creating an enclave, versus generating a token once and reusing it in each EINIT call. This isn't a huge issue since real world usage models likely won't be re-launching enclaves at a high rate, but it is measurable. On top of my other arguments, the key of the token's signer must match the current value in the LE hash MSRs, so except for future theoretical scenarios where we want to "revoke" an existing token, the only way we can end up with a token we don't trust is if the kernel launch enclave already screwed up or userspace has access to the LE's private key.
Re: [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Tue, Nov 14, 2017 at 10:12:25PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 14, 2017 at 10:41:50AM +1100, James Morris wrote:
> > > + if (IS_ERR(secs_epc)) {
> > > + ret = PTR_ERR(secs_epc);
> > > + goto out;
> > > + }
> >
> > > +out:
> > > + if (encl)
> > > + kref_put(&encl->refcount, sgx_encl_release);
> > > + return ret;
> > > +}
> >
> > Don't you need an sgx_free_page() somewhere here?
>
> It will get freed by sgx_encl_release().
The call flow was somewhat messy so I just moved sgx_encl_alloc() call
outside of sgx_encl_create().
/Jarkko
Re: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Tue, 2017-11-14 at 22:28 +0200, Jarkko Sakkinen wrote: > On Tue, Nov 14, 2017 at 09:55:06AM -0800, Sean Christopherson wrote: > > > > What do you mean by bottlenecks? Assuming you're referring to performance > > bottlenecks, this statement is flat out false. Moving the launch enclave > > into > > the kernel introduces performance bottlenecks, e.g. as implemented, a single > > LE > > services all EINIT requests and is protected by a mutex. That is the very > > definition of a bottleneck. > I guess the text does not do a good job describing what I meant. Maybe I > should refine it? Your argument about mutex is correct. > > The use of "bottleneck" does not specifically refer to performance. I'm > worried about splitting the tasks needed to launch an enclave between > kernel and user space. It could become difficult to manage when more > SGX features are added. That is what I was referring when I used the > word "bottleneck". > > I suppose you think I should refine the commit message? > > About the perf bottleneck. Given that all the data is already in > sgx_le_ctx the driver could for example have own LE process for every > opened /dev/sgx. Is your comment also suggesting to refine this or > could it be postponed? More that I don't understand why the driver doesn't allow userspace to provide an EINIT token, and reciprocally, doesn't provide the token back to userspace. IMO, the act of generating an EINIT token is orthogonal to deciding whether or not to run the enclave. Running code in a kernel-owned enclave is not specific to SGX, e.g. paranoid kernels could run other sensitive tasks in an enclave. Being forced to run an enclave to generate an EINIT token is an unfortunate speed bump that exists purely because hardware doesn't provide the option to disable launch control entirely. In other words, accepting a token via the IOCTL doesn't mean the driver has to use it, e.g. it can always ignore the token, enforce periodic reverification, check that the token was created by the driver, etc... And using the token doesn't preclude the driver from re-running its verification checks outside of the launch enclave. > The driver architecture already allows to scale this but it is not > nearly as bad issue as the one Dave pointed out. > > /Jarkko > ___ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-...@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Re: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Tue, Nov 14, 2017 at 09:55:06AM -0800, Sean Christopherson wrote: > What do you mean by bottlenecks? Assuming you're referring to performance > bottlenecks, this statement is flat out false. Moving the launch enclave into > the kernel introduces performance bottlenecks, e.g. as implemented, a single > LE > services all EINIT requests and is protected by a mutex. That is the very > definition of a bottleneck. I guess the text does not do a good job describing what I meant. Maybe I should refine it? Your argument about mutex is correct. The use of "bottleneck" does not specifically refer to performance. I'm worried about splitting the tasks needed to launch an enclave between kernel and user space. It could become difficult to manage when more SGX features are added. That is what I was referring when I used the word "bottleneck". I suppose you think I should refine the commit message? About the perf bottleneck. Given that all the data is already in sgx_le_ctx the driver could for example have own LE process for every opened /dev/sgx. Is your comment also suggesting to refine this or could it be postponed? The driver architecture already allows to scale this but it is not nearly as bad issue as the one Dave pointed out. /Jarkko
Re: [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Tue, Nov 14, 2017 at 10:41:50AM +1100, James Morris wrote:
> On Mon, 13 Nov 2017, Jarkko Sakkinen wrote:
>
> > +
> > + secs_epc = sgx_alloc_page(0);
>
> Use SGX_ALLOC_ATOMIC instead of 0 ?
It is used in the #PF handler where it is not safe to swap pages in the
call context. I.e. you cannot lock mmap_sem without potentially causing
a deadlock.
> > + if (IS_ERR(secs_epc)) {
> > + ret = PTR_ERR(secs_epc);
> > + goto out;
> > + }
>
> > +out:
> > + if (encl)
> > + kref_put(&encl->refcount, sgx_encl_release);
> > + return ret;
> > +}
>
> Don't you need an sgx_free_page() somewhere here?
It will get freed by sgx_encl_release().
> --
> James Morris
>
/Jarkko
Re: [intel-sgx-kernel-dev] [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Mon, 2017-11-13 at 21:45 +0200, Jarkko Sakkinen wrote: > Intel SGX is a set of CPU instructions that can be used by applications > to set aside private regions of code and data. The code outside the > enclave is disallowed to access the memory inside the enclave by the CPU > access control. > > SGX driver provides a ioctl API for loading and initializing enclaves. > Address range for enclaves is reserved with mmap() and they are > destroyed with munmap(). Enclave construction, measurement and > initialization is done with the provided the ioctl API. > > The driver implements also a swapper thread ksgxswapd for EPC pages > backed by a private shmem file. Currently it has a limitation of not > swapping VA pages but there is nothing preventing to implement it later > on. Now it was scoped out in order to keep the implementation simple. > > The parameter struct for SGX_IOC_ENCLAVE_INIT does not contain a > parameter to supply a launch token. Generating and using tokens is best > to be kept in the control of the kernel because it has direct binding to > the IA32_SGXPUBKEYHASHx MSRs (a core must have MSRs set to the same > value as the signer of token). > > By giving user space any role in the launch process is a risk for > introducing bottlenecks as kernel must exhibit behavior that user space > launch daemon depends on What do you mean by bottlenecks? Assuming you're referring to performance bottlenecks, this statement is flat out false. Moving the launch enclave into the kernel introduces performance bottlenecks, e.g. as implemented, a single LE services all EINIT requests and is protected by a mutex. That is the very definition of a bottleneck. The kernel will never be as performant as userspace when it comes to EINIT tokens because userspace can make informed decisions based on its usage model, e.g. number of LEs (or LE threads) to spawn, LE and token lifecycles, LE and token thread safety, etc... > , properietary risks (closed launch daemons on > closed platforms) This justifies the need for the kernel to be able to generate launch tokens, but I don't think allowing userspace to also provide its own tokens adds any proprietary risks. > and stability risks as there would be division of > semantics between user space and kernel. > What exactly are the stability risks? The token itself is architecturally defined and isn't fundamentally different than e.g. the sigstruct. Writing the LE hash MSRs as needed, e.g. for userspace LEs, isn't difficult. > Signed-off-by: Jarkko Sakkinen > ---
Re: [PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
On Mon, 13 Nov 2017, Jarkko Sakkinen wrote:
> +
> + secs_epc = sgx_alloc_page(0);
Use SGX_ALLOC_ATOMIC instead of 0 ?
> + if (IS_ERR(secs_epc)) {
> + ret = PTR_ERR(secs_epc);
> + goto out;
> + }
> +out:
> + if (encl)
> + kref_put(&encl->refcount, sgx_encl_release);
> + return ret;
> +}
Don't you need an sgx_free_page() somewhere here?
--
James Morris
[PATCH v5 06/11] intel_sgx: driver for Intel Software Guard Extensions
Intel SGX is a set of CPU instructions that can be used by applications to set aside private regions of code and data. The code outside the enclave is disallowed to access the memory inside the enclave by the CPU access control. SGX driver provides a ioctl API for loading and initializing enclaves. Address range for enclaves is reserved with mmap() and they are destroyed with munmap(). Enclave construction, measurement and initialization is done with the provided the ioctl API. The driver implements also a swapper thread ksgxswapd for EPC pages backed by a private shmem file. Currently it has a limitation of not swapping VA pages but there is nothing preventing to implement it later on. Now it was scoped out in order to keep the implementation simple. The parameter struct for SGX_IOC_ENCLAVE_INIT does not contain a parameter to supply a launch token. Generating and using tokens is best to be kept in the control of the kernel because it has direct binding to the IA32_SGXPUBKEYHASHx MSRs (a core must have MSRs set to the same value as the signer of token). By giving user space any role in the launch process is a risk for introducing bottlenecks as kernel must exhibit behavior that user space launch daemon depends on, properietary risks (closed launch daemons on closed platforms) and stability risks as there would be division of semantics between user space and kernel. Signed-off-by: Jarkko Sakkinen --- arch/x86/include/asm/sgx.h | 233 ++ arch/x86/include/asm/sgx_arch.h | 268 +++ arch/x86/include/uapi/asm/sgx.h | 138 drivers/platform/x86/Kconfig| 2 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_sgx/Kconfig | 19 + drivers/platform/x86/intel_sgx/Makefile | 13 + drivers/platform/x86/intel_sgx/sgx.h| 254 ++ drivers/platform/x86/intel_sgx/sgx_encl.c | 988 drivers/platform/x86/intel_sgx/sgx_ioctl.c | 277 +++ drivers/platform/x86/intel_sgx/sgx_main.c | 410 ++ drivers/platform/x86/intel_sgx/sgx_page_cache.c | 614 +++ drivers/platform/x86/intel_sgx/sgx_util.c | 372 + drivers/platform/x86/intel_sgx/sgx_vma.c| 117 +++ 14 files changed, 3706 insertions(+) create mode 100644 arch/x86/include/asm/sgx.h create mode 100644 arch/x86/include/asm/sgx_arch.h create mode 100644 arch/x86/include/uapi/asm/sgx.h create mode 100644 drivers/platform/x86/intel_sgx/Kconfig create mode 100644 drivers/platform/x86/intel_sgx/Makefile create mode 100644 drivers/platform/x86/intel_sgx/sgx.h create mode 100644 drivers/platform/x86/intel_sgx/sgx_encl.c create mode 100644 drivers/platform/x86/intel_sgx/sgx_ioctl.c create mode 100644 drivers/platform/x86/intel_sgx/sgx_main.c create mode 100644 drivers/platform/x86/intel_sgx/sgx_page_cache.c create mode 100644 drivers/platform/x86/intel_sgx/sgx_util.c create mode 100644 drivers/platform/x86/intel_sgx/sgx_vma.c diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h new file mode 100644 index ..9c567c05a8bb --- /dev/null +++ b/arch/x86/include/asm/sgx.h @@ -0,0 +1,233 @@ +/* + * This file is provided under a dual BSD/GPLv2 license. When using or + * redistributing this file, you may do so under either license. + * + * GPL LICENSE SUMMARY + * + * Copyright(c) 2016-2017 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * Contact Information: + * Jarkko Sakkinen + * Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo + * + * BSD LICENSE + * + * Copyright(c) 2016-2017 Intel Corporation. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Intel Corporation nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + *
