Re: [PATCH V2 16/32] x86/sgx: Support restricting of enclave page permissions

[Jarkko Sakkinen]

On Fri, Mar 11, 2022 at 09:53:29AM -0800, Reinette Chatre wrote:
 
> I do not believe that you encountered the #GP documented above because that
> check is already present in the current implementation of
> SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS:
> 
> sgx_ioc_enclave_restrict_permissions()->sgx_perm_from_user_secinfo():
> 	if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
> 		return -EINVAL;
> 
> It does return EINVAL which is the catch-all error code used to represent
> invalid input from user space. I am not convinced that EACCES should be used
> instead though, EACCES means "Permission denied", which is not the case here.
> The case here is just an invalid request.
> 
> It currently does not prevent the user from setting PROT_NONE though, which
> EMODPR does seem to allow.
> 
> I saw Haitao's note that EMODPE requires "Read access permitted by enclave".
> This motivates that EMODPR->PROT_NONE should not be allowed since it would
> not be possible to relax permissions (run EMODPE) after that. Even so, I
> also found in the SDM that EACCEPT has the note "Read access permitted
> by enclave". That seems to indicate that EMODPR->PROT_NONE is not practical
> from that perspective either since the enclave will not be able to
> EACCEPT the change. Does that match your understanding?
> 
> I will add the check for R in SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS at least.

Yes, I think we are in the same line with this.

But there is another thing.

As EAUG is taken care by the page handler so should EMODPR. It makes the
developer experience whole a lot easier when you don't have to back call
to host and ask it to execute EMODPR for the range.

It's also a huge incosistency in this patch set that they are handled
differently.

And it creates a concurrency case for user space that is complicated to say
the least, i.e. divided work between host and enclave implementation to
execute EMODPR is a nightmare scenario. On the other hand this is trivial
to sort out in kernel.

So what it means that, in one way or antoher, mprotect() needs to be the
melting point for both. This can be called mandatory requirement, however
this patch set it done, not least because of managing concurrency between
kernel and user space.

You can get that done by these steps:

1. Unmap PTE's in mprotect() flow.
2. In #PF handler, EMODPR with R set.

This clear API for enclave developer because you know in what state pages
are after mprotect(), and what you need to still do to them. Only the
syscall needs to be them performed by the host side.

BR, Jarkko