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
