Re: [RFC PATCH v2.1 14/30] x86/sgx: Support restricting of enclave page permissions

[Jarkko Sakkinen]

On Wed, Mar 09, 2022 at 08:59:42AM -0800, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote:
> > On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote:
> >> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote:
> >>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \
> >>> +	_IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm)
> >>
> >> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which
> >> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to
> >> flush out the TLB's, and move a page to pending state, which cannot be done
> >> from inside the enclave.
> 
> I see the main ingredient as running EMODPR to restrict the EPCM permissions. If
> the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is
> already possible since attempting to use EMODPR to relax permissions does not
> change any permissions (although it still sets EPCM.PR) but yet will still
> flush the TLB.

It's not just to flush the TLB. It also resets permissions to zero from
which it is easy to set the exact permissions with EMODPE.

> Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS
> removes the ability for users to flush the TLB after an EMODPE. If there are
> thus PTEs present at the time the user runs EMODPE the pages would not be
> accessible with the new permissions.
> 
> Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish
> this is not efficient because:
> - For the OS to flush the TLB the enclave pages need not be in the EPC but
>   in order to run EMODPR the enclave page needs to be in the EPC. In an 
>   oversubscribed environment running EMODPR unnecessarily can thus introduce
>   a significant delay. Please see the performance comparison I did in
>   https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@xxxxxxxxx/
>   The test shows that running EMODPR unnecessarily can be orders of magnitude slower.
> - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page
>   that needs to be cleared with an EACCEPT from within the enclave.
>   If the user just wants to reset the TLB after running EMODPE then it should
>   not be necessary to run EACCEPT again to reset EPCM.PR.
> 
> Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an 
> efficient way - it is quick (no need to load pages into EPC) and it does not
> require EACCEPT to clear EPCM.PR. 
> 
> It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could
> rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer.

Please do not add it. We do not have any use for it. It's not only used
to flush TLB's so it would not do any good. I just use it with fixed
PROT_NONE permissions.

> >> It's there because of microarchitecture constraints, and less so to work as
> >> a reasonable permission control mechanism (actually it does terrible job on
> >> that side and only confuses).
> >>
> >> Once you have this magic TLB reset button in place you can just do one
> >> EACCEPT and EMODPE inside the enclave and you're done.
> >>
> >> This is also kind of atomic in the sense that EACCEPT free's a page with no
> >> rights so no misuse can happend before EMODPE has tuned EPCM.
> > 
> > I wonder if this type of pattern could be made work out for Graphene:
> > 
> > 1. SGX_IOC_ENCLAVE_RESET_PAGES
> > 2. EACCEPT + EMODPE
> > 
> > This kind of delivers EMODP that everyone has been looking for.
> 
> EACCEPT will result in page table entries created for the enclave page. EMODPE
> will be able to relax the permissions but TLB flush would be required to
> access the page with the new permissions. SGX_IOC_ENCLAVE_RELAX_PERMISSIONS
> (renamed to SGX_IOC_ENCLAVE_RESET_PAGES?) that does just a TLB flush is
> required to be after EMODPE.

For EMODPE TLB flush is not required. I even verified this from Mark
Shanahan. And since access rights are zero, the page cannot be
deferenced by threads before EMODPE.


BR, Jarkko