Message-ID: <86495779-a9c5-45d5-0017-c491bf6354ab@intel.com>
Date:   Tue, 22 Feb 2022 14:39:31 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Nathaniel McCallum <nathaniel@...fian.com>
CC:     <dave.hansen@...ux.intel.com>, Jarkko Sakkinen <jarkko@...nel.org>,
        <tglx@...utronix.de>, <bp@...en8.de>,
        Andy Lutomirski <luto@...nel.org>, <mingo@...hat.com>,
        <linux-sgx@...r.kernel.org>, <x86@...nel.org>, <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 V2 00/32] x86/sgx and selftests/sgx: Support SGX2

Hi Nathaniel,

On 2/22/2022 12:27 PM, Nathaniel McCallum wrote:
> 1. This interface looks very odd to me. mmap() is the kernel interface
> for changing user space memory maps. Why are we introducing a new
> interface for this?

mmap() is the kernel interface used to create new mappings in the
virtual address space of the calling process. This is different from
the permissions and properties of the underlying file/memory being mapped.

A new interface is introduced because changes need to be made to the
permissions and properties of the underlying enclave. A new virtual
address space is not needed nor should existing VMAs be impacted.

This is similar to how mmap() is not used to change file permissions.

VMA permissions are separate from enclave page permissions as found in
the EPCM (Enclave Page Cache Map). The current implementation (SGX1) already
distinguishes between the VMA and EPCM permissions - for example, it is
already possible to create a read-only VMA from enclave pages that have
RW EPCM permissions. mmap() of a portion of EPC memory with a particular
permission does not imply that the underlying EPCM permissions (should)have
that permission. 

> You can just simply add a new mmap flag (i.e.
> MAP_SGX_TCS*) and then figure out which SGX instructions to execute
> based on the desired state of the memory maps. If you do this, none of
> the following ioctls are needed:
> 
> * SGX_IOC_ENCLAVE_RELAX_PERMISSIONS
> * SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS
> * SGX_IOC_ENCLAVE_REMOVE_PAGES
> * SGX_IOC_ENCLAVE_MODIFY_TYPE
> 
> It also means that languages don't have to grow support for all these
> ioctls. Instead, they can just reuse the existing mmap() bindings with
> the new flag. Also, multiple operations can be combined into a single
> mmap() call, amortizing the changes over a single context switch.
> 
> 2. Automatically adding pages with hard-coded permissions in a fault
> handler seems like a really bad idea.

Could you please elaborate why this is a bad idea?

> How do you distinguish between
> accesses which should result in an updated mapping and accesses that
> should result in a fault?

Accesses that should result in an updated mapping have two requirements:
(a) address accessed belongs to the enclave based on the address
    range specified during enclave create
(b) there is no backing enclave page for the address

> IMHO, all unmapped page accesses should
> result in a page fault. mmap() should be called first to identify the
> correct permissions for these pages.
> Then the page handler should be
> updated to use the permissions from the mapping when backfilling
> physical pages. If I understand correctly, this should also obviate

Regular enclave pages can _only_ be dynamically added with RW permission.

SGX2's support for adding regular pages to an enclave via the EAUG
instruction is architecturally set at RW. The OS cannot change those permissions
via the EAUG instruction nor can the OS do so with a different/additional
instruction because:
* the OS is not able to relax permissions since that can only be done from
within the enclave with ENCLU[EMODPE], thus it is not possible for the OS to
dynamically add pages via EAUG as RW and then relax permissions to RWX. 
* the OS is not able to EAUG a page and immediately attempt an EMODPR either
as Jarkko also recently inquired about:
https://lore.kernel.org/linux-sgx/80f3d7b9-e3d5-b2c0-7707-710bf6f5081e@intel.com/

> the need for the weird userspace callback to allow for execute
> permissions.

User policy integration would always be required to allow execute
permissions on a writable page. This is not expected to be a userspace
callback but instead integration with existing user policy subsystem(s).

> 
> 3. Implementing as I've suggested also means that we can lock down an
> enclave, for example - after code has been JITed, by closing the file
> descriptor. Once the file descriptor used to create the enclave is
> closed, no further mmap() can be performed on the enclave. Attempting
> to do EACCEPT on an unmapped page will generate a page fault.

This is not clear to me. If the file descriptor is closed and no further
mmap() is allowed then how would a process be able to enter the enclave
to execute code within it?

This series does indeed lock down the address range to ensure that it is
not possible to map memory that does not belong to the enclave after the
enclave is created. Please see:
https://lore.kernel.org/linux-sgx/1b833dbce6c937f71523f4aaf4b2181b9673519f.1644274683.git.reinette.chatre@intel.com/

> 
> * - I'm aware that a new flag might be frowned upon. I see a few other options:
> 1. reuse an existing flag which doesn't make sense in this context
> 2. communicate the page type in the offset argument
> 3. keep SGX_IOC_ENCLAVE_MODIFY_TYPE
> 

Reinette

Powered by blists - more mailing lists