Re: [PATCH 10/25] x86/sgx: Support enclave page permission changes

[Andy Lutomirski <luto@kernel.org>]

On 12/3/21 14:34, Reinette Chatre wrote:
> Hi Andy,
> 
> On 12/3/2021 11:38 AM, Andy Lutomirski wrote:
>> On 12/1/21 11:23, Reinette Chatre wrote:
>>> In the initial (SGX1) version of SGX, pages in an enclave need to be
>>> created with permissions that support all usages of the pages, from the
>>> time the enclave is initialized until it is unloaded. For example,
>>> pages used by a JIT compiler or when code needs to otherwise be
>>> relocated need to always have RWX permissions.
>>>
>>> SGX2 includes two functions that can be used to modify the enclave page
>>> permissions of regular enclave pages within an initialized enclave.
>>> ENCLS[EMODPR] is run from the OS and used to restrict enclave page
>>> permissions while ENCLU[EMODPE] is run from within the enclave to
>>> extend enclave page permissions.
>>>
>>> Enclave page permission changes need to be approached with care and
>>> for this reason this initial support is to allow enclave page
>>> permission changes _only_ if the new permissions are the same or
>>> more restrictive that the permissions originally vetted at the time the
>>> pages were added to the enclave. Support for extending enclave page
>>> permissions beyond what was originally vetted is deferred.
>>>
>>
>> I may well be missing something, but off the top of my head, literally 
>> the only reason that EMODPR needs CPL0 (i.e. ENCLS) is that it 
>> requires a TLB flush IPI to take effect.  (Score one for AMD for being 
>> having superior hardware in this regard.)
> 
> My understanding also is that it is the need for TLB flush that require 
> the privilege but I am trying to get more information here.
> 
>>
>> Given that, I don't see any reason for the EMODPR operation to be 
>> treated as security sensitive -- it just needs to be implemented 
>> correctly.  I don't even see why the host should (or even can!) do any 
>> useful tracking of the EPCM state.
> 
> The OS needs to know the EPCM permissions to be able to install the 
> appropriate PTEs. If the enclave chooses to change the enclave page 
> permissions from within the enclave then user space needs to let the OS 
> know via the SGX_IOC_PAGE_MODP ioctl to ensure that the OS can install 
> correct PTEs in support of the permission change.
> 
> 
>> (But I am confused about one thing: to the extent an enclave actually 
>> needs EMODPR, is there anything in the hardware or anything that the 
>> enclave can do short of actually poking the page from all threads and 
>> confirming that a fault occurs to make sure the OS actually flushed 
>> the TLB?  ISTM a malicious host could attack an enclave by omitting 
>> the TLB flush and then exploiting an enclave but that would have been 
>> mitigated if the flush occurred.)
> 
> When enclave page permissions are restricted it requires the enclave to 
> accept the new permissions from within the enclave by running 
> ENCLU[EACCEPT]. This instruction requires that (it will fail otherwise) 
> the OS completed an ENCLS[ETRACK] on the affected page - essentially 
> ENCLU[EACCEPT] can only succeed if no cached linear-to-physical address 
> mappings are present. The ETRACK flow is elaborate and I attempted to 
> document it in patch 06/25. Essentially, SGX hardware flushes all cached 
> linear-to-physical mappings when an enclave is exited and with ETRACK it 
> can be ensured that all threads that were in an enclave at the time the 
> tracking started (in this case after ENCLS[EMODPR]), have exited.
> 

Does the enclave do something before asking for the ioctl to put the 
page in a state where the tracking is armed?  I read the SDM, but I 
probably read the wrong part of the SDM, and I may have missed this.