From: Jarkko Sakkinen <jarkko@kernel.org>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: dave.hansen@linux.intel.com, tglx@linutronix.de, bp@alien8.de,
luto@kernel.org, mingo@redhat.com, linux-sgx@vger.kernel.org,
x86@kernel.org, seanjc@google.com, kai.huang@intel.com,
cathy.zhang@intel.com, cedric.xing@intel.com,
haitao.huang@intel.com, mark.shanahan@intel.com, hpa@zytor.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 06/32] x86/sgx: Support VMA permissions more relaxed than enclave permissions
Date: Mon, 7 Mar 2022 19:10:04 +0200 [thread overview]
Message-ID: <YiY8bD/QcmcHH8dz@iki.fi> (raw)
In-Reply-To: <0555a4b4a5e8879eb8f879ab3d9908302000f11c.1644274683.git.reinette.chatre@intel.com>
On Mon, Feb 07, 2022 at 04:45:28PM -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 same rule is again enforced by the page fault handler:
> faulted enclave pages are required to have equal or more relaxed
> EPCM permissions than the VMA permissions.
>
> On SGX1 systems the additional enforcement in the page fault handler
> is redundant and on SGX2 systems it incorrectly prevents access.
> On SGX1 systems it is unnecessary to repeat the enforcement of the
> permission rule. The rule used during original VMA creation will
> ensure that any access attempt will use correct permissions.
> With SGX2 the EPCM permissions of a page can change after VMA
> creation resulting in the VMA permissions potentially being more
> relaxed than the EPCM permissions and the page fault handler
> incorrectly blocking valid access attempts.
>
> Enable the VMA's pages to remain accessible while ensuring that
> the PTEs are installed to match the EPCM permissions but not be
> more relaxed than the VMA permissions.
>
> === Full Changelog ===
>
> An SGX enclave is an area of memory where parts of an application
> can reside. First an enclave is created and loaded (from
> non-enclave memory) with the code and data of an application,
> then user space can map (mmap()) the enclave memory to
> be able to enter the enclave at its defined entry points for
> execution within it.
>
> The hardware maintains a secure structure, the Enclave Page Cache Map
> (EPCM), that tracks the contents of the enclave. Of interest here is
> its tracking of the enclave page permissions. When a page is loaded
> into the enclave its permissions are specified and recorded in the
> EPCM. In parallel the kernel maintains permissions within the
> page table entries (PTEs) and the rule is that PTE permissions
> are not allowed to be more relaxed than the EPCM permissions.
>
> A new mapping (mmap()) of enclave memory can only succeed if the
> mapping has the same or weaker permissions than the permissions that
> were vetted during enclave creation. This is enforced by
> sgx_encl_may_map() that is called on the mmap() as well as mprotect()
> paths. This rule remains.
>
> One feature of SGX2 is to support the modification of EPCM permissions
> after enclave initialization. Enclave pages may thus already be part
> of a VMA at the time their EPCM permissions are changed resulting
> in the VMA's permissions potentially being more relaxed than the EPCM
> permissions.
>
> Allow permissions of existing VMAs to be more relaxed than EPCM
> permissions in preparation for dynamic EPCM permission changes
> made possible in SGX2. New VMAs that attempt to have more relaxed
> permissions than EPCM permissions continue to be unsupported.
>
> Reasons why permissions of existing VMAs are allowed to be more relaxed
> than EPCM permissions instead of dynamically changing VMA permissions
> when EPCM permissions change are:
> 1) Changing VMA permissions involve splitting VMAs which is an
> operation that can fail. Additionally changing EPCM permissions of
> a range of pages could also fail on any of the pages involved.
> Handling these error cases causes problems. For example, if an
> EPCM permission change fails and the VMA has already been split
> then it is not possible to undo the VMA split nor possible to
> undo the EPCM permission changes that did succeed before the
> failure.
> 2) The kernel has little insight into the user space where EPCM
> permissions are controlled from. For example, a RW page may
> be made RO just before it is made RX and splitting the VMAs
> while the VMAs may change soon is unnecessary.
>
> Remove the extra permission check called on a page fault
> (vm_operations_struct->fault) or during debugging
> (vm_operations_struct->access) when loading the enclave page from swap
> that ensures that the VMA permissions are not more relaxed than the
> EPCM permissions. Since a VMA could only exist if it passed the
> original permission checks during mmap() and a VMA may indeed
> have more relaxed permissions than the EPCM permissions this extra
> permission check is no longer appropriate.
>
> With the permission check removed, ensure that PTEs do
> not blindly inherit the VMA permissions but instead the permissions
> that the VMA and EPCM agree on. PTEs for writable pages (from VMA
> and enclave perspective) are installed with the writable bit set,
> reducing the need for this additional flow to the permission mismatch
> cases handled next.
>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V1:
> - Reword commit message (Jarkko).
> - Use "relax" instead of "exceed" when referring to permissions (Dave).
> - Add snippet to Documentation/x86/sgx.rst that highlights the
> relationship between VMA, EPCM, and PTE permissions on SGX
> systems (Andy).
>
> Documentation/x86/sgx.rst | 10 +++++++++
> arch/x86/kernel/cpu/sgx/encl.c | 38 ++++++++++++++++++----------------
> 2 files changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
> index 89ff924b1480..5659932728a5 100644
> --- a/Documentation/x86/sgx.rst
> +++ b/Documentation/x86/sgx.rst
> @@ -99,6 +99,16 @@ The relationships between the different permission masks are:
> * PTEs are installed to match the EPCM permissions, but not be more
> relaxed than the VMA permissions.
>
> +On systems supporting SGX2 EPCM permissions may change while the
> +enclave page belongs to a VMA without impacting the VMA permissions.
> +This means that a running VMA may appear to allow access to an enclave
> +page that is not allowed by its EPCM permissions. For example, when an
> +enclave page with RW EPCM permissions is mapped by a RW VMA but is
> +subsequently changed to have read-only EPCM permissions. The kernel
> +continues to maintain correct access to the enclave page through the
> +PTE that will ensure that only access allowed by both the VMA
> +and EPCM permissions are permitted.
> +
> Application interface
> =====================
>
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 48afe96ae0f0..b6105d9e7c46 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -91,10 +91,8 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page,
> }
>
> static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> - unsigned long addr,
> - unsigned long vm_flags)
> + unsigned long addr)
> {
> - unsigned long vm_prot_bits = vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
> struct sgx_epc_page *epc_page;
> struct sgx_encl_page *entry;
>
> @@ -102,14 +100,6 @@ static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> if (!entry)
> return ERR_PTR(-EFAULT);
>
> - /*
> - * Verify that the faulted page has equal or higher build time
> - * permissions than the VMA permissions (i.e. the subset of {VM_READ,
> - * VM_WRITE, VM_EXECUTE} in vma->vm_flags).
> - */
> - if ((entry->vm_max_prot_bits & vm_prot_bits) != vm_prot_bits)
> - return ERR_PTR(-EFAULT);
> -
> /* Entry successfully located. */
> if (entry->epc_page) {
> if (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED)
> @@ -138,7 +128,9 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
> {
> unsigned long addr = (unsigned long)vmf->address;
> struct vm_area_struct *vma = vmf->vma;
> + unsigned long page_prot_bits;
> struct sgx_encl_page *entry;
> + unsigned long vm_prot_bits;
> unsigned long phys_addr;
> struct sgx_encl *encl;
> vm_fault_t ret;
> @@ -155,7 +147,7 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
>
> mutex_lock(&encl->lock);
>
> - entry = sgx_encl_load_page(encl, addr, vma->vm_flags);
> + entry = sgx_encl_load_page(encl, addr);
> if (IS_ERR(entry)) {
> mutex_unlock(&encl->lock);
> @@ -167,7 +159,19 @@ static vm_fault_t sgx_vma_fault(struct vm_fault *vmf)
>
> phys_addr = sgx_get_epc_phys_addr(entry->epc_page);
>
> - ret = vmf_insert_pfn(vma, addr, PFN_DOWN(phys_addr));
> + /*
> + * Insert PTE to match the EPCM page permissions ensured to not
> + * exceed the VMA permissions.
> + */
> + vm_prot_bits = vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
> + page_prot_bits = entry->vm_max_prot_bits & vm_prot_bits;
> + /*
> + * Add VM_SHARED so that PTE is made writable right away if VMA
> + * and EPCM are writable (no COW in SGX).
> + */
> + page_prot_bits |= (vma->vm_flags & VM_SHARED);
> + ret = vmf_insert_pfn_prot(vma, addr, PFN_DOWN(phys_addr),
> + vm_get_page_prot(page_prot_bits));
> if (ret != VM_FAULT_NOPAGE) {
> mutex_unlock(&encl->lock);
>
> @@ -295,15 +299,14 @@ static int sgx_encl_debug_write(struct sgx_encl *encl, struct sgx_encl_page *pag
> * Load an enclave page to EPC if required, and take encl->lock.
> */
> static struct sgx_encl_page *sgx_encl_reserve_page(struct sgx_encl *encl,
> - unsigned long addr,
> - unsigned long vm_flags)
> + unsigned long addr)
> {
> struct sgx_encl_page *entry;
>
> for ( ; ; ) {
> mutex_lock(&encl->lock);
>
> - entry = sgx_encl_load_page(encl, addr, vm_flags);
> + entry = sgx_encl_load_page(encl, addr);
> if (PTR_ERR(entry) != -EBUSY)
> break;
>
> @@ -339,8 +342,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
> return -EFAULT;
>
> for (i = 0; i < len; i += cnt) {
> - entry = sgx_encl_reserve_page(encl, (addr + i) & PAGE_MASK,
> - vma->vm_flags);
> + entry = sgx_encl_reserve_page(encl, (addr + i) & PAGE_MASK);
> if (IS_ERR(entry)) {
> ret = PTR_ERR(entry);
> break;
> --
> 2.25.1
>
If you unconditionally set vm_max_prot_bits to RWX for dynamically created
pags, you would not need to do this.
These patches could be then safely dropped then:
- [PATCH V2 06/32] x86/sgx: Support VMA permissions more relaxed than enclave permissions
- [PATCH V2 08/32] x86/sgx: x86/sgx: Add sgx_encl_page->vm_run_prot_bits for dynamic permission changes
- [PATCH V2 15/32] x86/sgx: Support relaxing of enclave page permissions
And that would also keep full ABI compatibility without exceptions to the
existing mainline code.
BR, Jarkko
next prev parent reply other threads:[~2022-03-07 17:10 UTC|newest]
Thread overview: 130+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-08 0:45 [PATCH V2 00/32] x86/sgx and selftests/sgx: Support SGX2 Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 01/32] x86/sgx: Add short descriptions to ENCLS wrappers Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 02/32] x86/sgx: Add wrapper for SGX2 EMODPR function Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 03/32] x86/sgx: Add wrapper for SGX2 EMODT function Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 04/32] x86/sgx: Add wrapper for SGX2 EAUG function Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 05/32] Documentation/x86: Document SGX permission details Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 06/32] x86/sgx: Support VMA permissions more relaxed than enclave permissions Reinette Chatre
2022-03-07 17:10 ` Jarkko Sakkinen [this message]
2022-03-07 17:36 ` Reinette Chatre
2022-03-08 8:14 ` Jarkko Sakkinen
2022-03-08 9:06 ` Jarkko Sakkinen
2022-03-08 9:12 ` Jarkko Sakkinen
2022-03-08 16:04 ` Reinette Chatre
2022-03-08 17:00 ` Jarkko Sakkinen
2022-03-08 17:49 ` Reinette Chatre
2022-03-08 18:46 ` Jarkko Sakkinen
2022-03-11 11:06 ` Dr. Greg
2022-02-08 0:45 ` [PATCH V2 07/32] x86/sgx: Add pfn_mkwrite() handler for present PTEs Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 08/32] x86/sgx: x86/sgx: Add sgx_encl_page->vm_run_prot_bits for dynamic permission changes Reinette Chatre
2022-03-04 8:55 ` Jarkko Sakkinen
2022-03-04 19:19 ` Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 09/32] x86/sgx: Export sgx_encl_ewb_cpumask() Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 10/32] x86/sgx: Rename sgx_encl_ewb_cpumask() as sgx_encl_cpumask() Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 11/32] x86/sgx: Move PTE zap code to new sgx_zap_enclave_ptes() Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 12/32] x86/sgx: Make sgx_ipi_cb() available internally Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 13/32] x86/sgx: Create utility to validate user provided offset and length Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 14/32] x86/sgx: Keep record of SGX page type Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 15/32] x86/sgx: Support relaxing of enclave page permissions Reinette Chatre
2022-03-04 8:59 ` Jarkko Sakkinen
2022-02-08 0:45 ` [PATCH V2 16/32] x86/sgx: Support restricting " Reinette Chatre
2022-02-21 0:49 ` Jarkko Sakkinen
2022-02-22 18:35 ` Reinette Chatre
2022-02-23 15:46 ` Jarkko Sakkinen
2022-02-23 19:55 ` Reinette Chatre
2022-02-28 12:27 ` Jarkko Sakkinen
2022-02-23 19:21 ` Dhanraj, Vijay
2022-02-23 22:42 ` Reinette Chatre
2022-02-28 12:24 ` Jarkko Sakkinen
2022-02-28 13:19 ` Jarkko Sakkinen
2022-02-28 15:16 ` Dave Hansen
2022-02-28 17:44 ` Dhanraj, Vijay
2022-03-01 13:26 ` Jarkko Sakkinen
2022-03-01 13:42 ` Jarkko Sakkinen
2022-03-01 17:48 ` Reinette Chatre
2022-03-02 2:05 ` Jarkko Sakkinen
2022-03-02 2:11 ` Jarkko Sakkinen
2022-03-02 4:03 ` Jarkko Sakkinen
2022-03-02 22:57 ` Reinette Chatre
2022-03-03 16:08 ` Haitao Huang
2022-03-03 21:23 ` Reinette Chatre
2022-03-03 21:44 ` Dave Hansen
2022-03-05 3:19 ` Jarkko Sakkinen
2022-03-06 0:15 ` Jarkko Sakkinen
2022-03-06 0:25 ` Jarkko Sakkinen
2022-03-10 5:43 ` Jarkko Sakkinen
2022-03-10 5:59 ` Jarkko Sakkinen
2022-03-03 23:18 ` Jarkko Sakkinen
2022-03-04 4:03 ` Haitao Huang
2022-03-04 8:30 ` Jarkko Sakkinen
2022-03-04 15:51 ` Haitao Huang
2022-03-05 1:02 ` Jarkko Sakkinen
2022-03-06 14:24 ` Haitao Huang
2022-03-03 23:12 ` Jarkko Sakkinen
2022-03-04 0:48 ` Reinette Chatre
2022-03-10 6:10 ` Jarkko Sakkinen
2022-03-10 18:33 ` Haitao Huang
2022-03-11 12:10 ` Jarkko Sakkinen
2022-03-11 12:16 ` Jarkko Sakkinen
2022-03-11 12:33 ` Jarkko Sakkinen
2022-03-11 17:53 ` Reinette Chatre
2022-03-11 18:11 ` Jarkko Sakkinen
2022-03-11 19:28 ` Reinette Chatre
2022-03-14 3:42 ` Jarkko Sakkinen
2022-03-14 3:45 ` Jarkko Sakkinen
2022-03-14 3:54 ` Jarkko Sakkinen
2022-03-14 15:32 ` Reinette Chatre
2022-03-17 4:30 ` Jarkko Sakkinen
2022-03-17 22:08 ` Reinette Chatre
2022-03-17 22:51 ` Jarkko Sakkinen
2022-03-18 0:11 ` Reinette Chatre
2022-03-20 0:24 ` Jarkko Sakkinen
2022-03-28 23:22 ` Reinette Chatre
2022-03-30 15:00 ` Jarkko Sakkinen
2022-03-30 15:02 ` Jarkko Sakkinen
2022-03-14 2:49 ` Jarkko Sakkinen
2022-03-14 2:50 ` Jarkko Sakkinen
2022-03-14 2:58 ` Jarkko Sakkinen
2022-03-14 15:39 ` Haitao Huang
2022-03-17 4:34 ` Jarkko Sakkinen
2022-03-17 14:42 ` Haitao Huang
2022-03-17 4:37 ` Jarkko Sakkinen
2022-03-17 14:47 ` Haitao Huang
2022-03-17 7:01 ` Jarkko Sakkinen
2022-03-17 7:11 ` Jarkko Sakkinen
2022-03-17 14:28 ` Haitao Huang
2022-03-17 21:50 ` Jarkko Sakkinen
2022-03-17 22:00 ` Jarkko Sakkinen
2022-03-17 22:23 ` Jarkko Sakkinen
2022-02-08 0:45 ` [PATCH V2 17/32] selftests/sgx: Add test for EPCM permission changes Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 18/32] selftests/sgx: Add test for TCS page " Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 19/32] x86/sgx: Support adding of pages to an initialized enclave Reinette Chatre
2022-02-19 11:57 ` Jarkko Sakkinen
2022-02-19 12:01 ` Jarkko Sakkinen
2022-02-20 18:40 ` Jarkko Sakkinen
2022-02-22 19:19 ` Reinette Chatre
2022-02-23 15:46 ` Jarkko Sakkinen
2022-03-07 16:16 ` Jarkko Sakkinen
2022-02-08 0:45 ` [PATCH V2 20/32] x86/sgx: Tighten accessible memory range after enclave initialization Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 21/32] selftests/sgx: Test two different SGX2 EAUG flows Reinette Chatre
2022-03-07 16:39 ` Jarkko Sakkinen
2022-02-08 0:45 ` [PATCH V2 22/32] x86/sgx: Support modifying SGX page type Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 23/32] x86/sgx: Support complete page removal Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 24/32] Documentation/x86: Introduce enclave runtime management section Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 25/32] selftests/sgx: Introduce dynamic entry point Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 26/32] selftests/sgx: Introduce TCS initialization enclave operation Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 27/32] selftests/sgx: Test complete changing of page type flow Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 28/32] selftests/sgx: Test faulty enclave behavior Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 29/32] selftests/sgx: Test invalid access to removed enclave page Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 30/32] selftests/sgx: Test reclaiming of untouched page Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 31/32] x86/sgx: Free up EPC pages directly to support large page ranges Reinette Chatre
2022-02-08 0:45 ` [PATCH V2 32/32] selftests/sgx: Page removal stress test Reinette Chatre
2022-02-22 20:27 ` [PATCH V2 00/32] x86/sgx and selftests/sgx: Support SGX2 Nathaniel McCallum
2022-02-22 22:39 ` Reinette Chatre
2022-02-23 13:24 ` Nathaniel McCallum
2022-02-23 18:25 ` Reinette Chatre
2022-03-02 16:57 ` Nathaniel McCallum
2022-03-02 21:20 ` Reinette Chatre
2022-03-03 1:13 ` Nathaniel McCallum
2022-03-03 17:49 ` Reinette Chatre
2022-03-04 0:57 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YiY8bD/QcmcHH8dz@iki.fi \
--to=jarkko@kernel.org \
--cc=bp@alien8.de \
--cc=cathy.zhang@intel.com \
--cc=cedric.xing@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=haitao.huang@intel.com \
--cc=hpa@zytor.com \
--cc=kai.huang@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mark.shanahan@intel.com \
--cc=mingo@redhat.com \
--cc=reinette.chatre@intel.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).