Re: [PATCH 05/25] x86/sgx: Introduce runtime protection bits

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Andy,

On 12/4/2021 9:56 AM, Andy Lutomirski wrote:
> On 12/3/21 17:14, Reinette Chatre wrote:
>> Hi Andy,
>>
>> On 12/3/2021 4:38 PM, Andy Lutomirski wrote:
>>> On 12/3/21 14:12, Reinette Chatre wrote:
>>>> Hi Andy,
>>>>
>>>> On 12/3/2021 11:28 AM, Andy Lutomirski wrote:
>>>>> On 12/1/21 11:23, Reinette Chatre wrote:
>>>>>> Enclave creators declare their paging permission intent at the time
>>>>>> the pages are added to the enclave. These paging permissions are
>>>>>> vetted when pages are added to the enclave and stashed off
>>>>>> (in sgx_encl_page->vm_max_prot_bits) for later comparison with
>>>>>> enclave PTEs.
>>>>>>
>>>>>
>>>>> I'm a bit confused here. ENCLU[EMODPE] allows the enclave to change 
>>>>> the EPCM permission bits however it likes with no oversight from 
>>>>> the kernel.   So we end up with a whole bunch of permission masks:
>>>>
>>>> Before jumping to the permission masks I would like to step back and 
>>>> just confirm the context. We need to consider the following three 
>>>> permissions:
>>>>
>>>> EPCM permissions: the enclave page permissions maintained in the SGX 
>>>> hardware. The OS is constrained here in that it cannot query the 
>>>> current EPCM permissions. Even so, the OS needs to ensure PTEs are 
>>>> installed appropriately (we do not want a RW PTE for a read-only 
>>>> enclave page)
>>>
>>> Why not?  What's wrong with an RW PTE for a read-only enclave page?
>>>
>>> If you convince me that this is actually important, then I'll read 
>>> all the stuff below.
>>
>> Perhaps it is my misunderstanding/misinterpretation of the current 
>> implementation? From what I understand the current requirement, as 
>> enforced in the current mmap(), mprotect() as well as fault() hooks, 
>> is that mappings are required to have identical or weaker permission 
>> than the enclave permission.
> 
> The current implementation does require that, but for a perhaps 
> counterintuitive reason.  If a SELinux-restricted (or similarly 
> restricted) process that is *not* permitted to do JIT-like things loads 
> an enclave, it's entirely okay for it to initialize RW enclave pages 
> however it likes and it's entirely okay for it to initialize RX (or XO 
> if that ever becomes a thing) enclave pages from appropriately files on 
> disk.  But it's not okay for it to create RWX enclave pages or to 
> initialize RX enclave pages from untrusted application memory. [0]
> 
> So we have a half-baked implementation right now: the permission to 
> execute a page is decided based on secinfo (max permissions) when the 
> enclave is set up, and it's enforced at the PTE level.  The PTE 
> enforcement is because, on SGX2 hardware, the enclave can do EMODPE and 
> bypass any supposed restrictions in the EPCM.
> 
> The only coupling between EPCM and PTE here is that the max_perm is 
> initialized together with EPCM, but it didn't have to be that way.
> 
> An SGX2 implementation needs to be more fully baked, because in a 
> dynamic environment enclaves need to be able to use EMODPE and actually 
> end up with permissions that exceed the initial secinfo permissions.  So 

Could you please elaborate why this is a requirement? In this 
implementation the secinfo of a page added before enclave initialization 
(via SGX_IOC_ENCLAVE_ADD_PAGES) would indicate the maximum permissions 
it may have during its lifetime. Pages needing to be writable and 
executable during their lifetime can be created with RWX secinfo and 
during the enclave runtime the pages could obtain all combinations of 
permissions: RWX, R, RW, RX. A page added with RW secinfo may have R or 
RW permissions during its lifetime but never RX or RWX.

So far our inquiries on whether this is acceptable has been positive and 
is also what Dave attempted to put a spotlight on in:
https://lore.kernel.org/lkml/94d8d631-5345-66c4-52a3-941e52500f84@intel.com/

This above is specific to pages added before enclave initialization. In 
this implementation pages added after enclave initialization, those 
needing the ENCLS[EAUG] SGX2 instruction, are added with max permissions 
of RW so could only have R or RW permissions during their lifetime. This 
is an understood limitation and it is understood that integration with 
user policy is required to support these pages obtaining executable 
permission. The plan is to handle user policy integration in a series 
that follows this core SGX2 enabling.

> it needs to be possible to make a page that starts out R (or RW or 
> whatever) but nonetheless has max_perm=RWX so that the enclave can use a 
> combination of EMODPE and (ioctl-based) EMODPR to do JIT.  So I think 
> you should make it possible to set up pages like this, but I see no 
> reason to couple the PTE and the EPCM permissions.
> 
>>
>> Could you please elaborate how you envision PTEs should be managed in 
>> this implementation?
> 
> As above: PTE permissions may not exceed max_perm, and EPCM is entirely 
> separate except to the extent needed for ABI compatibility with SGX1 
> runtimes.

ok, so if I understand correctly you, since PTE permissions may not 
exceed max_perm and EPCM are separate, this seems to get back to your 
previous question of "What's wrong with an RW PTE for a read-only 
enclave page?"

This is indeed something that we could allow but not doing so (that is 
PTEs not exceeding EPCM permissions) would better support the SGX 
runtime. That is why I separated out the addition of the pfn_mkwrite() 
callback in the previous patch (04/25). Like in your example, there is a 
RW mapping of a read-only enclave page that first results in a RW PTE 
for the read-only enclave page. That would result in a #PF with the SGX 
flag set (0x8007). If the PTE matches the enclave permissions the page 
fault would have familiar 0x7 error code.

In either case user space would encounter a #PF so technically there is 
nothing "wrong" with allowing this - even so, as motivated in the 
previous patch: accurate exception information supports the SGX runtime, 
which is virtually always implemented inside a shared library, by 
providing accurate information in support of its management of the SGX 
enclave.


> [0] I'm not sure anyone actually has a system set up like this or that 
> the necessary LSM support is in the kernel.  But it's supposed to be 
> possible without changing the ABI.
> 

Reinette