Re: [PATCH 03/25] x86/sgx: Support VMA permissions exceeding enclave permissions

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

Hi Jarkko,

On 12/10/2021 9:39 PM, Jarkko Sakkinen wrote:
> On Mon, 2021-12-06 at 13:16 -0800, Reinette Chatre wrote:
>> On 12/4/2021 2:27 PM, Jarkko Sakkinen wrote:
>>> On Sun, Dec 05, 2021 at 12:25:59AM +0200, Jarkko Sakkinen wrote:
>>>> On Wed, Dec 01, 2021 at 11:23:01AM -0800, Reinette Chatre wrote:
>>>>> === Summary ===
>>>>>
>>>>> An SGX VMA can only be created if its permissions are the same or
>>>>> weaker than the Enclave Page Cache Map (EPCM) permissions. After VMA
>>>>> creation this rule continues to be enforced by the page fault handler.
>>>>>
>>>>> With SGX2 the EPCM permissions of a page can change after VMA
>>>>> creation resulting in the VMA exceeding the EPCM permissions and the
>>>>> page fault handler incorrectly blocking access.
>>>>>
>>>>> Enable the VMA's pages to remain accessible while ensuring that
>>>>> the page table entries are installed to match the EPCM permissions
>>>>> without exceeding the VMA perms issions.
>>>>
>>>> I don't understand what the short summary means in English, and the
>>>> commit message is way too bloated to make any conclusions. It really
>>>> needs a rewrite.
>>>>
>>>> These were the questions I could not find answer for:
>>>>
>>>> 1. Why it would be by any means safe to remove a permission check?
>>
>> The permission check is redundant for SGX1 and incorrect for SGX2.
>>
>> In the current SGX1 implementation the permission check in
>> sgx_encl_load_page() is redundant because an SGX VMA can only be created
>> if its permissions are the same or weaker than the EPCM permissions.
>>
>> In SGX2 a user is able to change EPCM permissions during runtime (while
>> VMA has the memory mapped). A RW VMA may thus originally have mapped an
>> enclave page with RW EPCM permissions but since then the enclave page
>> may have its permissions changed to read-only. The VMA should still be
>> able to read those enclave pages but the check in sgx_encl_load_page()
>> will prevent that.
>>
>>>> 2. Why not re-issuing mmap()'s is unfeasible? I.e. close existing
>>>>      VMA's and mmap() new ones.
>>
>> User is not prevented from closing existing VMAs and creating new ones.
>>
>>> 3. Isn't this an API/ABI break?
>>
>> Could you please elaborate where you see the API/ABI break? The rule
>> that new VMAs cannot exceed EPCM permissions is untouched.
>>
>> Reinette
> 
> I just don't understand the description. There's a whole bunch of text
> but
> 
> It does not discuss what the patch does in low-level detail what the
> patch does, e.g. the use of vm_insert_pfn_prot(). I honestly do not
> get the story here...

vmf_insert_pfn_prot() replaces the existing call to vmf_insert_pfn().

Notice how:

vm_fault_t vmf_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
                           unsigned long pfn)
{
         return vmf_insert_pfn_prot(vma, addr, pfn, vma->vm_page_prot);
}

vmf_insert_pfn() is replaced with the function it would call anyway. It 
is done because the PTE being installed should no longer blindly inherit 
the VMA permission as is done in the current code, but it should also 
take the EPCM permissions into account. This is because the EPCM 
permissions can change after the VMA is created.

For example, consider a RW VMA created to map pages with RW EPCM pages.
Since SGX1 does not allow EPCM permission changes it is ok to always 
install RW PTEs to access those pages and thus vmf_insert_pfn() is 
sufficient. In SGX2 the EPCM pages may become read-only and the PTEs 
should no longer be RW. This is made possible with the call to 
vmf_insert_pfn_prot() where the protection bits for the PTE can be 
provided (so that the PTE permissions do not exceed the EPCM permissions).

Reinette