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
