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
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
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) -- Thank you very much Reinette
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
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
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
© 2016 - 2026 Red Hat, Inc.