Message-ID: <e1c04077-0165-c5ec-53be-7fd732965e80@intel.com>
Date:   Fri, 11 Mar 2022 11:28:27 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>
CC:     Haitao Huang <haitao.huang@...ux.intel.com>,
        "Dhanraj, Vijay" <vijay.dhanraj@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "bp@...en8.de" <bp@...en8.de>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        "Zhang, Cathy" <cathy.zhang@...el.com>,
        "Xing, Cedric" <cedric.xing@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        "Shanahan, Mark" <mark.shanahan@...el.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 16/32] x86/sgx: Support restricting of enclave page
 permissions

Hi Jarkko,

On 3/11/2022 10:11 AM, Jarkko Sakkinen wrote:
> 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.

EMODPR has possible failures due to state that is managed by the user space
runtime. Being able to communicate accurate EMODPR error codes to user space
runtime is helpful to the runtime in supporting its management of the enclave
memory. Accurate EMODPR error codes can be communicated when using an ioctl(),
not when run from within a page fault handler. 
 
> So what it means that, in one way or antoher, mprotect() needs to be the
> melting point for both.

mprotect() is the syscall to modify VMA permissions. EPCM permissions are
different from VMA permissions and they are currently treated differently
by the kernel. 

Moving EPCM permission changes to mprotect() forces EPCM permissions to be
the same as VMA permissions. That is a significant change. It is also
inconsistent since EPCM permission changes cannot be managed completely
from the kernel since the kernel can only ever restrict permissions.

> 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.

There is also the very significant ETRACK flow that
needs to be run after EMODPR. The implications of sending IPIs to all
CPUs that may be running in an enclave while in a page fault handler needs
to be considered. Page faults should be as fast as possible.

If this is considered then this tremendous impact on the page fault handler
should be managed and avoided as much as possible - but how will the page
fault handler even know when it should run EMODPR? The enclave can run 
EMODPE from within the enclave at any time without any insight from the
kernel so the only way to have accurate permissions would then be to
run EMODPR on _every_ page fault, which is obviously a non-starter due
to the significant impact (EMODPR and ETRACK) and blast radius (IPIs).

Trying to move running of EMODPR earlier, during the mprotect() call itself
is also full of obstacles since the mprotect() call may result in VMAs
being split, which is an operation that can fail, and followed by
the EMODPR-ETRACK flows that can also fail (and not be able to 
undo the VMA splits). With the EMODPR-ETRACK flows that can fail it
is here also not possible to communicate accurately to user space since
now there is the whole page range to consider, for example, mprotect()
cannot communicate
(a) which pages caused the failure, and (b) what failure was encountered.
This is possible when using the ioctl().


> 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.

Supporting permission restriction in an ioctl() enables the runtime to manage
the enclave memory without needing to map it.

I have considered the idea of supporting the permission restriction with
mprotect() but as you can see in this response I did not find it to be
practical.

Reinette

Powered by blists - more mailing lists