Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 28, 2020 at 02:15:18PM +0300, Jarkko Sakkinen wrote: > On Thu, May 07, 2020 at 05:25:55PM -0700, Sean Christopherson wrote: > > Ah, fudge. shmem_zero_setup() triggers shmem_acct_size() and thus > > __vm_enough_memory(). Which I should have rememered because I've stared > > at that code several times when dealing with the enclave's backing store. > > I wasn't seeing the issue because I happened to use MAP_PRIVATE. > > > > So, bad analysis, good conclusion, i.e. the kernel is still doing the > > right thing, it's just not ideal for userspace. > > > > > > Jarkko, we should update the docs and selftest to recommend and use > > > > PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS > > > > or > > > > PROT_NONE, MAP_SHARED | MAP_NORESERVE | MAP_ANONYMOUS" > > > > when carving out ELRANGE, with an explicit comment that all the normal > > rules for mapping memory still apply. > > Ugh, had forgotten this. > > OK, I guess this comment explains it all: > > " > /* > * shmem_file_setup pre-accounts the whole fixed size of a VM object, > * for shared memory and for shared anonymous (/dev/zero) mappings > * (unless MAP_NORESERVE and sysctl_overcommit_memory <= 1), > * consistent with the pre-accounting of private mappings ... > */ > static inline int shmem_acct_size(unsigned long flags, loff_t size) > " Do not agree though that any documentation should be produced but the selftest should have correct parameters, yes. Instructions on how to reserve a range of addresses simply does not belong to SGX documentation because it is not SGX related in the first place. The patterns you showed are universal. I'll fix just the selftest for v31. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 07, 2020 at 05:25:55PM -0700, Sean Christopherson wrote: > Ah, fudge. shmem_zero_setup() triggers shmem_acct_size() and thus > __vm_enough_memory(). Which I should have rememered because I've stared > at that code several times when dealing with the enclave's backing store. > I wasn't seeing the issue because I happened to use MAP_PRIVATE. > > So, bad analysis, good conclusion, i.e. the kernel is still doing the > right thing, it's just not ideal for userspace. > > > Jarkko, we should update the docs and selftest to recommend and use > > PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS > > or > > PROT_NONE, MAP_SHARED | MAP_NORESERVE | MAP_ANONYMOUS" > > when carving out ELRANGE, with an explicit comment that all the normal > rules for mapping memory still apply. Ugh, had forgotten this. OK, I guess this comment explains it all: " /* * shmem_file_setup pre-accounts the whole fixed size of a VM object, * for shared memory and for shared anonymous (/dev/zero) mappings * (unless MAP_NORESERVE and sysctl_overcommit_memory <= 1), * consistent with the pre-accounting of private mappings ... */ static inline int shmem_acct_size(unsigned long flags, loff_t size) " /Jarkko
RE: [PATCH v29 00/20] Intel SGX foundations
From: Pavel Machek > Sent: 24 May 2020 22:27 .. > It provides false sense of security, and I'm afraid big companies will try to > force > people to use it. "DRM, now with hardware support". "Finally advertisments > you can > not skip". "Just run this piece of code on your machine to access your bank > account. > Trust us!" Hey malware guys, here is somewhere you can hide your code making it very difficult to find. You can then use the hardware disk encryption that people think gives them security to encrypt all the files. Job done... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v29 00/20] Intel SGX foundations
Hi! > > > At the very least a modular form of the driver should be > > > considered that would allow alternate implementations. Sean > > > indicated that there was a 'kludgy' approach that would allow an > > > alternate modular implementation alongside the in-kernel driver. > > > I believe that Andy has already indicated that may not be an > > > advisable approach. > > > Modularizing the the driver does nothing for your use case. The > > "driver" is a relatively lightweight wrapper, removing that is akin > > to making /dev/sgx/enclave inaccessible, i.e. it doesn't make the Well... SGX is proprietary feature of Intel. I don't see any effort for standartization so that other architectures could use it. Yet it provides userspace interface... You clearly want distros to enable it, but that will waste memory on non-Intel systems. That is not good. > Here in a nutshell is the paradox the kernel faces: > > No one seems to be disputing the fact that the primary focus of this > driver will be to support the notion of 'runtime encryption' and > Confidential Computing. The whole premise of the concept and economic > predicate of the initiative is that the owner/manager of the platform > has no visibility into what is being done on the platform. Well, in my eyes preventing owner of the machine from accessing all its parts is pretty questionable. Physics says it is impossible, many tried, and many failed. Why it should be different this time? > If the Linux community believes that standard platform security > controls can make qualitatively good judgements on what enclave based > execution is doing, it is effectively making the statement that the > very concept of runtime encryption and by extension Confidential > Computing is invalid. And yes, I believe that concept of Confidential Computing is invalid.. and we should simply not merge this support. It provides false sense of security, and I'm afraid big companies will try to force people to use it. "DRM, now with hardware support". "Finally advertisments you can not skip". "Just run this piece of code on your machine to access your bank account. Trust us!" :-(. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v29 00/20] Intel SGX foundations
On Fri, 2020-05-15 at 15:54 -0400, Nathaniel McCallum wrote: > The (Red Hat sponsored) Enarx project will continue building an > unofficial, unsupported version of the Fedora kernel with the SGX > patches[0] until such time as the patches are upstream. Once upstream, > I intend to propose that the feature be enabled in the stock Fedora > kernel. > > Enarx requires EDMM support as a prerequisite to being production > ready. Therefore, we are likely to continue building this custom > Fedora kernel with the latest patches until such point as EDMM support > lands upstream. This also implies that I have no current plan to > request an SGX backport to a RHEL kernel until such time as it > supports our full needs. > > Disclaimer: I do not control RHEL or Fedora kernel features. None of > the above constitutes a commitment to deliver anything. > > [0]: https://copr.fedorainfracloud.org/coprs/npmccallum/enarx/package/kernel/ SGX is somewhat self-contained feature, i.e. should be easy to backport for any recent kernel. Only the vDSO is outside of arch/x86/kernel/cpu/sgx. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 14, 2020 at 5:17 AM Dr. Greg wrote: > > On Fri, May 08, 2020 at 12:56:35PM -0700, Sean Christopherson wrote: > > Good morning, I hope the week is proceeding well for everyone. > > > On Fri, May 08, 2020 at 02:02:26PM -0500, Dr. Greg wrote: > > > On Thu, May 07, 2020 at 02:41:30AM +0200, Thomas Gleixner wrote: > > > > The diffstat of your patch is irrelevant. What's relevant is the > > > > fact that it is completely unreviewed and that it creates yet > > > > another user space visible ABI with a noticable lack of > > > > documentation. > > > > ... > > > > > As to lack of review, we would certainly welcome technical and API > > > comments but we cannot force them. > > > Thomas' point isn't that your code needs to be reviewed, it's that > > trying to squeeze it into the initial merge will, not might, _will_ > > push out the acceptance of SGX. The same is true for other features > > we have lined up, e.g. KVM and cgroup support. It's not a slight on > > your code, it's just reality at this point. > > For the record and for whatever it is worth at this point. The patch > has been available in its present form and function for around 14 > months. > > So there was plenty of time for its consideration and integration into > what is being prepared as the final merge candidate. > > > > In fact, anyone who reviews the patch will see that the current driver > > > creates a pointer in the ioctl call to pass downward into the hardware > > > initialization routines. Our code simply replaces that pointer with > > > one supplied by userspace. > > > Personally, I'm in favor of adding a reserved placeholder for a > > token so as to avoid adding a second ioctl() in the event that > > something gets upstreamed that wants the token. Ditto for a file > > descriptor for the backing store in sgx_enclave_create. > > That would be a very low cost and forward looking addition. > > > > At the very least a modular form of the driver should be > > > considered that would allow alternate implementations. Sean > > > indicated that there was a 'kludgy' approach that would allow an > > > alternate modular implementation alongside the in-kernel driver. > > > I believe that Andy has already indicated that may not be an > > > advisable approach. > > > Modularizing the the driver does nothing for your use case. The > > "driver" is a relatively lightweight wrapper, removing that is akin > > to making /dev/sgx/enclave inaccessible, i.e. it doesn't make the > > EPC tracking and core code go away. Modularizing the whole thing is > > flat out not an option, as doing so is not compatible with an EPC > > cgroup and adds unnecessary complexity to KVM enabling, both of > > which are highly desired features by pretty much everyone that plans > > on using SGX. > > All well taken technical points, but they don't speak directly to the > issue at hand. The potential security issue in all of this is access > to /dev/sgx/enclave, not the EPC tracking and core code. > > Here in a nutshell is the paradox the kernel faces: > > No one seems to be disputing the fact that the primary focus of this > driver will be to support the notion of 'runtime encryption' and > Confidential Computing. The whole premise of the concept and economic > predicate of the initiative is that the owner/manager of the platform > has no visibility into what is being done on the platform. > > If the Linux community believes that standard platform security > controls can make qualitatively good judgements on what enclave based > execution is doing, it is effectively making the statement that the > very concept of runtime encryption and by extension Confidential > Computing is invalid. > > If we accept the concept that Confidential Computing is valid then we > admit the technology provides the opportunity for unsupervised code > execution and data manipulation. > > Our premise in all of this is that one page of code implementing three > linked lists is a small price to pay to provide platform owners with > the opportunity to optionally implement what is effectively 2-factor > authentication over this process. > > Going forward, if we are intellectually honest, all of this suggests > that adding complexity to the driver with LSM controls makes little > sense technically. Since, by definition and economic intent, the > technology provides tools and infrastructure that allows these > controls to be evaded. > > The notion that entities with adversarial intent would not try and > take advantage of this flies in the face of everything we know about > security. > > > Andy is right to caution against playing games to squish in-kernel > > things, but the virtualization snafu is a completely different > > beast. E.g. SGX doesn't require fiddling with CR4, won't corrupt > > random memory across kexec(), doesn't block INIT, etc... And I'm > > not advocating long-term shenanigans, I just wanted to point out > > that there are options for running out-of-tree drivers in the short >
Re: [PATCH v29 00/20] Intel SGX foundations
On Fri, 2020-05-15 at 11:42 +0200, Borislav Petkov wrote: > On Fri, May 15, 2020 at 12:28:54PM +0300, Jarkko Sakkinen wrote: > > Uh oh, should probably address this. Should I send v31 today with a "nosgx" > > patch added? Sorry for missing this one :-/ > > Not the whole thing - just the one patch as a reply to your thread pls. > > Thx. OK, cool, thank you. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Fri, May 15, 2020 at 12:28:54PM +0300, Jarkko Sakkinen wrote: > Uh oh, should probably address this. Should I send v31 today with a "nosgx" > patch added? Sorry for missing this one :-/ Not the whole thing - just the one patch as a reply to your thread pls. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, 2020-05-14 at 18:20 +0200, Borislav Petkov wrote: > On Thu, May 14, 2020 at 09:15:59AM -0700, Sean Christopherson wrote: > > I'm not opposed to adding a kernel param to disable SGX. At one point > > there was a proposal to extend clearcpuid to allow disabling multiple > > feature bits, but it looks like that went the way of the dodo. > > > > Note, such a param would disable SGX entirely, e.g. clear the feature bit > > in /proc/cpuinfo and prevent any in-kernel SGX code from running. > > It is a usual practice for big features like SGX to add a > "nosgx" cmdline param to disable it in case something goes > south. We do this for all features - see all "no*" switches in > Documentation/admin-guide/kernel-parameters.txt Uh oh, should probably address this. Should I send v31 today with a "nosgx" patch added? Sorry for missing this one :-/ /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, 2020-05-14 at 12:05 -0700, Seth Moore wrote: > On Fri, May 8, 2020 at 12:08 PM Sean Christopherson > wrote: > > Adding some Google folks to the party. > > Thanks, Sean. > > > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > > > Intel(R) 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. > > > > > > There is a new hardware unit in the processor called Memory Encryption > > > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > > > one or many MEE regions that can hold enclave data by configuring them > > > with > > > PRMRR registers. > > > > > > The MEE automatically encrypts the data leaving the processor package to > > > the MEE regions. The data is encrypted using a random key whose life-time > > > is exactly one power cycle. > > > > > > The current implementation requires that the firmware sets > > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > > decide what enclaves it wants run. The implementation does not create > > > any bottlenecks to support read-only MSRs later on. > > > > > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > > > > > cat /proc/cpuinfo | grep sgx > > We applied the v29 patches to Linux 5.6.0, then tested on Xeon(R) E-2186G > with Asylo (http://asylo.dev). > > Looks good. All Asylo tests pass. > > Tested-by: Seth Moore Thanks. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, 2020-05-14 at 21:30 +0200, Thomas Gleixner wrote: > Jarkko Sakkinen writes: > > General question: maybe it would be easiest that I issue a pull request > > once everyone feels that the series is ready to be pulled and stop sending > > new versions of the series? > > Might be the easiest for you, but I prefer a final series in email. > > Thanks, > > tglx For me both are just as easy or as hard :-) Just wanted to query the preference. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, 2020-05-14 at 09:15 -0700, Sean Christopherson wrote: > On Thu, May 14, 2020 at 04:16:37AM -0500, Dr. Greg wrote: > > What we would recommend at this point is that you and Jarkko do the > > Linux community and beyond a favor and wire up a simple kernel > > command-line parameter that controls the ability of the driver to be > > used, ie. enables/disables access to /dev/sgx/enclave. > > I'm not opposed to adding a kernel param to disable SGX. At one point > there was a proposal to extend clearcpuid to allow disabling multiple > feature bits, but it looks like that went the way of the dodo. > > Note, such a param would disable SGX entirely, e.g. clear the feature bit > in /proc/cpuinfo and prevent any in-kernel SGX code from running. Greg, you are free to submit a patch for review that adds any possible kernel command line parameter SGX and beyond. SGX support does not "wire up" anything that would prevent reviewing such patches. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
Jarkko Sakkinen writes: > > General question: maybe it would be easiest that I issue a pull request > once everyone feels that the series is ready to be pulled and stop sending > new versions of the series? Might be the easiest for you, but I prefer a final series in email. Thanks, tglx
Re: [PATCH v29 00/20] Intel SGX foundations
Borislav Petkov writes: > On Thu, May 14, 2020 at 09:15:59AM -0700, Sean Christopherson wrote: >> I'm not opposed to adding a kernel param to disable SGX. At one point >> there was a proposal to extend clearcpuid to allow disabling multiple >> feature bits, but it looks like that went the way of the dodo. clearcpuid is a trainwreck which should have never happened. >> Note, such a param would disable SGX entirely, e.g. clear the feature bit >> in /proc/cpuinfo and prevent any in-kernel SGX code from running. > > It is a usual practice for big features like SGX to add a > "nosgx" cmdline param to disable it in case something goes > south. We do this for all features - see all "no*" switches in > Documentation/admin-guide/kernel-parameters.txt Correct. Thanks, tglx
Re: [PATCH v29 00/20] Intel SGX foundations
On Fri, May 8, 2020 at 12:08 PM Sean Christopherson wrote: > > Adding some Google folks to the party. Thanks, Sean. > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > > Intel(R) 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. > > > > There is a new hardware unit in the processor called Memory Encryption > > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > > one or many MEE regions that can hold enclave data by configuring them with > > PRMRR registers. > > > > The MEE automatically encrypts the data leaving the processor package to > > the MEE regions. The data is encrypted using a random key whose life-time > > is exactly one power cycle. > > > > The current implementation requires that the firmware sets > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > decide what enclaves it wants run. The implementation does not create > > any bottlenecks to support read-only MSRs later on. > > > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > > > cat /proc/cpuinfo | grep sgx We applied the v29 patches to Linux 5.6.0, then tested on Xeon(R) E-2186G with Asylo (http://asylo.dev). Looks good. All Asylo tests pass. Tested-by: Seth Moore
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 14, 2020 at 09:15:59AM -0700, Sean Christopherson wrote: > I'm not opposed to adding a kernel param to disable SGX. At one point > there was a proposal to extend clearcpuid to allow disabling multiple > feature bits, but it looks like that went the way of the dodo. > > Note, such a param would disable SGX entirely, e.g. clear the feature bit > in /proc/cpuinfo and prevent any in-kernel SGX code from running. It is a usual practice for big features like SGX to add a "nosgx" cmdline param to disable it in case something goes south. We do this for all features - see all "no*" switches in Documentation/admin-guide/kernel-parameters.txt Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 14, 2020 at 04:16:37AM -0500, Dr. Greg wrote: > What we would recommend at this point is that you and Jarkko do the > Linux community and beyond a favor and wire up a simple kernel > command-line parameter that controls the ability of the driver to be > used, ie. enables/disables access to /dev/sgx/enclave. I'm not opposed to adding a kernel param to disable SGX. At one point there was a proposal to extend clearcpuid to allow disabling multiple feature bits, but it looks like that went the way of the dodo. Note, such a param would disable SGX entirely, e.g. clear the feature bit in /proc/cpuinfo and prevent any in-kernel SGX code from running.
Re: [PATCH v29 00/20] Intel SGX foundations
On Tue, 2020-05-12 at 19:55 +0800, Hui, Chunyang wrote: > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > > Intel(R) 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. > > > > There is a new hardware unit in the processor called Memory Encryption > > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > > one or many MEE regions that can hold enclave data by configuring them with > > PRMRR registers. > > > > The MEE automatically encrypts the data leaving the processor package to > > the MEE regions. The data is encrypted using a random key whose life-time > > is exactly one power cycle. > > > > The current implementation requires that the firmware sets > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > decide what enclaves it wants run. The implementation does not create > > any bottlenecks to support read-only MSRs later on. > > > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > > > cat /proc/cpuinfo | grep sgx > > Tested-by: Chunyang Hui > > Occlum project (https://github.com/occlum/occlum) is a libOS built on top of > Intel SGX feature. We ran Occlum tests using patch v29 on SGX hardware with > the Flexible Launch Control (FLC) feature and didn't find any problems. > As Occlum core developers, we would like these patches to be merged soon. Great, thanks adding tested by to the driver and reclaimer patch. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Fri, May 08, 2020 at 12:56:35PM -0700, Sean Christopherson wrote: Good morning, I hope the week is proceeding well for everyone. > On Fri, May 08, 2020 at 02:02:26PM -0500, Dr. Greg wrote: > > On Thu, May 07, 2020 at 02:41:30AM +0200, Thomas Gleixner wrote: > > > The diffstat of your patch is irrelevant. What's relevant is the > > > fact that it is completely unreviewed and that it creates yet > > > another user space visible ABI with a noticable lack of > > > documentation. > > ... > > > As to lack of review, we would certainly welcome technical and API > > comments but we cannot force them. > Thomas' point isn't that your code needs to be reviewed, it's that > trying to squeeze it into the initial merge will, not might, _will_ > push out the acceptance of SGX. The same is true for other features > we have lined up, e.g. KVM and cgroup support. It's not a slight on > your code, it's just reality at this point. For the record and for whatever it is worth at this point. The patch has been available in its present form and function for around 14 months. So there was plenty of time for its consideration and integration into what is being prepared as the final merge candidate. > > In fact, anyone who reviews the patch will see that the current driver > > creates a pointer in the ioctl call to pass downward into the hardware > > initialization routines. Our code simply replaces that pointer with > > one supplied by userspace. > Personally, I'm in favor of adding a reserved placeholder for a > token so as to avoid adding a second ioctl() in the event that > something gets upstreamed that wants the token. Ditto for a file > descriptor for the backing store in sgx_enclave_create. That would be a very low cost and forward looking addition. > > At the very least a modular form of the driver should be > > considered that would allow alternate implementations. Sean > > indicated that there was a 'kludgy' approach that would allow an > > alternate modular implementation alongside the in-kernel driver. > > I believe that Andy has already indicated that may not be an > > advisable approach. > Modularizing the the driver does nothing for your use case. The > "driver" is a relatively lightweight wrapper, removing that is akin > to making /dev/sgx/enclave inaccessible, i.e. it doesn't make the > EPC tracking and core code go away. Modularizing the whole thing is > flat out not an option, as doing so is not compatible with an EPC > cgroup and adds unnecessary complexity to KVM enabling, both of > which are highly desired features by pretty much everyone that plans > on using SGX. All well taken technical points, but they don't speak directly to the issue at hand. The potential security issue in all of this is access to /dev/sgx/enclave, not the EPC tracking and core code. Here in a nutshell is the paradox the kernel faces: No one seems to be disputing the fact that the primary focus of this driver will be to support the notion of 'runtime encryption' and Confidential Computing. The whole premise of the concept and economic predicate of the initiative is that the owner/manager of the platform has no visibility into what is being done on the platform. If the Linux community believes that standard platform security controls can make qualitatively good judgements on what enclave based execution is doing, it is effectively making the statement that the very concept of runtime encryption and by extension Confidential Computing is invalid. If we accept the concept that Confidential Computing is valid then we admit the technology provides the opportunity for unsupervised code execution and data manipulation. Our premise in all of this is that one page of code implementing three linked lists is a small price to pay to provide platform owners with the opportunity to optionally implement what is effectively 2-factor authentication over this process. Going forward, if we are intellectually honest, all of this suggests that adding complexity to the driver with LSM controls makes little sense technically. Since, by definition and economic intent, the technology provides tools and infrastructure that allows these controls to be evaded. The notion that entities with adversarial intent would not try and take advantage of this flies in the face of everything we know about security. > Andy is right to caution against playing games to squish in-kernel > things, but the virtualization snafu is a completely different > beast. E.g. SGX doesn't require fiddling with CR4, won't corrupt > random memory across kexec(), doesn't block INIT, etc... And I'm > not advocating long-term shenanigans, I just wanted to point out > that there are options for running out-of-tree drivers in the short > term, e.g. until proper policy controls land upstream. It appears that the distributions, at least IBM/RHEL, are going to compile this driver in and backport it as well. What we would recommend at this point is that you and Jar
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-05-14 00:14, Jarkko Sakkinen wrote: > General question: maybe it would be easiest that I issue a pull request > once everyone feels that the series is ready to be pulled and stop sending > new versions of the series? Sounds good -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, 2020-05-06 at 09:39 -0700, Jordan Hand wrote: > On 4/21/20 2:52 PM, Jarkko Sakkinen wrote: > > Intel(R) 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. > > > > There is a new hardware unit in the processor called Memory Encryption > > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > > one or many MEE regions that can hold enclave data by configuring them with > > PRMRR registers. > > > > The MEE automatically encrypts the data leaving the processor package to > > the MEE regions. The data is encrypted using a random key whose life-time > > is exactly one power cycle. > > > > The current implementation requires that the firmware sets > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > decide what enclaves it wants run. The implementation does not create > > any bottlenecks to support read-only MSRs later on. > > > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > > > cat /proc/cpuinfo | grep sgx > > > > v29: > > * The selftest has been moved to selftests/sgx. Because SGX is an execution > >environment of its own, it really isn't a great fit with more "standard" > >x86 tests. > > > >The RSA key is now generated on fly and the whole signing process has > >been made as part of the enclave loader instead of signing the enclave > >during the compilation time. > > > >Finally, the enclave loader loads now the test enclave directly from its > >ELF file, which means that ELF file does not need to be coverted as raw > >binary during the build process. > > * Version the mm_list instead of using on synchronize_mm() when adding new > >entries. We hold the write lock for the mm_struct, and dup_mm() can thus > >deadlock with the page reclaimer, which could hold the lock for the old > >mm_struct. > > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can > >be used to reserve the address range. Now /dev/sgx supports only opaque > >mappings to the (initialized) enclave data. > > * Make the vDSO callable directly from C by preserving RBX and taking leaf > >from RCX. > > > > Tested with the Open Enclave SDK on top of Intel PSW. Specifically built > the Intel PSW with changes to support /dev/sgx mapping[1] new in v29. > > Tested-by: Jordan Hand > > [1] https://github.com/intel/linux-sgx/pull/530 Thank you! /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, 2020-05-14 at 01:14 +0300, Jarkko Sakkinen wrote: > On Wed, 2020-05-06 at 17:42 -0400, Nathaniel McCallum wrote: > > Tested on Enarx. This requires a patch[0] for v29 support. > > > > Tested-by: Nathaniel McCallum > > Thank you. Update in my tree. > > Sean, I'll fixed that whitespace issue too in my tree. > > General question: maybe it would be easiest that I issue a pull request > once everyone feels that the series is ready to be pulled and stop sending > new versions of the series? My honest feelings about the series ATM are: 1. It is not perfect like no code never is and there are always issues. 2. Some things are very well matured, even more so than in a lot of mainline code I've seen. I'm particularly happy how the locking code has been converged. 3. Not worried to maintain the code in its current state. It is manageable. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, 2020-05-06 at 17:42 -0400, Nathaniel McCallum wrote: > Tested on Enarx. This requires a patch[0] for v29 support. > > Tested-by: Nathaniel McCallum Thank you. Update in my tree. Sean, I'll fixed that whitespace issue too in my tree. General question: maybe it would be easiest that I issue a pull request once everyone feels that the series is ready to be pulled and stop sending new versions of the series? /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Tue, May 12, 2020 at 07:55:58PM +0800, Hui, Chunyang wrote: > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > > > cat /proc/cpuinfo | grep sgx > > Tested-by: Chunyang Hui > Occlum project (https://github.com/occlum/occlum) is a libOS built > on top of Intel SGX feature. We ran Occlum tests using patch v29 on > SGX hardware with the Flexible Launch Control (FLC) feature and > didn't find any problems. As Occlum core developers, we would like > these patches to be merged soon. Do you use the Intel PSW or your own? Are you using the standard ECALL interface or did the tests run with the new VDSO entry and exception handler? Have a good day. Dr. Greg As always, Dr. Greg Wettstein, Ph.D, Worker Autonomously self-defensive Enjellic Systems Development, LLC IOT platforms and edge devices. 4206 N. 19th Ave. Fargo, ND 58102 PH: 701-281-1686 EMAIL: g...@enjellic.com -- "I had far rather walk, as I do, in daily terror of eternity, than feel that this was only a children's game in which all of the contestants would get equally worthless prizes in the end." -- T. S. Elliot
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > Intel(R) 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. > > There is a new hardware unit in the processor called Memory Encryption > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > one or many MEE regions that can hold enclave data by configuring them with > PRMRR registers. > > The MEE automatically encrypts the data leaving the processor package to > the MEE regions. The data is encrypted using a random key whose life-time > is exactly one power cycle. > > The current implementation requires that the firmware sets > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > decide what enclaves it wants run. The implementation does not create > any bottlenecks to support read-only MSRs later on. > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > cat /proc/cpuinfo | grep sgx Tested-by: Chunyang Hui Occlum project (https://github.com/occlum/occlum) is a libOS built on top of Intel SGX feature. We ran Occlum tests using patch v29 on SGX hardware with the Flexible Launch Control (FLC) feature and didn't find any problems. As Occlum core developers, we would like these patches to be merged soon. > v29: > * The selftest has been moved to selftests/sgx. Because SGX is an execution > environment of its own, it really isn't a great fit with more "standard" > x86 tests. > > The RSA key is now generated on fly and the whole signing process has > been made as part of the enclave loader instead of signing the enclave > during the compilation time. > > Finally, the enclave loader loads now the test enclave directly from its > ELF file, which means that ELF file does not need to be coverted as raw > binary during the build process. > * Version the mm_list instead of using on synchronize_mm() when adding new > entries. We hold the write lock for the mm_struct, and dup_mm() can thus > deadlock with the page reclaimer, which could hold the lock for the old > mm_struct. > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can > be used to reserve the address range. Now /dev/sgx supports only opaque > mappings to the (initialized) enclave data. > * Make the vDSO callable directly from C by preserving RBX and taking leaf > from RCX. > > v28: > * Documented to Documentation/x86/sgx.rst how the kernel manages the > enclave ownership. > * Removed non-LC flow from sgx_einit(). > * Removed struct sgx_einittoken since only the size of the corresponding > microarchitectural structure is used in the series ATM. > > v27: > * Disallow RIE processes to use enclaves as there could a permission > conflict between VMA and enclave permissions. > * In the documentation, replace "grep /proc/cpuinfo" with > "grep sgx /proc/cpuinfo". > > v26: > * Fixed the commit author in "x86/sgx: Linux Enclave Driver", which was > changed in v25 by mistake. > * Addressed a bunch of grammar mistakes in sgx.rst (thanks Randy once > again for such a detailed feedback). > * Added back the MAINTAINERS update commit, which was mistakenly removed > in v25. > * EREMOVE's for SECS cannot be done while sanitizing an EPC section. The > CPU does not allow to remove a SECS page before all of its children have > been removed, and a child page can be in some other section than the one > currently being processed. Thus, removed special SECS processing from > sgx_sanitize_page() and instead put sections through it twice. In the > 2nd round the lists should only contain SECS pages. > > v25: > * Fix a double-free issue when SGX_IOC_ENCLAVE_ADD_PAGES > fails on executing ENCLS[EADD]. The rollback path executed > radix_tree_delete() on the same address twice when this happened. > * Return -EINTR instead of -ERESTARTSYS in SGX_IOC_ENCLAVE_ADD_PAGES when > a signal is pending. > * As requested by Borislav, move the CPUID 0x12 features to their own word > in cpufeatures. > * Sean fixed a bug from sgx_reclaimer_write() where sgx_encl_put_backing() > was called with an uninitialized pointer when sgx_encl_get_backing() > fails. > * Migrated /dev/sgx/* to misc. This is future-proof as struct miscdevice > has 'groups' for setting up sysfs attributes for the device. > * Use device_initcall instead of subsys_initcall so that misc_class is > initialized before SGX is initialized. > * Return -EACCES in SGX_IOC_ENCLAVE_INIT when caller tries to select > enclave attributes that we the kernel does not allow it to set instead > of -EINVAL. > * Unless SGX public key MSRs are writable always deny the feature from > Linux. Previously this was only denied from driver. How VMs should be > supported is not really part of initial patch set, which makes this > an obvious
Re: [PATCH v29 00/20] Intel SGX foundations
On Fri, May 08, 2020 at 02:02:26PM -0500, Dr. Greg wrote: > On Thu, May 07, 2020 at 02:41:30AM +0200, Thomas Gleixner wrote: > > The diffstat of your patch is irrelevant. What's relevant is the > > fact that it is completely unreviewed and that it creates yet > > another user space visible ABI with a noticable lack of > > documentation. ... > As to lack of review, we would certainly welcome technical and API > comments but we cannot force them. Thomas' point isn't that your code needs to be reviewed, it's that trying to squeeze it into the initial merge will, not might, _will_ push out the acceptance of SGX. The same is true for other features we have lined up, e.g. KVM and cgroup support. It's not a slight on your code, it's just reality at this point. > In fact, anyone who reviews the patch will see that the current driver > creates a pointer in the ioctl call to pass downward into the hardware > initialization routines. Our code simply replaces that pointer with > one supplied by userspace. Personally, I'm in favor of adding a reserved placeholder for a token so as to avoid adding a second ioctl() in the event that something gets upstreamed that wants the token. Ditto for a file descriptor for the backing store in sgx_enclave_create. But, I have contributed exactly zero ioctls to the kernel, so take that with a big block of salt :-) > At the very least a modular form of the driver should be considered > that would allow alternate implementations. Sean indicated that there > was a 'kludgy' approach that would allow an alternate modular > implementation alongside the in-kernel driver. I believe that Andy > has already indicated that may not be an advisable approach. Modularizing the the driver does nothing for your use case. The "driver" is a relatively lightweight wrapper, removing that is akin to making /dev/sgx/enclave inaccessible, i.e. it doesn't make the EPC tracking and core code go away. Modularizing the whole thing is flat out not an option, as doing so is not compatible with an EPC cgroup and adds unnecessary complexity to KVM enabling, both of which are highly desired features by pretty much everyone that plans on using SGX. Andy is right to caution against playing games to squish in-kernel things, but the virtualization snafu is a completely different beast. E.g. SGX doesn't require fiddling with CR4, won't corrupt random memory across kexec(), doesn't block INIT, etc... And I'm not advocating long-term shenanigans, I just wanted to point out that there are options for running out-of-tree drivers in the short term, e.g. until proper policy controls land upstream.
Re: [PATCH v29 00/20] Intel SGX foundations
Adding some Google folks to the party. On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > Intel(R) 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. > > There is a new hardware unit in the processor called Memory Encryption > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > one or many MEE regions that can hold enclave data by configuring them with > PRMRR registers. > > The MEE automatically encrypts the data leaving the processor package to > the MEE regions. The data is encrypted using a random key whose life-time > is exactly one power cycle. > > The current implementation requires that the firmware sets > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > decide what enclaves it wants run. The implementation does not create > any bottlenecks to support read-only MSRs later on. > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > cat /proc/cpuinfo | grep sgx > > v29: > * The selftest has been moved to selftests/sgx. Because SGX is an execution > environment of its own, it really isn't a great fit with more "standard" > x86 tests. > > The RSA key is now generated on fly and the whole signing process has > been made as part of the enclave loader instead of signing the enclave > during the compilation time. > > Finally, the enclave loader loads now the test enclave directly from its > ELF file, which means that ELF file does not need to be coverted as raw > binary during the build process. > * Version the mm_list instead of using on synchronize_mm() when adding new > entries. We hold the write lock for the mm_struct, and dup_mm() can thus > deadlock with the page reclaimer, which could hold the lock for the old > mm_struct. > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can > be used to reserve the address range. Now /dev/sgx supports only opaque > mappings to the (initialized) enclave data. > * Make the vDSO callable directly from C by preserving RBX and taking leaf > from RCX. > > v28: > * Documented to Documentation/x86/sgx.rst how the kernel manages the > enclave ownership. > * Removed non-LC flow from sgx_einit(). > * Removed struct sgx_einittoken since only the size of the corresponding > microarchitectural structure is used in the series ATM. > > v27: > * Disallow RIE processes to use enclaves as there could a permission > conflict between VMA and enclave permissions. > * In the documentation, replace "grep /proc/cpuinfo" with > "grep sgx /proc/cpuinfo". > > v26: > * Fixed the commit author in "x86/sgx: Linux Enclave Driver", which was > changed in v25 by mistake. > * Addressed a bunch of grammar mistakes in sgx.rst (thanks Randy once > again for such a detailed feedback). > * Added back the MAINTAINERS update commit, which was mistakenly removed > in v25. > * EREMOVE's for SECS cannot be done while sanitizing an EPC section. The > CPU does not allow to remove a SECS page before all of its children have > been removed, and a child page can be in some other section than the one > currently being processed. Thus, removed special SECS processing from > sgx_sanitize_page() and instead put sections through it twice. In the > 2nd round the lists should only contain SECS pages. > > v25: > * Fix a double-free issue when SGX_IOC_ENCLAVE_ADD_PAGES > fails on executing ENCLS[EADD]. The rollback path executed > radix_tree_delete() on the same address twice when this happened. > * Return -EINTR instead of -ERESTARTSYS in SGX_IOC_ENCLAVE_ADD_PAGES when > a signal is pending. > * As requested by Borislav, move the CPUID 0x12 features to their own word > in cpufeatures. > * Sean fixed a bug from sgx_reclaimer_write() where sgx_encl_put_backing() > was called with an uninitialized pointer when sgx_encl_get_backing() > fails. > * Migrated /dev/sgx/* to misc. This is future-proof as struct miscdevice > has 'groups' for setting up sysfs attributes for the device. > * Use device_initcall instead of subsys_initcall so that misc_class is > initialized before SGX is initialized. > * Return -EACCES in SGX_IOC_ENCLAVE_INIT when caller tries to select > enclave attributes that we the kernel does not allow it to set instead > of -EINVAL. > * Unless SGX public key MSRs are writable always deny the feature from > Linux. Previously this was only denied from driver. How VMs should be > supported is not really part of initial patch set, which makes this > an obvious choice. > * Cleaned up and refined documentation to be more approachable. > > v24: > * Reclaim unmeasured and TCS pages (regression in v23). > * Replace usages of GFP_HIGHUSER with GFP_KERNEL. > * Return -EIO on when EADD or EEXTEND fails in %SGX_IOC_ENCLAVE_ADD_PAGES > and use t
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 07, 2020 at 02:41:30AM +0200, Thomas Gleixner wrote: > Greg, Good morning Thomas, I hope the week has gone well for you, the same to everyone else reading this. > "Dr. Greg" writes: > > As an aside, for those who haven't spent the last 5+ years of their > > life working with this technology. SGX2 hardware platforms have the > > ability to allow unrestricted code execution in enclave context. > Unrestricted code execution even before loaded? Unrestricted by > priviledge levels? The LSM/IMA infrastructure will have no visibility into code that will be executed and data processed in enclave context, see immediately below. > > No amount of LSM or IMA interventions can provide any control over > > that. > They can restrict what is started and loaded before anything SGX > happens. At best, the visibility and inspection will be over a standard bootstrap loader of some type is in no way related to the actual code that will be executed or data processed in enclave context. Furthermore, given what is becoming the excepted software architecture for the SGX industry, that code will largely have full access to system resources. > If you want to make real technical arguments then please be specific > and precise and spare us the handwaving marketing speak. Thomas, you don't know me from boo, those that do know me and have worked for me would tell you with absolute certainty that being a 'handwavy marketing type' is the complete antithesis of my character and how I practice engineering. Andy wanted to know why I felt the current driver disadvantaged our work, I provided a technical summary, omitted completely in your response, that indicated how we are using SGX technology in a manner that is inherently different from the rest of the industry. > > In fact, the Confidential Computing Consortium, sponsored by none > > other then the Linux Foundation, has at its fundamental tenant, > And that's relevant to the technical question in which way? It speaks directly to the primary marketing objective that is driving the economics of what this technology is going to turn into. The metaphor for objective, that now seems to be generally accepted, is 'Runtime Encryption'. One of the most common threads on the SGX developer's list is how developers can restrict the ability of other's to see what their enclaves are doing or the code in them. The standard response is 'nothing', a security context has to be established through remote attestation and the confidential material then conveyed into the enclave using appropriate confidentiality and integrity protections. As further technical evidence for all of this, I would refer readers to the following tag in the Intel Linux SGX SDK: sgx_2.1.3 That tags the release that introduced the implementation of Protected Code Loader (PCL) support. This allows developers to create enclaves that are encrypted at rest and then decrypted, loaded and executed by a second bootstrap loader enclave that protects the confidentiality of the first enclave. SGX2 hardware support makes the concept of protected code loading, at once, more practical, more efficient and closes a visibility vulnerability that the current PCL model has. The impact of this has to be viewed in the context of the economic market forces that are driving 'runtime encryption'. The original SGX programming model promoted by Intel was to partition applications so that sensitive data, and the code that operated on it, were inside of an enclave. That approach was doomed by the economic factors driving software development which are roughly as follows: 1.) Ease of development. 2.) Cost of development. 3.) Time to market. 4.) Return on investment. 5.) Security In about that order, although there are probably a half dozen other factors between 4 and 5. As a result, the 'runtime encryption' industry moved to a library OS model where unmodified applications can be run in an enclave along along with full interpreter environments such as Java, Python, Javascript and WASM. In this architecture all of the requests for operating system resources are shimmed through OCALL's to be serviced by the OS. If you combine all of these factors and influences, you end up with, looking forward, to an architecture that will favor a small bootstrap loader that downloads and executes code, over protected channels, that has almost full access to operating system resources. In this model the only visibility that the platform owner has, and by extension the LSM/IMA infrastructure, is the bootstrap loader itself. This architecture will also be driven by how attractive the concept is to the devops model and cloud orchestration in general. Our technical arguement is that it does not seem unreasonable for the Linux driver, at the discretion of the platform owner, to be able to implement the equivalent of 2-factor authentication over the initiation of such execution. I apologize for the detail and hope it is at once both su
Re: [PATCH v29 00/20] Intel SGX foundations
On 5/7/20 11:06 AM, Dr. Greg wrote: On Wed, May 06, 2020 at 09:39:55AM -0700, Jordan Hand wrote: Good afternoon, I hope the week is going well for everyone. On 4/21/20 2:52 PM, Jarkko Sakkinen wrote: Make the vDSO callable directly from C by preserving RBX and taking leaf from RCX. Tested with the Open Enclave SDK on top of Intel PSW. Specifically built the Intel PSW with changes to support /dev/sgx mapping[1] new in v29. Tested-by: Jordan Hand [1] https://github.com/intel/linux-sgx/pull/530 Did you re-wire your SDK to convert all your ECALL and exception handling to the new VDSO architecture? > No. We have many users of our SDK who rely on the out-of-tree driver and will for the foreseeable future. I aim to support both in-tree and out-of-tree with minimal code diff. Failures in enclave loading and initialization demonstrate themselves pretty clearly and are in the domain of the PSW being used. If there are going to be subtle SGX application operability issues that need to be found they will be in the new ECALL and exception handling mechanisms. Fair enough, no I have not tested those mechanisms. Apologies, I should have removed that line from the quoted text. -Jordan
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, Apr 29, 2020 at 8:30 AM Sean Christopherson wrote: > > On Sun, Apr 26, 2020 at 11:57:53AM -0500, Dr. Greg wrote: > > In closing, it is important to note that the proposed SGX driver is > > not available as a module. This effectively excludes any alternative > > implementations of the driver without replacement of the kernel at > > large. > > No it doesn't. The SGX subsytem won't allocate EPC pages unless userspace > creates an enclave, i.e. preventing unprivileged userspace from accessing > /dev/sgx/enclave will allow loading an alternative out-of-tree SGX module. > Yes, SGX sanitizes the EPC on boot, but that's arguably a good thing for > out-of-tree modules. > > And if you want to get crafty and squash in-kernel SGX altogether, boot > with "clearcpuid=" and/or "clearcpuid=" to disable in-kernel > support entirely. SGX won't be correctly enumerated in /proc/cpuinfo > relative to the existence of an out-of-tree module, but that seems like a > very minor issue if you're running with a completely different SGX driver. > > > It also means that any platform, with SGX hardware support, > > running a kernel with this driver, has the potential for the > > security/privacy issues noted above. > > Unless I'm mistaken, /dev/sgx is root-only by default. There are far > scarier mechanisms available to root for hosing the system. > > > If key based policy management is not allowed, then the driver needs > > to be re-architected to have modular support so that alternative > > implementations or the absence of any driver support are at least > > tenable. > > As above, using an alternative implementation is teneble, albeit a bit > kludgy. It is worth noting that, if someone actualy does this, and a future kernel patch breaks it, the upstream developers are unlikely to apologize or even feel particularly bad. See, for example, the current situation with VirtualBox.
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 07, 2020 at 05:35:31PM -0500, Haitao Huang wrote: > On Thu, 07 May 2020 14:34:59 -0500, Sean Christopherson > wrote: > > >On Thu, May 07, 2020 at 12:49:15PM -0400, Nathaniel McCallum wrote: > >>> For larger size mmap, I think it requires enabling vm overcommit mode > >>1: > >>> echo 1 | sudo tee /proc/sys/vm/overcommit_memory > > > >It shouldn't unless the initial mmap is "broken". Not truly broken, but > >broken in the sense that what Enarx is asking for is not actually what it > >desires. > > > So I tried, this passes with mode 1 but fail with ENOMEM by default: > > mmap(NULL, 0x1000UL, PROT_NONE, MAP_SHARED| MAP_ANONYMOUS, -1, 0); Ah, fudge. shmem_zero_setup() triggers shmem_acct_size() and thus __vm_enough_memory(). Which I should have rememered because I've stared at that code several times when dealing with the enclave's backing store. I wasn't seeing the issue because I happened to use MAP_PRIVATE. So, bad analysis, good conclusion, i.e. the kernel is still doing the right thing, it's just not ideal for userspace. Jarkko, we should update the docs and selftest to recommend and use PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS or PROT_NONE, MAP_SHARED | MAP_NORESERVE | MAP_ANONYMOUS" when carving out ELRANGE, with an explicit comment that all the normal rules for mapping memory still apply.
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, 07 May 2020 14:34:59 -0500, Sean Christopherson wrote: On Thu, May 07, 2020 at 12:49:15PM -0400, Nathaniel McCallum wrote: On Thu, May 7, 2020 at 1:03 AM Haitao Huang wrote: > > On Wed, 06 May 2020 17:14:22 -0500, Sean Christopherson > wrote: > > > On Wed, May 06, 2020 at 05:42:42PM -0400, Nathaniel McCallum wrote: > >> Tested on Enarx. This requires a patch[0] for v29 support. > >> > >> Tested-by: Nathaniel McCallum > >> > >> However, we did uncover a small usability issue. See below. > >> > >> [0]: > >> https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662 > > > > ... > > > >> > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. > >> anonymous) can > >> > be used to reserve the address range. Now /dev/sgx supports only > >> opaque > >> > mappings to the (initialized) enclave data. > >> > >> The statement "Any mapping..." isn't actually true. Yeah, this definitely misleading. I haven't looked at our most recent docs, but I'm going to go out on a limb and assume we haven't documented the preferred mechanism for carving out virtual memory for the enclave. That absolutely should be done. > >> Enarx creates a large enclave (currently 64GiB). This worked when we > >> created a file-backed mapping on /dev/sgx/enclave. However, switching > >> to an anonymous mapping fails with ENOMEM. We suspect this is because > >> the kernel attempts to allocate all the pages and zero them but there > >> is insufficient RAM available. We currently work around this by > >> creating a shared mapping on /dev/zero. > > > > Hmm, the kernel shouldn't actually allocate physical pages unless they're > > written. I'll see if I can reproduce. > > > > For larger size mmap, I think it requires enabling vm overcommit mode 1: > echo 1 | sudo tee /proc/sys/vm/overcommit_memory It shouldn't unless the initial mmap is "broken". Not truly broken, but broken in the sense that what Enarx is asking for is not actually what it desires. So I tried, this passes with mode 1 but fail with ENOMEM by default: mmap(NULL, 0x1000UL, PROT_NONE, MAP_SHARED| MAP_ANONYMOUS, -1, 0);
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, 07 May 2020 11:49:15 -0500, Nathaniel McCallum wrote: On Thu, May 7, 2020 at 1:03 AM Haitao Huang wrote: On Wed, 06 May 2020 17:14:22 -0500, Sean Christopherson wrote: > On Wed, May 06, 2020 at 05:42:42PM -0400, Nathaniel McCallum wrote: >> Tested on Enarx. This requires a patch[0] for v29 support. >> >> Tested-by: Nathaniel McCallum >> >> However, we did uncover a small usability issue. See below. >> >> [0]: >> https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662 > > ... > >> > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. >> anonymous) can >> > be used to reserve the address range. Now /dev/sgx supports only >> opaque >> > mappings to the (initialized) enclave data. >> >> The statement "Any mapping..." isn't actually true. >> >> Enarx creates a large enclave (currently 64GiB). This worked when we >> created a file-backed mapping on /dev/sgx/enclave. However, switching >> to an anonymous mapping fails with ENOMEM. We suspect this is because >> the kernel attempts to allocate all the pages and zero them but there >> is insufficient RAM available. We currently work around this by >> creating a shared mapping on /dev/zero. > > Hmm, the kernel shouldn't actually allocate physical pages unless they're > written. I'll see if I can reproduce. > For larger size mmap, I think it requires enabling vm overcommit mode 1: echo 1 | sudo tee /proc/sys/vm/overcommit_memory Which means the default experience isn't good. Yes, it is not good default. But this is not sgx specific IIUC. Normal applications would have the same issue if they ask for large mapping than whatever limit kernel enforces by default.
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 07, 2020 at 12:49:15PM -0400, Nathaniel McCallum wrote: > On Thu, May 7, 2020 at 1:03 AM Haitao Huang > wrote: > > > > On Wed, 06 May 2020 17:14:22 -0500, Sean Christopherson > > wrote: > > > > > On Wed, May 06, 2020 at 05:42:42PM -0400, Nathaniel McCallum wrote: > > >> Tested on Enarx. This requires a patch[0] for v29 support. > > >> > > >> Tested-by: Nathaniel McCallum > > >> > > >> However, we did uncover a small usability issue. See below. > > >> > > >> [0]: > > >> https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662 > > > > > > ... > > > > > >> > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. > > >> anonymous) can > > >> > be used to reserve the address range. Now /dev/sgx supports only > > >> opaque > > >> > mappings to the (initialized) enclave data. > > >> > > >> The statement "Any mapping..." isn't actually true. Yeah, this definitely misleading. I haven't looked at our most recent docs, but I'm going to go out on a limb and assume we haven't documented the preferred mechanism for carving out virtual memory for the enclave. That absolutely should be done. > > >> Enarx creates a large enclave (currently 64GiB). This worked when we > > >> created a file-backed mapping on /dev/sgx/enclave. However, switching > > >> to an anonymous mapping fails with ENOMEM. We suspect this is because > > >> the kernel attempts to allocate all the pages and zero them but there > > >> is insufficient RAM available. We currently work around this by > > >> creating a shared mapping on /dev/zero. > > > > > > Hmm, the kernel shouldn't actually allocate physical pages unless they're > > > written. I'll see if I can reproduce. > > > > > > > For larger size mmap, I think it requires enabling vm overcommit mode 1: > > echo 1 | sudo tee /proc/sys/vm/overcommit_memory It shouldn't unless the initial mmap is "broken". Not truly broken, but broken in the sense that what Enarx is asking for is not actually what it desires. > Which means the default experience isn't good. What PROT_* and MAP_* flags are passed to mmap()? Overcommit only applies to VM_WRITE (a.k.a. PROT_WRITE) && !VM_SHARED && !VM_NORESERVED and, ignoring rlimits, VM expansion only applies to VM_WRITE && !VM_SHARED && !VM_STACK So hopefully Enarx is doing something like base = mmap(NULL, 64gb, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); because that means this is effectively a userspace bug. This goes back to my comment about the mmap() being "broken". Userspace is asking for a writable, private mapping, in which case it absolutely should be accounted. If using base = mmap(NULL, 64gb, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); works, then updating the SGX docs to better explain how to establish ELRANGE is sufficient (we need to that in any case). If the above still fails then something else is in play.
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, May 06, 2020 at 09:39:55AM -0700, Jordan Hand wrote: Good afternoon, I hope the week is going well for everyone. > On 4/21/20 2:52 PM, Jarkko Sakkinen wrote: > > Make the vDSO callable directly from C by preserving RBX and taking leaf > > from RCX. > Tested with the Open Enclave SDK on top of Intel PSW. Specifically built > the Intel PSW with changes to support /dev/sgx mapping[1] new in v29. > > Tested-by: Jordan Hand > > [1] https://github.com/intel/linux-sgx/pull/530 Did you re-wire your SDK to convert all your ECALL and exception handling to the new VDSO architecture? Failures in enclave loading and initialization demonstrate themselves pretty clearly and are in the domain of the PSW being used. If there are going to be subtle SGX application operability issues that need to be found they will be in the new ECALL and exception handling mechanisms. Have a good remainder of the day. Dr. Greg As always, Dr. Greg Wettstein, Ph.D, Worker Artisans in autonomously Enjellic Systems Development, LLC self-defensive IOT platforms 4206 N. 19th Ave. and edge devices. Fargo, ND 58102 PH: 701-281-1686 EMAIL: g...@enjellic.com -- "Davidsen's first rule of system administration: He learns to swim fastest who is thrown in the deepest water." -- Bill Davidsen
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 7, 2020 at 1:03 AM Haitao Huang wrote: > > On Wed, 06 May 2020 17:14:22 -0500, Sean Christopherson > wrote: > > > On Wed, May 06, 2020 at 05:42:42PM -0400, Nathaniel McCallum wrote: > >> Tested on Enarx. This requires a patch[0] for v29 support. > >> > >> Tested-by: Nathaniel McCallum > >> > >> However, we did uncover a small usability issue. See below. > >> > >> [0]: > >> https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662 > > > > ... > > > >> > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. > >> anonymous) can > >> > be used to reserve the address range. Now /dev/sgx supports only > >> opaque > >> > mappings to the (initialized) enclave data. > >> > >> The statement "Any mapping..." isn't actually true. > >> > >> Enarx creates a large enclave (currently 64GiB). This worked when we > >> created a file-backed mapping on /dev/sgx/enclave. However, switching > >> to an anonymous mapping fails with ENOMEM. We suspect this is because > >> the kernel attempts to allocate all the pages and zero them but there > >> is insufficient RAM available. We currently work around this by > >> creating a shared mapping on /dev/zero. > > > > Hmm, the kernel shouldn't actually allocate physical pages unless they're > > written. I'll see if I can reproduce. > > > > For larger size mmap, I think it requires enabling vm overcommit mode 1: > echo 1 | sudo tee /proc/sys/vm/overcommit_memory Which means the default experience isn't good.
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, 06 May 2020 17:14:22 -0500, Sean Christopherson
wrote:
On Wed, May 06, 2020 at 05:42:42PM -0400, Nathaniel McCallum wrote:
Tested on Enarx. This requires a patch[0] for v29 support.
Tested-by: Nathaniel McCallum
However, we did uncover a small usability issue. See below.
[0]:
https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662
...
> * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g.
anonymous) can
> be used to reserve the address range. Now /dev/sgx supports only
opaque
> mappings to the (initialized) enclave data.
The statement "Any mapping..." isn't actually true.
Enarx creates a large enclave (currently 64GiB). This worked when we
created a file-backed mapping on /dev/sgx/enclave. However, switching
to an anonymous mapping fails with ENOMEM. We suspect this is because
the kernel attempts to allocate all the pages and zero them but there
is insufficient RAM available. We currently work around this by
creating a shared mapping on /dev/zero.
Hmm, the kernel shouldn't actually allocate physical pages unless they're
written. I'll see if I can reproduce.
For larger size mmap, I think it requires enabling vm overcommit mode 1:
echo 1 | sudo tee /proc/sys/vm/overcommit_memory
If we want to keep this mmap() strategy, we probably don't want to
advise mmap(ANON) if it allocates all the memory for the enclave ahead
of time, even if it won't be used. This would be wasteful.
OTOH, having to mmap("/dev/zero") seems a bit awkward.
--
Using Opera's mail client: http://www.opera.com/mail/
Re: [PATCH v29 00/20] Intel SGX foundations
Greg, "Dr. Greg" writes: > As an aside, for those who haven't spent the last 5+ years of their > life working with this technology. SGX2 hardware platforms have the > ability to allow unrestricted code execution in enclave context. Unrestricted code execution even before loaded? Unrestricted by priviledge levels? > No amount of LSM or IMA interventions can provide any control over > that. They can restrict what is started and loaded before anything SGX happens. If you want to make real technical arguments then please be specific and precise and spare us the handwaving marketing speak. > In fact, the Confidential Computing Consortium, sponsored by none > other then the Linux Foundation, has at its fundamental tenant, And that's relevant to the technical question in which way? > the notion of developing an eco-system that allows the execution of > code and processing of data, over which, the owner or administrator of > the platform has no visibility or control. It would seem only logical > that adversarial interests would indulge themseleves in those > capabilities as well. > > With respect to SGX and these issues, cryptographic policy management > is important for the same reason that 2-factor authentication is now > considered standard practice in the security industry. > > We appreciate, Jarkko, that you feel that such infrastructure is > optional, like virtualization support, and is something that can go in > after the driver is mainlined. As the diffstat for our patch points > out, the proposed support has virtually no impact on the driver, the > same cannot be said for virtualization capabilities. The diffstat of your patch is irrelevant. What's relevant is the fact that it is completely unreviewed and that it creates yet another user space visible ABI with a noticable lack of documentation. > Moreover, adding support for key based policy management later would > require the addition of another ioctl in order to avoid ABI > compatibility issues. And that's a problem because? > The current initialization ioctl is best suited, from an engineering > perspective, to support this type of infrastructure. What's wrong with having IOCTL_INIT_TYPE_A and IOCTL_INIT_TYPE_B? Nothing at all. It's pretty straight forward and in fact a better solution than a duct taped multiplexing all in one IOCTL_INIT_PONIES approach. > In fact, the necessary support was removed from the ioctl for > political reasons rather then for any valid engineering rationale on > flexible launch control platforms, particularly with our patch or an > equivalent approach. You're surely making a convincing technical argument by claiming that this was a political decision. The amount of non-technical, i.e. political arguments in your mail is definitely larger than the technical content. > Hopefully this is a reasoned technical approach to what has been a > long standing issue surrounding this technology. It's an approach which guarantees that the driver will miss the next merge window. If that's your intention, then please let us know. Merging the current set of patches does not prevent anything you want to be added. It's an extension to the base implementation and not a prerequisite. > Best wishes for a productive week. > > Dr. Greg Thanks a lot for the best wishes. Unfortunately reading this email was not necessarily productive for me, but I surely wish that you can make productive use of my reply. Thanks, tglx > -- > "Opportunity is missed by most people because it is dressed in overalls > and looks like work." > -- Thomas Edison -- "Failure is simply the opportunity to begin again, this time more intelligently" -- Henry Ford
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, May 06, 2020 at 05:42:42PM -0400, Nathaniel McCallum wrote:
> Tested on Enarx. This requires a patch[0] for v29 support.
>
> Tested-by: Nathaniel McCallum
>
> However, we did uncover a small usability issue. See below.
>
> [0]:
> https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662
...
> > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can
> > be used to reserve the address range. Now /dev/sgx supports only opaque
> > mappings to the (initialized) enclave data.
>
> The statement "Any mapping..." isn't actually true.
>
> Enarx creates a large enclave (currently 64GiB). This worked when we
> created a file-backed mapping on /dev/sgx/enclave. However, switching
> to an anonymous mapping fails with ENOMEM. We suspect this is because
> the kernel attempts to allocate all the pages and zero them but there
> is insufficient RAM available. We currently work around this by
> creating a shared mapping on /dev/zero.
Hmm, the kernel shouldn't actually allocate physical pages unless they're
written. I'll see if I can reproduce.
> If we want to keep this mmap() strategy, we probably don't want to
> advise mmap(ANON) if it allocates all the memory for the enclave ahead
> of time, even if it won't be used. This would be wasteful.
>
> OTOH, having to mmap("/dev/zero") seems a bit awkward.
Re: [PATCH v29 00/20] Intel SGX foundations
Tested on Enarx. This requires a patch[0] for v29 support.
Tested-by: Nathaniel McCallum
However, we did uncover a small usability issue. See below.
[0]:
https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662
On Tue, Apr 21, 2020 at 5:53 PM Jarkko Sakkinen
wrote:
>
> Intel(R) 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.
>
> There is a new hardware unit in the processor called Memory Encryption
> Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define
> one or many MEE regions that can hold enclave data by configuring them with
> PRMRR registers.
>
> The MEE automatically encrypts the data leaving the processor package to
> the MEE regions. The data is encrypted using a random key whose life-time
> is exactly one power cycle.
>
> The current implementation requires that the firmware sets
> IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can
> decide what enclaves it wants run. The implementation does not create
> any bottlenecks to support read-only MSRs later on.
>
> You can tell if your CPU supports SGX by looking into /proc/cpuinfo:
>
> cat /proc/cpuinfo | grep sgx
>
> v29:
> * The selftest has been moved to selftests/sgx. Because SGX is an execution
> environment of its own, it really isn't a great fit with more "standard"
> x86 tests.
>
> The RSA key is now generated on fly and the whole signing process has
> been made as part of the enclave loader instead of signing the enclave
> during the compilation time.
>
> Finally, the enclave loader loads now the test enclave directly from its
> ELF file, which means that ELF file does not need to be coverted as raw
> binary during the build process.
> * Version the mm_list instead of using on synchronize_mm() when adding new
> entries. We hold the write lock for the mm_struct, and dup_mm() can thus
> deadlock with the page reclaimer, which could hold the lock for the old
> mm_struct.
> * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can
> be used to reserve the address range. Now /dev/sgx supports only opaque
> mappings to the (initialized) enclave data.
The statement "Any mapping..." isn't actually true.
Enarx creates a large enclave (currently 64GiB). This worked when we
created a file-backed mapping on /dev/sgx/enclave. However, switching
to an anonymous mapping fails with ENOMEM. We suspect this is because
the kernel attempts to allocate all the pages and zero them but there
is insufficient RAM available. We currently work around this by
creating a shared mapping on /dev/zero.
If we want to keep this mmap() strategy, we probably don't want to
advise mmap(ANON) if it allocates all the memory for the enclave ahead
of time, even if it won't be used. This would be wasteful.
OTOH, having to mmap("/dev/zero") seems a bit awkward.
Currently, the selftest uses mmap(ANON) and the patch message above
recommends it.
> * Make the vDSO callable directly from C by preserving RBX and taking leaf
> from RCX.
>
> v28:
> * Documented to Documentation/x86/sgx.rst how the kernel manages the
> enclave ownership.
> * Removed non-LC flow from sgx_einit().
> * Removed struct sgx_einittoken since only the size of the corresponding
> microarchitectural structure is used in the series ATM.
>
> v27:
> * Disallow RIE processes to use enclaves as there could a permission
> conflict between VMA and enclave permissions.
> * In the documentation, replace "grep /proc/cpuinfo" with
> "grep sgx /proc/cpuinfo".
>
> v26:
> * Fixed the commit author in "x86/sgx: Linux Enclave Driver", which was
> changed in v25 by mistake.
> * Addressed a bunch of grammar mistakes in sgx.rst (thanks Randy once
> again for such a detailed feedback).
> * Added back the MAINTAINERS update commit, which was mistakenly removed
> in v25.
> * EREMOVE's for SECS cannot be done while sanitizing an EPC section. The
> CPU does not allow to remove a SECS page before all of its children have
> been removed, and a child page can be in some other section than the one
> currently being processed. Thus, removed special SECS processing from
> sgx_sanitize_page() and instead put sections through it twice. In the
> 2nd round the lists should only contain SECS pages.
>
> v25:
> * Fix a double-free issue when SGX_IOC_ENCLAVE_ADD_PAGES
> fails on executing ENCLS[EADD]. The rollback path executed
> radix_tree_delete() on the same address twice when this happened.
> * Return -EINTR instead of -ERESTARTSYS in SGX_IOC_ENCLAVE_ADD_PAGES when
> a signal is pending.
> * As requested by Borislav, move the CPUID 0x12 features to their own word
> in cpufeatures.
> * Sean fixed a bug from sgx_reclaimer_write() where sgx_encl_put_backing()
> was called with an uninitialized pointer when s
Re: [PATCH v29 00/20] Intel SGX foundations
On 4/21/20 2:52 PM, Jarkko Sakkinen wrote: Intel(R) 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. There is a new hardware unit in the processor called Memory Encryption Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define one or many MEE regions that can hold enclave data by configuring them with PRMRR registers. The MEE automatically encrypts the data leaving the processor package to the MEE regions. The data is encrypted using a random key whose life-time is exactly one power cycle. The current implementation requires that the firmware sets IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can decide what enclaves it wants run. The implementation does not create any bottlenecks to support read-only MSRs later on. You can tell if your CPU supports SGX by looking into /proc/cpuinfo: cat /proc/cpuinfo | grep sgx v29: * The selftest has been moved to selftests/sgx. Because SGX is an execution environment of its own, it really isn't a great fit with more "standard" x86 tests. The RSA key is now generated on fly and the whole signing process has been made as part of the enclave loader instead of signing the enclave during the compilation time. Finally, the enclave loader loads now the test enclave directly from its ELF file, which means that ELF file does not need to be coverted as raw binary during the build process. * Version the mm_list instead of using on synchronize_mm() when adding new entries. We hold the write lock for the mm_struct, and dup_mm() can thus deadlock with the page reclaimer, which could hold the lock for the old mm_struct. * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can be used to reserve the address range. Now /dev/sgx supports only opaque mappings to the (initialized) enclave data. * Make the vDSO callable directly from C by preserving RBX and taking leaf from RCX. Tested with the Open Enclave SDK on top of Intel PSW. Specifically built the Intel PSW with changes to support /dev/sgx mapping[1] new in v29. Tested-by: Jordan Hand [1] https://github.com/intel/linux-sgx/pull/530
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, Apr 30, 2020 at 04:12:07PM +0200, Jethro Beekman wrote: > On 2020-04-30 10:23, Jarkko Sakkinen wrote: > > On Thu, Apr 30, 2020 at 09:19:48AM +0200, Jethro Beekman wrote: > >> On 2020-04-30 05:46, Jarkko Sakkinen wrote: > >>> On Wed, Apr 29, 2020 at 05:27:48PM +0200, Jethro Beekman wrote: > On 2020-04-21 23:52, Jarkko Sakkinen wrote: > > Intel(R) 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. > > > > There is a new hardware unit in the processor called Memory Encryption > > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can > > define > > one or many MEE regions that can hold enclave data by configuring them > > with > > PRMRR registers. > > > > The MEE automatically encrypts the data leaving the processor package to > > the MEE regions. The data is encrypted using a random key whose > > life-time > > is exactly one power cycle. > > > > The current implementation requires that the firmware sets > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > decide what enclaves it wants run. The implementation does not create > > any bottlenecks to support read-only MSRs later on. > > > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > > > cat /proc/cpuinfo | grep sgx > > Let's merge this. > >>> > >>> So can I tag reviewed-by's? > >>> > >> > >> No, but you already have my tested-by's. > >> > >> If it helps I can try to review some patches, but 1) I know nothing > >> about kernel coding guidelines and best practices and 2) I know little > >> about most kernel internals, so I won't be able to review every patch. > > > > Ackd-by *acknowledges* that the patches work for you. I think that would > > be then the correct choice for the driver patch and patches before that. > > > > Lets go with that if that is cool for you of course. > > > > Did you run the selftest only or possibly also some internal Fortanix > > tests? > > > > v29 patches 2 through 18: > > Acked-by: Jethro Beekman > > I only ran production SGX software. I didn't run the self test. That's great to hear thank you. Updated my tree accordingly. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Sat, May 02, 2020 at 05:48:30PM -0700, Andy Lutomirski wrote: Good morning, I hope the week is starting well for everyone. > > On May 2, 2020, at 4:05 PM, Dr. Greg wrote: > > In a nutshell, the driver needs our patch that implements > > cryptographic policy management. > > > > A patch that: > > > > 1.) Does not change the default behavior of the driver. > > > > 2.) Implements capabilities that are consistent with what the hardware > > was designed to do, but only at the discretion of the platform owner. > > > > 3.) Has no impact on the driver architecture. > > > > The only consumer for this driver are SGX runtimes. To our knowledge, > > there exist only two complete runtime implementations, Intel's and > > ours. It us unclear why our approach to the use of the technology > > should be discriminated against when it doesn't impact the other user > > community. > Can you clarify how exactly this patch set discriminates against > your stack? The driver has no provisions for implementing cryptographically based SGX policy management of any type. Our stack is extremely lightweight with no external dependencies and is used in privacy and security sensitive applications, including financial services of certain types. There is a desire in this, and other venues, to use cloud and edge resources with a strong guarantee that the platforms have only had a known set of behaviors. The current DAC based controls in the driver are insufficient to provide those guarantees. I believe I have discussed our use of SGX previously. In a nutshell, we use SGX based modeling engines to supervise kernel based behavioral namespaces, one enclave per namespace. The closest equivalent work that we have seen may be the IPE architecture advanced by Deven Bowers at Microsoft but we address a number of issues that work does not, including non-kernel based behavioral supervision. We support the concern over hardware locked platforms and do not disagree with the driver not supporting those platforms. That being said, there is no technical rationale for not providing cryptographic policy management on FLC based platforms, as I believe our patch demonstrates. Best wishes for a productive week. Dr. Greg As always, Dr. Greg Wettstein, Ph.D, Worker Artisans in autonomously Enjellic Systems Development, LLC self-defensive platforms. 4206 N. 19th Ave. Fargo, ND 58102 PH: 701-281-1686 EMAIL: g...@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
Re: [PATCH v29 00/20] Intel SGX foundations
> On May 2, 2020, at 4:05 PM, Dr. Greg wrote: > > On Thu, Apr 30, 2020 at 06:59:11AM +0300, Jarkko Sakkinen wrote: > > Good afternoon, I hope the weekend is going well for everyone. > >>> On Wed, Apr 29, 2020 at 08:14:59AM -0700, Sean Christopherson wrote: >>> On Wed, Apr 29, 2020 at 08:23:29AM +0300, Jarkko Sakkinen wrote: On Sun, Apr 26, 2020 at 11:57:53AM -0500, Dr. Greg wrote: > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: >> The current implementation requires that the firmware sets >> IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately >> the kernel can decide what enclaves it wants run. The >> implementation does not create any bottlenecks to support >> read-only MSRs later on. > It seems highly unlikely that a driver implementation with any type of > support for read-only launch control registers would ever get into the > kernel. All one needs to do is review the conversations that Matthew > Garrett's lockdown patches engender to get a sense of that, ie: > > https://lwn.net/Articles/818277/ We do not require read-only MSRs. >>> >>> Greg is pointing out the opposite, that supporting read-only MSRs is highly >>> unlikely to ever be supported in the mainline kernel. > >> In a nutshell, what is wrong in the current code changes and what >> *exactly* should we change? This is way too high level at the moment >> at least for my limited brain capacity. > > In a nutshell, the driver needs our patch that implements > cryptographic policy management. > > A patch that: > > 1.) Does not change the default behavior of the driver. > > 2.) Implements capabilities that are consistent with what the hardware > was designed to do, but only at the discretion of the platform owner. > > 3.) Has no impact on the driver architecture. > > The only consumer for this driver are SGX runtimes. To our knowledge, > there exist only two complete runtime implementations, Intel's and > ours. It us unclear why our approach to the use of the technology > should be discriminated against when it doesn't impact the other user > community. Can you clarify how exactly this patch set discriminates against your stack? > > The Linux kernel that I have worked on and supported since 1992 has > always focused on technical rationale and meritocracy rather then > politics. We would be interested in why our proposal for the driver > fails on the former grounds rather then the latter. > >> /Jarkko > > Best wishes for a productive week. > > Dr. Greg > > As always, > Dr. Greg Wettstein, Ph.D, Worker Artisans in autonomously > Enjellic Systems Development, LLC self-defensive IOT platforms > 4206 N. 19th Ave. and edge devices. > Fargo, ND 58102 > PH: 701-281-1686 EMAIL: g...@enjellic.com > -- > "The best way to predict the future is to invent it." >-- Alan Kay
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, Apr 30, 2020 at 06:59:11AM +0300, Jarkko Sakkinen wrote: Good afternoon, I hope the weekend is going well for everyone. > On Wed, Apr 29, 2020 at 08:14:59AM -0700, Sean Christopherson wrote: > > On Wed, Apr 29, 2020 at 08:23:29AM +0300, Jarkko Sakkinen wrote: > > > On Sun, Apr 26, 2020 at 11:57:53AM -0500, Dr. Greg wrote: > > > > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > > > > > > > > The current implementation requires that the firmware sets > > > > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately > > > > > the kernel can decide what enclaves it wants run. The > > > > > implementation does not create any bottlenecks to support > > > > > read-only MSRs later on. > > > > > > > It seems highly unlikely that a driver implementation with any type of > > > > support for read-only launch control registers would ever get into the > > > > kernel. All one needs to do is review the conversations that Matthew > > > > Garrett's lockdown patches engender to get a sense of that, ie: > > > > > > > > https://lwn.net/Articles/818277/ > > > > > > We do not require read-only MSRs. > > > > Greg is pointing out the opposite, that supporting read-only MSRs is highly > > unlikely to ever be supported in the mainline kernel. > In a nutshell, what is wrong in the current code changes and what > *exactly* should we change? This is way too high level at the moment > at least for my limited brain capacity. In a nutshell, the driver needs our patch that implements cryptographic policy management. A patch that: 1.) Does not change the default behavior of the driver. 2.) Implements capabilities that are consistent with what the hardware was designed to do, but only at the discretion of the platform owner. 3.) Has no impact on the driver architecture. The only consumer for this driver are SGX runtimes. To our knowledge, there exist only two complete runtime implementations, Intel's and ours. It us unclear why our approach to the use of the technology should be discriminated against when it doesn't impact the other user community. The Linux kernel that I have worked on and supported since 1992 has always focused on technical rationale and meritocracy rather then politics. We would be interested in why our proposal for the driver fails on the former grounds rather then the latter. > /Jarkko Best wishes for a productive week. Dr. Greg As always, Dr. Greg Wettstein, Ph.D, Worker Artisans in autonomously Enjellic Systems Development, LLC self-defensive IOT platforms 4206 N. 19th Ave. and edge devices. Fargo, ND 58102 PH: 701-281-1686 EMAIL: g...@enjellic.com -- "The best way to predict the future is to invent it." -- Alan Kay
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-04-30 10:23, Jarkko Sakkinen wrote: > On Thu, Apr 30, 2020 at 09:19:48AM +0200, Jethro Beekman wrote: >> On 2020-04-30 05:46, Jarkko Sakkinen wrote: >>> On Wed, Apr 29, 2020 at 05:27:48PM +0200, Jethro Beekman wrote: On 2020-04-21 23:52, Jarkko Sakkinen wrote: > Intel(R) 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. > > There is a new hardware unit in the processor called Memory Encryption > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > one or many MEE regions that can hold enclave data by configuring them > with > PRMRR registers. > > The MEE automatically encrypts the data leaving the processor package to > the MEE regions. The data is encrypted using a random key whose life-time > is exactly one power cycle. > > The current implementation requires that the firmware sets > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > decide what enclaves it wants run. The implementation does not create > any bottlenecks to support read-only MSRs later on. > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > cat /proc/cpuinfo | grep sgx Let's merge this. >>> >>> So can I tag reviewed-by's? >>> >> >> No, but you already have my tested-by's. >> >> If it helps I can try to review some patches, but 1) I know nothing >> about kernel coding guidelines and best practices and 2) I know little >> about most kernel internals, so I won't be able to review every patch. > > Ackd-by *acknowledges* that the patches work for you. I think that would > be then the correct choice for the driver patch and patches before that. > > Lets go with that if that is cool for you of course. > > Did you run the selftest only or possibly also some internal Fortanix > tests? > v29 patches 2 through 18: Acked-by: Jethro Beekman I only ran production SGX software. I didn't run the self test. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, Apr 30, 2020 at 09:19:48AM +0200, Jethro Beekman wrote: > On 2020-04-30 05:46, Jarkko Sakkinen wrote: > > On Wed, Apr 29, 2020 at 05:27:48PM +0200, Jethro Beekman wrote: > >> On 2020-04-21 23:52, Jarkko Sakkinen wrote: > >>> Intel(R) 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. > >>> > >>> There is a new hardware unit in the processor called Memory Encryption > >>> Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > >>> one or many MEE regions that can hold enclave data by configuring them > >>> with > >>> PRMRR registers. > >>> > >>> The MEE automatically encrypts the data leaving the processor package to > >>> the MEE regions. The data is encrypted using a random key whose life-time > >>> is exactly one power cycle. > >>> > >>> The current implementation requires that the firmware sets > >>> IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > >>> decide what enclaves it wants run. The implementation does not create > >>> any bottlenecks to support read-only MSRs later on. > >>> > >>> You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > >>> > >>> cat /proc/cpuinfo | grep sgx > >> > >> Let's merge this. > > > > So can I tag reviewed-by's? > > > > No, but you already have my tested-by's. > > If it helps I can try to review some patches, but 1) I know nothing > about kernel coding guidelines and best practices and 2) I know little > about most kernel internals, so I won't be able to review every patch. Ackd-by *acknowledges* that the patches work for you. I think that would be then the correct choice for the driver patch and patches before that. Lets go with that if that is cool for you of course. Did you run the selftest only or possibly also some internal Fortanix tests? /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-04-30 05:46, Jarkko Sakkinen wrote: > On Wed, Apr 29, 2020 at 05:27:48PM +0200, Jethro Beekman wrote: >> On 2020-04-21 23:52, Jarkko Sakkinen wrote: >>> Intel(R) 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. >>> >>> There is a new hardware unit in the processor called Memory Encryption >>> Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define >>> one or many MEE regions that can hold enclave data by configuring them with >>> PRMRR registers. >>> >>> The MEE automatically encrypts the data leaving the processor package to >>> the MEE regions. The data is encrypted using a random key whose life-time >>> is exactly one power cycle. >>> >>> The current implementation requires that the firmware sets >>> IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can >>> decide what enclaves it wants run. The implementation does not create >>> any bottlenecks to support read-only MSRs later on. >>> >>> You can tell if your CPU supports SGX by looking into /proc/cpuinfo: >>> >>> cat /proc/cpuinfo | grep sgx >> >> Let's merge this. > > So can I tag reviewed-by's? > No, but you already have my tested-by's. If it helps I can try to review some patches, but 1) I know nothing about kernel coding guidelines and best practices and 2) I know little about most kernel internals, so I won't be able to review every patch. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, Apr 29, 2020 at 08:14:59AM -0700, Sean Christopherson wrote: > On Wed, Apr 29, 2020 at 08:23:29AM +0300, Jarkko Sakkinen wrote: > > On Sun, Apr 26, 2020 at 11:57:53AM -0500, Dr. Greg wrote: > > > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > > > > > > Good day, I hope the weekend is going well for everyone. > > > > > > > Intel(R) 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. > > > > > > > > ... [ elided ] .. > > > > > > > > The current implementation requires that the firmware sets > > > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > > > decide what enclaves it wants run. The implementation does not create > > > > any bottlenecks to support read-only MSRs later on. > > > > > > It seems highly unlikely that a driver implementation with any type of > > > support for read-only launch control registers would ever get into the > > > kernel. All one needs to do is review the conversations that Matthew > > > Garrett's lockdown patches engender to get a sense of that, ie: > > > > > > https://lwn.net/Articles/818277/ > > > > We do not require read-only MSRs. > > Greg is pointing out the opposite, that supporting read-only MSRs is highly > unlikely to ever be supported in the mainline kernel. In a nutshell, what is wrong in the current code changes and what *exactly* should we change? This is way too high level at the moment at least for my limited brain capacity. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, Apr 29, 2020 at 05:27:48PM +0200, Jethro Beekman wrote: > On 2020-04-21 23:52, Jarkko Sakkinen wrote: > > Intel(R) 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. > > > > There is a new hardware unit in the processor called Memory Encryption > > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > > one or many MEE regions that can hold enclave data by configuring them with > > PRMRR registers. > > > > The MEE automatically encrypts the data leaving the processor package to > > the MEE regions. The data is encrypted using a random key whose life-time > > is exactly one power cycle. > > > > The current implementation requires that the firmware sets > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > decide what enclaves it wants run. The implementation does not create > > any bottlenecks to support read-only MSRs later on. > > > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > > > cat /proc/cpuinfo | grep sgx > > Let's merge this. So can I tag reviewed-by's? /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-04-21 23:52, Jarkko Sakkinen wrote: > Intel(R) 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. > > There is a new hardware unit in the processor called Memory Encryption > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > one or many MEE regions that can hold enclave data by configuring them with > PRMRR registers. > > The MEE automatically encrypts the data leaving the processor package to > the MEE regions. The data is encrypted using a random key whose life-time > is exactly one power cycle. > > The current implementation requires that the firmware sets > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > decide what enclaves it wants run. The implementation does not create > any bottlenecks to support read-only MSRs later on. > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > cat /proc/cpuinfo | grep sgx Let's merge this. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v29 00/20] Intel SGX foundations
On Sun, Apr 26, 2020 at 11:57:53AM -0500, Dr. Greg wrote: > In closing, it is important to note that the proposed SGX driver is > not available as a module. This effectively excludes any alternative > implementations of the driver without replacement of the kernel at > large. No it doesn't. The SGX subsytem won't allocate EPC pages unless userspace creates an enclave, i.e. preventing unprivileged userspace from accessing /dev/sgx/enclave will allow loading an alternative out-of-tree SGX module. Yes, SGX sanitizes the EPC on boot, but that's arguably a good thing for out-of-tree modules. And if you want to get crafty and squash in-kernel SGX altogether, boot with "clearcpuid=" and/or "clearcpuid=" to disable in-kernel support entirely. SGX won't be correctly enumerated in /proc/cpuinfo relative to the existence of an out-of-tree module, but that seems like a very minor issue if you're running with a completely different SGX driver. > It also means that any platform, with SGX hardware support, > running a kernel with this driver, has the potential for the > security/privacy issues noted above. Unless I'm mistaken, /dev/sgx is root-only by default. There are far scarier mechanisms available to root for hosing the system. > If key based policy management is not allowed, then the driver needs > to be re-architected to have modular support so that alternative > implementations or the absence of any driver support are at least > tenable. As above, using an alternative implementation is teneble, albeit a bit kludgy.
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, Apr 29, 2020 at 08:23:29AM +0300, Jarkko Sakkinen wrote: > On Sun, Apr 26, 2020 at 11:57:53AM -0500, Dr. Greg wrote: > > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > > > > Good day, I hope the weekend is going well for everyone. > > > > > Intel(R) 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. > > > > > > ... [ elided ] .. > > > > > > The current implementation requires that the firmware sets > > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > > decide what enclaves it wants run. The implementation does not create > > > any bottlenecks to support read-only MSRs later on. > > > > It seems highly unlikely that a driver implementation with any type of > > support for read-only launch control registers would ever get into the > > kernel. All one needs to do is review the conversations that Matthew > > Garrett's lockdown patches engender to get a sense of that, ie: > > > > https://lwn.net/Articles/818277/ > > We do not require read-only MSRs. Greg is pointing out the opposite, that supporting read-only MSRs is highly unlikely to ever be supported in the mainline kernel.
Re: [PATCH v29 00/20] Intel SGX foundations
On Sun, Apr 26, 2020 at 11:57:53AM -0500, Dr. Greg wrote: > On Wed, Apr 22, 2020 at 12:52:56AM +0300, Jarkko Sakkinen wrote: > > Good day, I hope the weekend is going well for everyone. > > > Intel(R) 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. > > > > ... [ elided ] .. > > > > The current implementation requires that the firmware sets > > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > > decide what enclaves it wants run. The implementation does not create > > any bottlenecks to support read-only MSRs later on. > > It seems highly unlikely that a driver implementation with any type of > support for read-only launch control registers would ever get into the > kernel. All one needs to do is review the conversations that Matthew > Garrett's lockdown patches engender to get a sense of that, ie: > > https://lwn.net/Articles/818277/ We do not require read-only MSRs. /Jarkko
Re: [PATCH v29 00/20] Intel SGX foundations
On Wed, Apr 22, 2020 at 09:48:58AM -0700, Connor Kuehl wrote: > On 4/21/20 2:52 PM, Jarkko Sakkinen wrote: > > v29: > > * The selftest has been moved to selftests/sgx. Because SGX is an execution > >environment of its own, it really isn't a great fit with more "standard" > >x86 tests. > > > >The RSA key is now generated on fly and the whole signing process has > >been made as part of the enclave loader instead of signing the enclave > >during the compilation time. > > > >Finally, the enclave loader loads now the test enclave directly from its > >ELF file, which means that ELF file does not need to be coverted as raw > >binary during the build process. > > * Version the mm_list instead of using on synchronize_mm() when adding new > >entries. We hold the write lock for the mm_struct, and dup_mm() can thus > >deadlock with the page reclaimer, which could hold the lock for the old > >mm_struct. > > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can > >be used to reserve the address range. Now /dev/sgx supports only opaque > >mappings to the (initialized) enclave data. > > * Make the vDSO callable directly from C by preserving RBX and taking leaf > >from RCX. > > Hi all, > > I've been producing Fedora 32 kernel builds with the SGX patches applied for > a few of weeks and I've just produced a build with this latest revision[1]. > We've been using these kernels to enable SGX for some of our > development/test machines. Thanks a lot! /Jarkko
