Message-ID: <fc4a8c94-4e1a-be53-50fe-802f6f985eaa@intel.com>
Date:   Fri, 3 Dec 2021 14:34:22 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Andy Lutomirski <luto@...nel.org>, <dave.hansen@...ux.intel.com>,
        <jarkko@...nel.org>, <tglx@...utronix.de>, <bp@...en8.de>,
        <mingo@...hat.com>, <linux-sgx@...r.kernel.org>, <x86@...nel.org>
CC:     <seanjc@...gle.com>, <kai.huang@...el.com>,
        <cathy.zhang@...el.com>, <cedric.xing@...el.com>,
        <haitao.huang@...el.com>, <mark.shanahan@...el.com>,
        <hpa@...or.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/25] x86/sgx: Support enclave page permission changes

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.

Reinette

Powered by blists - more mailing lists