| Message ID | cover.1649878359.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86/sgx and selftests/sgx: Support SGX2 | expand |
On Wed, 2022-04-13 at 14:10 -0700, Reinette Chatre wrote: > Now that the discussions surrounding the support for SGX2 is settling, > the kselftest audience is added to the discussion for the first time > to consider the testing of the new features. > > V3: https://lore.kernel.org/lkml/cover.1648847675.git.reinette.chatre@intel.com/ > > Changes since V3 that directly impact user space: > - SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS ioctl()'s struct > sgx_enclave_restrict_permissions no longer provides entire secinfo, > just the new permissions in new "permissions" struct member. (Jarkko) > - Rename SGX_IOC_ENCLAVE_MODIFY_TYPE ioctl() to > SGX_IOC_ENCLAVE_MODIFY_TYPES. (Jarkko) > - SGX_IOC_ENCLAVE_MODIFY_TYPES ioctl()'s struct sgx_enclave_modify_type > no longer provides entire secinfo, just the new page type in new > "page_type" struct member. (Jarkko) > > Details about changes since V3 that do not directly impact user space: > - Add new patch to enable VA pages to be added without invoking reclaimer > directly if no EPC pages are available, failing instead. This enables > VA pages to be added with enclave's mutex held. Fixes an issue > encountered by Haitao. More details in new patch "x86/sgx: Support VA page > allocation without reclaiming". > - While refactoring, change existing code to consistently use > IS_ALIGNED(). (Jarkko) > - Many patches received a tag from Jarkko. > - Many smaller changes, please refer to individual patches. > > V2: https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/ > > Changes since V2 that directly impact user space: > - Maximum allowed permissions of dynamically added pages is RWX, > previously limited to RW. (Jarkko) > Dynamically added pages are initially created with architecturally > limited EPCM permissions of RW. mmap() and mprotect() of these pages > with RWX permissions would no longer be blocked by SGX driver. PROT_EXEC > on dynamically added pages will be possible after running ENCLU[EMODPE] > from within the enclave with appropriate VMA permissions. > > - The kernel no longer attempts to track the EPCM runtime permissions. (Jarkko) > Consequences are: > - Kernel does not modify PTEs to follow EPCM permissions. User space > will receive #PF with SGX error code in cases where the V2 > implementation would have resulted in regular (non-SGX) page fault > error code. > - SGX_IOC_ENCLAVE_RELAX_PERMISSIONS is removed. This ioctl() was used > to clear PTEs after permissions were modified from within the enclave > and ensure correct PTEs are installed. Since PTEs no longer track > EPCM permissions the changes in EPCM permissions would not impact PTEs. > As long as new permissions are within the maximum vetted permissions > (vm_max_prot_bits) only ENCLU[EMODPE] from within enclave is needed, > as accompanied by appropriate VMA permissions. > > - struct sgx_enclave_restrict_perm renamed to > sgx_enclave_restrict_permissions (Jarkko) > > - struct sgx_enclave_modt renamed to struct sgx_enclave_modify_type > to be consistent with the verbose naming of other SGX uapi structs. > > Details about changes since V2 that do not directly impact user space: > - Kernel no longer tracks the runtime EPCM permissions with the aim of > installing accurate PTEs. (Jarkko) > - In support of this change the following patches were removed: > Documentation/x86: Document SGX permission details > x86/sgx: Support VMA permissions more relaxed than enclave permissions > x86/sgx: Add pfn_mkwrite() handler for present PTEs > x86/sgx: Add sgx_encl_page->vm_run_prot_bits for dynamic permission changes > x86/sgx: Support relaxing of enclave page permissions > - No more handling of scenarios where VMA permissions may be more > relaxed than what the EPCM allows. Enclaves are not prevented > from accessing such pages and the EPCM permissions are entrusted > to control access as supported by the SGX error code in page faults. > - No more explicit setting of protection bits in page fault handler. > Protection bits are inherited from VMA similar to SGX1 support. > > - Selftest patches are moved to the end of the series. (Jarkko) > > - New patch contributed by Jarkko to avoid duplicated code: > x86/sgx: Export sgx_encl_page_alloc() > > - New patch separating changes from existing patch. (Jarkko) > x86/sgx: Export sgx_encl_{grow,shrink}() > > - New patch to keep one required benefit from the (now removed) kernel > EPCM permission tracking: > x86/sgx: Support loading enclave page without VMA permissions check > > - Updated cover letter to reflect architecture changes. > > - Many smaller changes, please refer to individual patches. > > V1: https://lore.kernel.org/linux-sgx/cover.1638381245.git.reinette.chatre@intel.com/ > > Changes since V1 that directly impact user space: > - SGX2 permission changes changed from a single ioctl() named > SGX_IOC_PAGE_MODP to two new ioctl()s: > SGX_IOC_ENCLAVE_RELAX_PERMISSIONS and > SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS, supported by two different > parameter structures (SGX_IOC_ENCLAVE_RELAX_PERMISSIONS does > not support a result output parameter) (Jarkko). > > User space flow impact: After user space runs ENCLU[EMODPE] it > needs to call SGX_IOC_ENCLAVE_RELAX_PERMISSIONS to have PTEs > updated. Previously running SGX_IOC_PAGE_MODP in this scenario > resulted in EPCM.PR being set but calling > SGX_IOC_ENCLAVE_RELAX_PERMISSIONS will not result in EPCM.PR > being set anymore and thus no need for an additional > ENCLU[EACCEPT]. > > - SGX_IOC_ENCLAVE_RELAX_PERMISSIONS and > SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS > obtain new permissions from secinfo as parameter instead of > the permissions directly (Jarkko). > > - ioctl() supporting SGX2 page type change is renamed from > SGX_IOC_PAGE_MODT to SGX_IOC_ENCLAVE_MODIFY_TYPE (Jarkko). > > - SGX_IOC_ENCLAVE_MODIFY_TYPE obtains new page type from secinfo > as parameter instead of the page type directly (Jarkko). > > - ioctl() supporting SGX2 page removal is renamed from > SGX_IOC_PAGE_REMOVE to SGX_IOC_ENCLAVE_REMOVE_PAGES (Jarkko). > > - All ioctl() parameter structures have been renamed as a result of the > ioctl() renaming: > SGX_IOC_ENCLAVE_RELAX_PERMISSIONS => struct sgx_enclave_relax_perm > SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS => struct sgx_enclave_restrict_perm > SGX_IOC_ENCLAVE_MODIFY_TYPE => struct sgx_enclave_modt > SGX_IOC_ENCLAVE_REMOVE_PAGES => struct sgx_enclave_remove_pages > > Changes since V1 that do not directly impact user space: > - Number of patches in series increased from 25 to 32 primarily because > of splitting the original submission: > - Wrappers for the new SGX2 functions are introduced in three separate > patches replacing the original "x86/sgx: Add wrappers for SGX2 > functions" > (Jarkko). > - Moving and renaming sgx_encl_ewb_cpumask() is done with two patches > replacing the original "x86/sgx: Use more generic name for enclave > cpumask function" (Jarkko). > - Support for SGX2 EPCM permission changes is split into two ioctls(), > one for relaxing and one for restricting permissions, each introduced > by a new patch replacing the original "x86/sgx: Support enclave page > permission changes" (Jarkko). > - Extracted code used by existing ioctls() for usage by new ioctl()s > into a new utility in new patch "x86/sgx: Create utility to validate > user provided offset and length" (Dave did not specifically ask for > this but it addresses his review feedback). > - Two new Documentation patches to support the SGX2 work > ("Documentation/x86: Introduce enclave runtime management") and > a dedicated section on the enclave permission management > ("Documentation/x86: Document SGX permission details") (Andy). > - Most patches were reworked to improve the language by: > * aiming to refer to exact item instead of English rephrasing (Jarkko). > * use ioctl() instead of ioctl throughout (Dave). > * Use "relaxed" instead of "exceed" when referring to permissions > (Dave). > - Improved documentation with several additions to > Documentation/x86/sgx.rst. > - Many smaller changes, please refer to individual patches. > > Hi Everybody, > > The current Linux kernel support for SGX includes support for SGX1 that > requires that an enclave be created with properties that accommodate all > usages over its (the enclave's) lifetime. This includes properties such > as permissions of enclave pages, the number of enclave pages, and the > number of threads supported by the enclave. > > Consequences of this requirement to have the enclave be created to > accommodate all usages include: > * pages needing to support relocated code are required to have RWX > permissions for their entire lifetime, > * an enclave needs to be created with the maximum stack and heap > projected to be needed during the enclave's entire lifetime which > can be longer than the processes running within it, > * an enclave needs to be created with support for the maximum number > of threads projected to run in the enclave. > > Since SGX1 a few more functions were introduced, collectively called > SGX2, that support modifications to an initialized enclave. Hardware > supporting these functions are already available as listed on > https://github.com/ayeks/SGX-hardware > > This series adds support for SGX2, also referred to as Enclave Dynamic > Memory Management (EDMM). This includes: > > * Support modifying EPCM permissions of regular enclave pages belonging > to an initialized enclave. Only permission restriction is supported > via a new ioctl() SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS. Relaxing of > EPCM permissions can only be done from within the enclave with the > SGX instruction ENCLU[EMODPE]. > > * Support dynamic addition of regular enclave pages to an initialized > enclave. At creation new pages are architecturally limited to RW EPCM > permissions but will be accessible with PROT_EXEC after the enclave > runs ENCLU[EMODPE] to relax EPCM permissions to RWX. > Pages are dynamically added to an initialized enclave from the SGX > page fault handler. > > * Support expanding an initialized enclave to accommodate more threads. > More threads can be accommodated by an enclave with the addition of > Thread Control Structure (TCS) pages that is done by changing the > type of regular enclave pages to TCS pages using a new ioctl() > SGX_IOC_ENCLAVE_MODIFY_TYPES. > > * Support removing regular and TCS pages from an initialized enclave. > Removing pages is accomplished in two stages as supported by two new > ioctl()s SGX_IOC_ENCLAVE_MODIFY_TYPES (same ioctl() as mentioned in > previous bullet) and SGX_IOC_ENCLAVE_REMOVE_PAGES. > > * Tests covering all the new flows, some edge cases, and one > comprehensive stress scenario. > > No additional work is needed to support SGX2 in a virtualized > environment. All tests included in this series passed when run from > a guest as tested with the recent QEMU release based on 6.2.0 > that supports SGX. > > Patches 1 through 14 prepare the existing code for SGX2 support by > introducing the SGX2 functions, refactoring code, and tracking enclave > page types. > > Patches 15 through 21 enable the SGX2 features and include a > Documentation patch. > > Patches 22 through 31 test several scenarios of all the enabled > SGX2 features. > > This series is based on v5.18-rc2. > > Your feedback will be greatly appreciated. > > Regards, > > Reinette > > Jarkko Sakkinen (1): > x86/sgx: Export sgx_encl_page_alloc() > > Reinette Chatre (30): > x86/sgx: Add short descriptions to ENCLS wrappers > x86/sgx: Add wrapper for SGX2 EMODPR function > x86/sgx: Add wrapper for SGX2 EMODT function > x86/sgx: Add wrapper for SGX2 EAUG function > x86/sgx: Support loading enclave page without VMA permissions check > x86/sgx: Export sgx_encl_ewb_cpumask() > x86/sgx: Rename sgx_encl_ewb_cpumask() as sgx_encl_cpumask() > x86/sgx: Move PTE zap code to new sgx_zap_enclave_ptes() > x86/sgx: Make sgx_ipi_cb() available internally > x86/sgx: Create utility to validate user provided offset and length > x86/sgx: Keep record of SGX page type > x86/sgx: Export sgx_encl_{grow,shrink}() > x86/sgx: Support VA page allocation without reclaiming > x86/sgx: Support restricting of enclave page permissions > x86/sgx: Support adding of pages to an initialized enclave > x86/sgx: Tighten accessible memory range after enclave initialization > x86/sgx: Support modifying SGX page type > x86/sgx: Support complete page removal > x86/sgx: Free up EPC pages directly to support large page ranges > Documentation/x86: Introduce enclave runtime management section > selftests/sgx: Add test for EPCM permission changes > selftests/sgx: Add test for TCS page permission changes > selftests/sgx: Test two different SGX2 EAUG flows > selftests/sgx: Introduce dynamic entry point > selftests/sgx: Introduce TCS initialization enclave operation > selftests/sgx: Test complete changing of page type flow > selftests/sgx: Test faulty enclave behavior > selftests/sgx: Test invalid access to removed enclave page > selftests/sgx: Test reclaiming of untouched page > selftests/sgx: Page removal stress test > > Documentation/x86/sgx.rst | 15 + > arch/x86/include/asm/sgx.h | 8 + > arch/x86/include/uapi/asm/sgx.h | 61 + > arch/x86/kernel/cpu/sgx/encl.c | 329 +++- > arch/x86/kernel/cpu/sgx/encl.h | 15 +- > arch/x86/kernel/cpu/sgx/encls.h | 33 + > arch/x86/kernel/cpu/sgx/ioctl.c | 640 +++++++- > arch/x86/kernel/cpu/sgx/main.c | 75 +- > arch/x86/kernel/cpu/sgx/sgx.h | 3 + > tools/testing/selftests/sgx/defines.h | 23 + > tools/testing/selftests/sgx/load.c | 41 + > tools/testing/selftests/sgx/main.c | 1435 +++++++++++++++++ > tools/testing/selftests/sgx/main.h | 1 + > tools/testing/selftests/sgx/test_encl.c | 68 + > .../selftests/sgx/test_encl_bootstrap.S | 6 + > 15 files changed, 2625 insertions(+), 128 deletions(-) > > > base-commit: ce522ba9ef7e2d9fb22a39eb3371c0c64e2a433e IMHO, we can pull this after +1 version. I think I had only one nit (one character to a struct name it was), and I've been testing this series *extensively* with real-world code (wasm run-time that we are developing), so I'm confident that it is *good enough*. Reinette, for the EMODT patch, as long as you fix the struct name you can add my reviewed-by and also tested-by to that patch before you send it! It's so narrow change. BR, Jarkko
Hi Jarkko, On 4/14/2022 4:25 AM, Jarkko Sakkinen wrote: > On Wed, 2022-04-13 at 14:10 -0700, Reinette Chatre wrote: > IMHO, we can pull this after +1 version. I think I had only one nit > (one character to a struct name it was), and I've been testing this > series *extensively* with real-world code (wasm run-time that we are > developing), so I'm confident that it is *good enough*. Thank you very much. I am aware of other teams successfully building on and testing this work. I do hope that they could also provide an ack to help increase the confidence in this work. > > Reinette, for the EMODT patch, as long as you fix the struct name > you can add my reviewed-by and also tested-by to that patch before > you send it! It's so narrow change. Thank you. I will make the struct name change and also plan to make the same change to the function names in that patch to ensure that everything is consistent in that regard. Reinette
On Thu, 2022-04-14 at 09:34 -0700, Reinette Chatre wrote: > Hi Jarkko, > > On 4/14/2022 4:25 AM, Jarkko Sakkinen wrote: > > On Wed, 2022-04-13 at 14:10 -0700, Reinette Chatre wrote: > > IMHO, we can pull this after +1 version. I think I had only one nit > > (one character to a struct name it was), and I've been testing this > > series *extensively* with real-world code (wasm run-time that we are > > developing), so I'm confident that it is *good enough*. > > Thank you very much. I am aware of other teams successfully building > on and testing this work. I do hope that they could also provide an > ack to help increase the confidence in this work. > > > > > Reinette, for the EMODT patch, as long as you fix the struct name > > you can add my reviewed-by and also tested-by to that patch before > > you send it! It's so narrow change. > > Thank you. I will make the struct name change and also plan to > make the same change to the function names in that patch to ensure that > everything is consistent in that regard. I think getting ack from anyone working Graphene-SGX would bring a great coverage of different use cases. It's different same of Enarx in the sense that both can run arbitrary applicatons written e.g. with C++ although approaches are on opposite sides. > Reinette BR; Jarkko
Hi Jarkko, I am working on enabling Gramine with this EDMM patch series. I had tested with V2 patch series and it looked fine. Will evaluate Gramine with V4 patch series and post my updates in a couple of days. Regards, -Vijay > -----Original Message----- > From: Jarkko Sakkinen <jarkko@kernel.org> > Sent: Thursday, April 14, 2022 9:56 AM > To: Chatre, Reinette <reinette.chatre@intel.com>; > dave.hansen@linux.intel.com; tglx@linutronix.de; bp@alien8.de; Lutomirski, > Andy <luto@kernel.org>; mingo@redhat.com; linux-sgx@vger.kernel.org; > x86@kernel.org; shuah@kernel.org; linux-kselftest@vger.kernel.org > Cc: Christopherson,, Sean <seanjc@google.com>; Huang, Kai > <kai.huang@intel.com>; Zhang, Cathy <cathy.zhang@intel.com>; Xing, > Cedric <cedric.xing@intel.com>; Huang, Haitao <haitao.huang@intel.com>; > Shanahan, Mark <mark.shanahan@intel.com>; Dhanraj, Vijay > <vijay.dhanraj@intel.com>; hpa@zytor.com; linux-kernel@vger.kernel.org > Subject: Re: [PATCH V4 00/31] x86/sgx and selftests/sgx: Support SGX2 > > On Thu, 2022-04-14 at 09:34 -0700, Reinette Chatre wrote: > > Hi Jarkko, > > > > On 4/14/2022 4:25 AM, Jarkko Sakkinen wrote: > > > On Wed, 2022-04-13 at 14:10 -0700, Reinette Chatre wrote: > > > IMHO, we can pull this after +1 version. I think I had only one nit > > > (one character to a struct name it was), and I've been testing this > > > series *extensively* with real-world code (wasm run-time that we are > > > developing), so I'm confident that it is *good enough*. > > > > Thank you very much. I am aware of other teams successfully building > > on and testing this work. I do hope that they could also provide an > > ack to help increase the confidence in this work. > > > > > > > > Reinette, for the EMODT patch, as long as you fix the struct name > > > you can add my reviewed-by and also tested-by to that patch before > > > you send it! It's so narrow change. > > > > Thank you. I will make the struct name change and also plan to make > > the same change to the function names in that patch to ensure that > > everything is consistent in that regard. > > I think getting ack from anyone working Graphene-SGX would bring a great > coverage of different use cases. It's different same of Enarx in the sense that > both can run arbitrary applicatons written e.g. with C++ although approaches > are on opposite sides. > > > Reinette > > BR; Jarkko
On Thu, 2022-04-14 at 18:35 +0000, Dhanraj, Vijay wrote: > Hi Jarkko, > > I am working on enabling Gramine with this EDMM patch series. I had tested with V2 patch series and it looked fine. Will evaluate Gramine with V4 patch series and post my updates in a couple of > days. OK, good to hear. Looking forward to it. BR, Jarkko
Hi All, I evaluated V4 patch changes with Gramine and ran into an issue when trying to set EPC page permission to PROT_NONE. It looks like with V3 patch series a change was introduced which requires kernel to have at least R permission when calling RESTRICT IOCTL. This change was done under the assumption that EPCM requires at least R permission for EMODPE/EACCEPT to succeed. But when testing with V2 version, EACCEPT worked fine with page permission set to PROT_NONE. Thanks to @Shanahan, Mark for confirming that EPCM does not need to have R value to allow EACCEPT or EMODPE. Given this, can we please revert this change? Thanks, -Vijay > -----Original Message----- > From: Jarkko Sakkinen <jarkko@kernel.org> > Sent: Sunday, April 17, 2022 7:58 AM > To: Dhanraj, Vijay <vijay.dhanraj@intel.com>; Chatre, Reinette > <reinette.chatre@intel.com>; dave.hansen@linux.intel.com; > tglx@linutronix.de; bp@alien8.de; Lutomirski, Andy <luto@kernel.org>; > mingo@redhat.com; linux-sgx@vger.kernel.org; x86@kernel.org; > shuah@kernel.org; linux-kselftest@vger.kernel.org > Cc: Christopherson,, Sean <seanjc@google.com>; Huang, Kai > <kai.huang@intel.com>; Zhang, Cathy <cathy.zhang@intel.com>; Xing, > Cedric <cedric.xing@intel.com>; Huang, Haitao <haitao.huang@intel.com>; > Shanahan, Mark <mark.shanahan@intel.com>; hpa@zytor.com; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH V4 00/31] x86/sgx and selftests/sgx: Support SGX2 > > On Thu, 2022-04-14 at 18:35 +0000, Dhanraj, Vijay wrote: > > Hi Jarkko, > > > > I am working on enabling Gramine with this EDMM patch series. I had > > tested with V2 patch series and it looked fine. Will evaluate Gramine with > V4 patch series and post my updates in a couple of days. > > OK, good to hear. Looking forward to it. > > BR, Jarkko
Hi Vijay and Mark, On 4/21/2022 4:46 PM, Dhanraj, Vijay wrote: > Hi All, > > I evaluated V4 patch changes with Gramine and ran into an issue when trying to set EPC page permission to PROT_NONE. It looks like with V3 patch series a change was introduced which requires kernel to have at least R permission when calling RESTRICT IOCTL. This change was done under the assumption that EPCM requires at least R permission for EMODPE/EACCEPT to succeed. But when testing with V2 version, EACCEPT worked fine with page permission set to PROT_NONE. > > Thanks to @Shanahan, Mark for confirming that EPCM does not need to have R value to allow EACCEPT or EMODPE. Given this, can we please revert this change? > Thank you very much for pointing this out. I can revert the change to what was done in V2 where the only check is to ensure that W requires R. This is a requirement of EMODPR. Could you please check if this snippet results in things working for you again? ---8<--- diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 83674d054c13..7c7c8a61196e 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -855,12 +855,8 @@ static long sgx_ioc_enclave_restrict_permissions(struct sgx_encl *encl, if (params.permissions & ~SGX_SECINFO_PERMISSION_MASK) return -EINVAL; - /* - * Read access is required for the enclave to be able to use the page. - * SGX instructions like ENCLU[EMODPE] and ENCLU[EACCEPT] require - * read access. - */ - if (!(params.permissions & SGX_SECINFO_R)) + if ((params.permissions & SGX_SECINFO_W) && + !(params.permissions & SGX_SECINFO_R)) return -EINVAL; if (params.result || params.count)
On Thu, Apr 21, 2022 at 11:46:57PM +0000, Dhanraj, Vijay wrote: > Hi All, > > I evaluated V4 patch changes with Gramine and ran into an issue when > trying to set EPC page permission to PROT_NONE. It looks like with V3 > patch series a change was introduced which requires kernel to have at > least R permission when calling RESTRICT IOCTL. This change was done > under the assumption that EPCM requires at least R permission for > EMODPE/EACCEPT to succeed. But when testing with V2 version, EACCEPT > worked fine with page permission set to PROT_NONE. > > Thanks to @Shanahan, Mark for confirming that EPCM does not need to have > R value to allow EACCEPT or EMODPE. Given this, can we please revert this > change? > > Thanks, > -Vijay Let's try to avoid top-posting and split the lines appropriately. BR, Jarkko
On Thu, Apr 21, 2022 at 08:29:31PM -0700, Reinette Chatre wrote: > Hi Vijay and Mark, > > On 4/21/2022 4:46 PM, Dhanraj, Vijay wrote: > > Hi All, > > > > I evaluated V4 patch changes with Gramine and ran into an issue when trying to set EPC page permission to PROT_NONE. It looks like with V3 patch series a change was introduced which requires kernel to have at least R permission when calling RESTRICT IOCTL. This change was done under the assumption that EPCM requires at least R permission for EMODPE/EACCEPT to succeed. But when testing with V2 version, EACCEPT worked fine with page permission set to PROT_NONE. > > > > Thanks to @Shanahan, Mark for confirming that EPCM does not need to have R value to allow EACCEPT or EMODPE. Given this, can we please revert this change? > > > > Thank you very much for pointing this out. I can revert the change > to what was done in V2 where the only check is to ensure that W requires R. > This is a requirement of EMODPR. Could you please check if this snippet > results in things working for you again? > > ---8<--- > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 83674d054c13..7c7c8a61196e 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -855,12 +855,8 @@ static long sgx_ioc_enclave_restrict_permissions(struct sgx_encl *encl, > if (params.permissions & ~SGX_SECINFO_PERMISSION_MASK) > return -EINVAL; > > - /* > - * Read access is required for the enclave to be able to use the page. > - * SGX instructions like ENCLU[EMODPE] and ENCLU[EACCEPT] require > - * read access. > - */ > - if (!(params.permissions & SGX_SECINFO_R)) > + if ((params.permissions & SGX_SECINFO_W) && > + !(params.permissions & SGX_SECINFO_R)) > return -EINVAL; > > if (params.result || params.count) Just adding that it's fine for me to revert this. BR, Jarkko
On Fri, 2022-04-22 at 12:16 +0300, Jarkko Sakkinen wrote: > On Thu, Apr 21, 2022 at 08:29:31PM -0700, Reinette Chatre wrote: > > Hi Vijay and Mark, > > > > On 4/21/2022 4:46 PM, Dhanraj, Vijay wrote: > > > Hi All, > > > > > > I evaluated V4 patch changes with Gramine and ran into an issue when trying to set EPC page permission to PROT_NONE. It looks like with V3 patch series a change was introduced which requires > > > kernel to have at least R permission when calling RESTRICT IOCTL. This change was done under the assumption that EPCM requires at least R permission for EMODPE/EACCEPT to succeed. But when > > > testing with V2 version, EACCEPT worked fine with page permission set to PROT_NONE. > > > > > > Thanks to @Shanahan, Mark for confirming that EPCM does not need to have R value to allow EACCEPT or EMODPE. Given this, can we please revert this change? > > > > > > > Thank you very much for pointing this out. I can revert the change > > to what was done in V2 where the only check is to ensure that W requires R. > > This is a requirement of EMODPR. Could you please check if this snippet > > results in things working for you again? > > > > ---8<--- > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > > index 83674d054c13..7c7c8a61196e 100644 > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > > @@ -855,12 +855,8 @@ static long sgx_ioc_enclave_restrict_permissions(struct sgx_encl *encl, > > if (params.permissions & ~SGX_SECINFO_PERMISSION_MASK) > > return -EINVAL; > > > > - /* > > - * Read access is required for the enclave to be able to use the page. > > - * SGX instructions like ENCLU[EMODPE] and ENCLU[EACCEPT] require > > - * read access. > > - */ > > - if (!(params.permissions & SGX_SECINFO_R)) > > + if ((params.permissions & SGX_SECINFO_W) && > > + !(params.permissions & SGX_SECINFO_R)) > > return -EINVAL; > > > > if (params.result || params.count) > > Just adding that it's fine for me to revert this. Jethro, I thought it would be also good to get yor view on the current series. Is this something that your platform can live with? BR, Jarkko
Hi Reinette and Jarkko, > On Thu, Apr 21, 2022 at 08:29:31PM -0700, Reinette Chatre wrote: > > Hi Vijay and Mark, > > > > On 4/21/2022 4:46 PM, Dhanraj, Vijay wrote: > > > Hi All, > > > > > > I evaluated V4 patch changes with Gramine and ran into an issue when > trying to set EPC page permission to PROT_NONE. It looks like with V3 patch > series a change was introduced which requires kernel to have at least R > permission when calling RESTRICT IOCTL. This change was done under the > assumption that EPCM requires at least R permission for EMODPE/EACCEPT > to succeed. But when testing with V2 version, EACCEPT worked fine with > page permission set to PROT_NONE. > > > > > > Thanks to @Shanahan, Mark for confirming that EPCM does not need to > have R value to allow EACCEPT or EMODPE. Given this, can we please revert > this change? > > > > > > > Thank you very much for pointing this out. I can revert the change to > > what was done in V2 where the only check is to ensure that W requires R. > > This is a requirement of EMODPR. Could you please check if this > > snippet results in things working for you again? > > > > ---8<--- > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c > > b/arch/x86/kernel/cpu/sgx/ioctl.c index 83674d054c13..7c7c8a61196e > > 100644 > > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > > @@ -855,12 +855,8 @@ static long > sgx_ioc_enclave_restrict_permissions(struct sgx_encl *encl, > > if (params.permissions & ~SGX_SECINFO_PERMISSION_MASK) > > return -EINVAL; > > > > - /* > > - * Read access is required for the enclave to be able to use the page. > > - * SGX instructions like ENCLU[EMODPE] and ENCLU[EACCEPT] > require > > - * read access. > > - */ > > - if (!(params.permissions & SGX_SECINFO_R)) > > + if ((params.permissions & SGX_SECINFO_W) && > > + !(params.permissions & SGX_SECINFO_R)) > > return -EINVAL; > > > > if (params.result || params.count) > > Just adding that it's fine for me to revert this. Thanks, I verified your patch and now I am able to set EPCM page permission with PROT_NONE. I also verified the following SGX2 interfaces, SGX_IOC_ENCLAVE_MODIFY_TYPES SGX_IOC_ENCLAVE_REMOVE_PAGES SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS And also tested dynamically adding pages to enclave using #PF based approach and this works as expected. Please feel free to add my Tested-by for the below patches which test the above IOCTLs [PATCH V4 16/31] x86/sgx: Support adding of pages to an initialized enclave [PATCH V4 15/31] x86/sgx: Support restricting of enclave page permissions [PATCH V4 18/31] x86/sgx: Support modifying SGX page type [PATCH V4 19/31] x86/sgx: Support complete page removal > > BR, Jarkko
Hi Vijay, On 4/25/2022 1:17 PM, Dhanraj, Vijay wrote: >> On Thu, Apr 21, 2022 at 08:29:31PM -0700, Reinette Chatre wrote: >>> On 4/21/2022 4:46 PM, Dhanraj, Vijay wrote: >>>> I evaluated V4 patch changes with Gramine and ran into an issue when >> trying to set EPC page permission to PROT_NONE. It looks like with V3 patch >> series a change was introduced which requires kernel to have at least R >> permission when calling RESTRICT IOCTL. This change was done under the >> assumption that EPCM requires at least R permission for EMODPE/EACCEPT >> to succeed. But when testing with V2 version, EACCEPT worked fine with >> page permission set to PROT_NONE. >>>> >>>> Thanks to @Shanahan, Mark for confirming that EPCM does not need to >> have R value to allow EACCEPT or EMODPE. Given this, can we please revert >> this change? >>>> >>> >>> Thank you very much for pointing this out. I can revert the change to >>> what was done in V2 where the only check is to ensure that W requires R. >>> This is a requirement of EMODPR. Could you please check if this >>> snippet results in things working for you again? >>> >>> ---8<--- >>> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c >>> b/arch/x86/kernel/cpu/sgx/ioctl.c index 83674d054c13..7c7c8a61196e >>> 100644 >>> --- a/arch/x86/kernel/cpu/sgx/ioctl.c >>> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c >>> @@ -855,12 +855,8 @@ static long >> sgx_ioc_enclave_restrict_permissions(struct sgx_encl *encl, >>> if (params.permissions & ~SGX_SECINFO_PERMISSION_MASK) >>> return -EINVAL; >>> >>> - /* >>> - * Read access is required for the enclave to be able to use the page. >>> - * SGX instructions like ENCLU[EMODPE] and ENCLU[EACCEPT] >> require >>> - * read access. >>> - */ >>> - if (!(params.permissions & SGX_SECINFO_R)) >>> + if ((params.permissions & SGX_SECINFO_W) && >>> + !(params.permissions & SGX_SECINFO_R)) >>> return -EINVAL; >>> >>> if (params.result || params.count) >> >> Just adding that it's fine for me to revert this. > > Thanks, I verified your patch and now I am able to set EPCM page permission with PROT_NONE. > > I also verified the following SGX2 interfaces, > SGX_IOC_ENCLAVE_MODIFY_TYPES > SGX_IOC_ENCLAVE_REMOVE_PAGES > SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS > > And also tested dynamically adding pages to enclave using #PF based approach and this works as expected. > > Please feel free to add my Tested-by for the below patches which test the above IOCTLs > > [PATCH V4 16/31] x86/sgx: Support adding of pages to an initialized enclave > [PATCH V4 15/31] x86/sgx: Support restricting of enclave page permissions > [PATCH V4 18/31] x86/sgx: Support modifying SGX page type > [PATCH V4 19/31] x86/sgx: Support complete page removal > Thank you very much for all the testing. I will include the above snippet into V5 of "x86/sgx: Support restricting of enclave page permissions" and add your Tested-by tag to the four patches you listed. Reinette
On Mon, 2022-04-25 at 16:56 -0700, Reinette Chatre wrote: > > [PATCH V4 16/31] x86/sgx: Support adding of pages to an initialized enclave > > [PATCH V4 15/31] x86/sgx: Support restricting of enclave page permissions > > [PATCH V4 18/31] x86/sgx: Support modifying SGX page type > > [PATCH V4 19/31] x86/sgx: Support complete page removal You can add my tested-by to all of the four now [*]. [*] https://github.com/enarx/enarx/pull/1776 BR, Jarkko
Now that the discussions surrounding the support for SGX2 is settling, the kselftest audience is added to the discussion for the first time to consider the testing of the new features. V3: https://lore.kernel.org/lkml/cover.1648847675.git.reinette.chatre@intel.com/ Changes since V3 that directly impact user space: - SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS ioctl()'s struct sgx_enclave_restrict_permissions no longer provides entire secinfo, just the new permissions in new "permissions" struct member. (Jarkko) - Rename SGX_IOC_ENCLAVE_MODIFY_TYPE ioctl() to SGX_IOC_ENCLAVE_MODIFY_TYPES. (Jarkko) - SGX_IOC_ENCLAVE_MODIFY_TYPES ioctl()'s struct sgx_enclave_modify_type no longer provides entire secinfo, just the new page type in new "page_type" struct member. (Jarkko) Details about changes since V3 that do not directly impact user space: - Add new patch to enable VA pages to be added without invoking reclaimer directly if no EPC pages are available, failing instead. This enables VA pages to be added with enclave's mutex held. Fixes an issue encountered by Haitao. More details in new patch "x86/sgx: Support VA page allocation without reclaiming". - While refactoring, change existing code to consistently use IS_ALIGNED(). (Jarkko) - Many patches received a tag from Jarkko. - Many smaller changes, please refer to individual patches. V2: https://lore.kernel.org/lkml/cover.1644274683.git.reinette.chatre@intel.com/ Changes since V2 that directly impact user space: - Maximum allowed permissions of dynamically added pages is RWX, previously limited to RW. (Jarkko) Dynamically added pages are initially created with architecturally limited EPCM permissions of RW. mmap() and mprotect() of these pages with RWX permissions would no longer be blocked by SGX driver. PROT_EXEC on dynamically added pages will be possible after running ENCLU[EMODPE] from within the enclave with appropriate VMA permissions. - The kernel no longer attempts to track the EPCM runtime permissions. (Jarkko) Consequences are: - Kernel does not modify PTEs to follow EPCM permissions. User space will receive #PF with SGX error code in cases where the V2 implementation would have resulted in regular (non-SGX) page fault error code. - SGX_IOC_ENCLAVE_RELAX_PERMISSIONS is removed. This ioctl() was used to clear PTEs after permissions were modified from within the enclave and ensure correct PTEs are installed. Since PTEs no longer track EPCM permissions the changes in EPCM permissions would not impact PTEs. As long as new permissions are within the maximum vetted permissions (vm_max_prot_bits) only ENCLU[EMODPE] from within enclave is needed, as accompanied by appropriate VMA permissions. - struct sgx_enclave_restrict_perm renamed to sgx_enclave_restrict_permissions (Jarkko) - struct sgx_enclave_modt renamed to struct sgx_enclave_modify_type to be consistent with the verbose naming of other SGX uapi structs. Details about changes since V2 that do not directly impact user space: - Kernel no longer tracks the runtime EPCM permissions with the aim of installing accurate PTEs. (Jarkko) - In support of this change the following patches were removed: Documentation/x86: Document SGX permission details x86/sgx: Support VMA permissions more relaxed than enclave permissions x86/sgx: Add pfn_mkwrite() handler for present PTEs x86/sgx: Add sgx_encl_page->vm_run_prot_bits for dynamic permission changes x86/sgx: Support relaxing of enclave page permissions - No more handling of scenarios where VMA permissions may be more relaxed than what the EPCM allows. Enclaves are not prevented from accessing such pages and the EPCM permissions are entrusted to control access as supported by the SGX error code in page faults. - No more explicit setting of protection bits in page fault handler. Protection bits are inherited from VMA similar to SGX1 support. - Selftest patches are moved to the end of the series. (Jarkko) - New patch contributed by Jarkko to avoid duplicated code: x86/sgx: Export sgx_encl_page_alloc() - New patch separating changes from existing patch. (Jarkko) x86/sgx: Export sgx_encl_{grow,shrink}() - New patch to keep one required benefit from the (now removed) kernel EPCM permission tracking: x86/sgx: Support loading enclave page without VMA permissions check - Updated cover letter to reflect architecture changes. - Many smaller changes, please refer to individual patches. V1: https://lore.kernel.org/linux-sgx/cover.1638381245.git.reinette.chatre@intel.com/ Changes since V1 that directly impact user space: - SGX2 permission changes changed from a single ioctl() named SGX_IOC_PAGE_MODP to two new ioctl()s: SGX_IOC_ENCLAVE_RELAX_PERMISSIONS and SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS, supported by two different parameter structures (SGX_IOC_ENCLAVE_RELAX_PERMISSIONS does not support a result output parameter) (Jarkko). User space flow impact: After user space runs ENCLU[EMODPE] it needs to call SGX_IOC_ENCLAVE_RELAX_PERMISSIONS to have PTEs updated. Previously running SGX_IOC_PAGE_MODP in this scenario resulted in EPCM.PR being set but calling SGX_IOC_ENCLAVE_RELAX_PERMISSIONS will not result in EPCM.PR being set anymore and thus no need for an additional ENCLU[EACCEPT]. - SGX_IOC_ENCLAVE_RELAX_PERMISSIONS and SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS obtain new permissions from secinfo as parameter instead of the permissions directly (Jarkko). - ioctl() supporting SGX2 page type change is renamed from SGX_IOC_PAGE_MODT to SGX_IOC_ENCLAVE_MODIFY_TYPE (Jarkko). - SGX_IOC_ENCLAVE_MODIFY_TYPE obtains new page type from secinfo as parameter instead of the page type directly (Jarkko). - ioctl() supporting SGX2 page removal is renamed from SGX_IOC_PAGE_REMOVE to SGX_IOC_ENCLAVE_REMOVE_PAGES (Jarkko). - All ioctl() parameter structures have been renamed as a result of the ioctl() renaming: SGX_IOC_ENCLAVE_RELAX_PERMISSIONS => struct sgx_enclave_relax_perm SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS => struct sgx_enclave_restrict_perm SGX_IOC_ENCLAVE_MODIFY_TYPE => struct sgx_enclave_modt SGX_IOC_ENCLAVE_REMOVE_PAGES => struct sgx_enclave_remove_pages Changes since V1 that do not directly impact user space: - Number of patches in series increased from 25 to 32 primarily because of splitting the original submission: - Wrappers for the new SGX2 functions are introduced in three separate patches replacing the original "x86/sgx: Add wrappers for SGX2 functions" (Jarkko). - Moving and renaming sgx_encl_ewb_cpumask() is done with two patches replacing the original "x86/sgx: Use more generic name for enclave cpumask function" (Jarkko). - Support for SGX2 EPCM permission changes is split into two ioctls(), one for relaxing and one for restricting permissions, each introduced by a new patch replacing the original "x86/sgx: Support enclave page permission changes" (Jarkko). - Extracted code used by existing ioctls() for usage by new ioctl()s into a new utility in new patch "x86/sgx: Create utility to validate user provided offset and length" (Dave did not specifically ask for this but it addresses his review feedback). - Two new Documentation patches to support the SGX2 work ("Documentation/x86: Introduce enclave runtime management") and a dedicated section on the enclave permission management ("Documentation/x86: Document SGX permission details") (Andy). - Most patches were reworked to improve the language by: * aiming to refer to exact item instead of English rephrasing (Jarkko). * use ioctl() instead of ioctl throughout (Dave). * Use "relaxed" instead of "exceed" when referring to permissions (Dave). - Improved documentation with several additions to Documentation/x86/sgx.rst. - Many smaller changes, please refer to individual patches. Hi Everybody, The current Linux kernel support for SGX includes support for SGX1 that requires that an enclave be created with properties that accommodate all usages over its (the enclave's) lifetime. This includes properties such as permissions of enclave pages, the number of enclave pages, and the number of threads supported by the enclave. Consequences of this requirement to have the enclave be created to accommodate all usages include: * pages needing to support relocated code are required to have RWX permissions for their entire lifetime, * an enclave needs to be created with the maximum stack and heap projected to be needed during the enclave's entire lifetime which can be longer than the processes running within it, * an enclave needs to be created with support for the maximum number of threads projected to run in the enclave. Since SGX1 a few more functions were introduced, collectively called SGX2, that support modifications to an initialized enclave. Hardware supporting these functions are already available as listed on https://github.com/ayeks/SGX-hardware This series adds support for SGX2, also referred to as Enclave Dynamic Memory Management (EDMM). This includes: * Support modifying EPCM permissions of regular enclave pages belonging to an initialized enclave. Only permission restriction is supported via a new ioctl() SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS. Relaxing of EPCM permissions can only be done from within the enclave with the SGX instruction ENCLU[EMODPE]. * Support dynamic addition of regular enclave pages to an initialized enclave. At creation new pages are architecturally limited to RW EPCM permissions but will be accessible with PROT_EXEC after the enclave runs ENCLU[EMODPE] to relax EPCM permissions to RWX. Pages are dynamically added to an initialized enclave from the SGX page fault handler. * Support expanding an initialized enclave to accommodate more threads. More threads can be accommodated by an enclave with the addition of Thread Control Structure (TCS) pages that is done by changing the type of regular enclave pages to TCS pages using a new ioctl() SGX_IOC_ENCLAVE_MODIFY_TYPES. * Support removing regular and TCS pages from an initialized enclave. Removing pages is accomplished in two stages as supported by two new ioctl()s SGX_IOC_ENCLAVE_MODIFY_TYPES (same ioctl() as mentioned in previous bullet) and SGX_IOC_ENCLAVE_REMOVE_PAGES. * Tests covering all the new flows, some edge cases, and one comprehensive stress scenario. No additional work is needed to support SGX2 in a virtualized environment. All tests included in this series passed when run from a guest as tested with the recent QEMU release based on 6.2.0 that supports SGX. Patches 1 through 14 prepare the existing code for SGX2 support by introducing the SGX2 functions, refactoring code, and tracking enclave page types. Patches 15 through 21 enable the SGX2 features and include a Documentation patch. Patches 22 through 31 test several scenarios of all the enabled SGX2 features. This series is based on v5.18-rc2. Your feedback will be greatly appreciated. Regards, Reinette Jarkko Sakkinen (1): x86/sgx: Export sgx_encl_page_alloc() Reinette Chatre (30): x86/sgx: Add short descriptions to ENCLS wrappers x86/sgx: Add wrapper for SGX2 EMODPR function x86/sgx: Add wrapper for SGX2 EMODT function x86/sgx: Add wrapper for SGX2 EAUG function x86/sgx: Support loading enclave page without VMA permissions check x86/sgx: Export sgx_encl_ewb_cpumask() x86/sgx: Rename sgx_encl_ewb_cpumask() as sgx_encl_cpumask() x86/sgx: Move PTE zap code to new sgx_zap_enclave_ptes() x86/sgx: Make sgx_ipi_cb() available internally x86/sgx: Create utility to validate user provided offset and length x86/sgx: Keep record of SGX page type x86/sgx: Export sgx_encl_{grow,shrink}() x86/sgx: Support VA page allocation without reclaiming x86/sgx: Support restricting of enclave page permissions x86/sgx: Support adding of pages to an initialized enclave x86/sgx: Tighten accessible memory range after enclave initialization x86/sgx: Support modifying SGX page type x86/sgx: Support complete page removal x86/sgx: Free up EPC pages directly to support large page ranges Documentation/x86: Introduce enclave runtime management section selftests/sgx: Add test for EPCM permission changes selftests/sgx: Add test for TCS page permission changes selftests/sgx: Test two different SGX2 EAUG flows selftests/sgx: Introduce dynamic entry point selftests/sgx: Introduce TCS initialization enclave operation selftests/sgx: Test complete changing of page type flow selftests/sgx: Test faulty enclave behavior selftests/sgx: Test invalid access to removed enclave page selftests/sgx: Test reclaiming of untouched page selftests/sgx: Page removal stress test Documentation/x86/sgx.rst | 15 + arch/x86/include/asm/sgx.h | 8 + arch/x86/include/uapi/asm/sgx.h | 61 + arch/x86/kernel/cpu/sgx/encl.c | 329 +++- arch/x86/kernel/cpu/sgx/encl.h | 15 +- arch/x86/kernel/cpu/sgx/encls.h | 33 + arch/x86/kernel/cpu/sgx/ioctl.c | 640 +++++++- arch/x86/kernel/cpu/sgx/main.c | 75 +- arch/x86/kernel/cpu/sgx/sgx.h | 3 + tools/testing/selftests/sgx/defines.h | 23 + tools/testing/selftests/sgx/load.c | 41 + tools/testing/selftests/sgx/main.c | 1435 +++++++++++++++++ tools/testing/selftests/sgx/main.h | 1 + tools/testing/selftests/sgx/test_encl.c | 68 + .../selftests/sgx/test_encl_bootstrap.S | 6 + 15 files changed, 2625 insertions(+), 128 deletions(-) base-commit: ce522ba9ef7e2d9fb22a39eb3371c0c64e2a433e