Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: "Dr. Greg" <greg@enjellic.com>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: x86@kernel.org, linux-sgx@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	Matthew Wilcox <willy@infradead.org>,
	Mel Gorman <mgorman@techsingularity.net>,
	Jethro Beekman <jethro@fortanix.com>,
	Dave Hansen <dave.hansen@intel.com>,
	andriy.shevchenko@linux.intel.com, asapek@google.com,
	bp@alien8.de, cedric.xing@intel.com, chenalexchen@google.com,
	conradparker@google.com, cyhanish@google.com,
	haitao.huang@intel.com, kai.huang@intel.com, kai.svahn@intel.com,
	kmoy@google.com, ludloff@google.com, luto@kernel.org,
	nhorman@redhat.com, npmccallum@redhat.com, puiterwijk@redhat.com,
	rientjes@google.com, tglx@linutronix.de, yaozhangx@google.com,
	mikko.ylinen@intel.com
Subject: Re: [PATCH v41 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct
Date: Sun, 15 Nov 2020 11:08:53 -0600
Message-ID: <20201115170853.GA26176@wind.enjellic.com> (raw)
In-Reply-To: <20201112220135.165028-11-jarkko@kernel.org>

On Fri, Nov 13, 2020 at 12:01:21AM +0200, Jarkko Sakkinen wrote:

Good morning, I hope the weekend is going well for everyone.

> From: Sean Christopherson <sean.j.christopherson@intel.com>

We wish Sean well in whatever new avocation he has chosen.

> Background
> ==========
> 
> 1. SGX enclave pages are populated with data by copying from normal memory
>    via ioctl() (SGX_IOC_ENCLAVE_ADD_PAGES), which will be added later in
>    this series.
> 2. It is desirable to be able to restrict those normal memory data sources.
>    For instance, to ensure that the source data is executable before
>    copying data to an executable enclave page.
> 3. Enclave page permissions are dynamic (just like normal permissions) and
>    can be adjusted at runtime with mprotect().
>
> This creates a problem because the original data source may have long since
> vanished at the time when enclave page permissions are established (mmap()
> or mprotect()).
> 
> The solution (elsewhere in this series) is to force enclaves creators to
> declare their paging permission *intent* up front to the ioctl().  This
> intent can be immediately compared to the source data???s mapping and
> rejected if necessary.
> 
> The ???intent??? is also stashed off for later comparison with enclave
> PTEs. This ensures that any future mmap()/mprotect() operations
> performed by the enclave creator or done on behalf of the enclave
> can be compared with the earlier declared permissions.

The new mprotect hook in vm_operations_struct is indeed useful, as I
will demonstrate in a subsequent patch for consideration.  However,
the officially stated intent of this version of the driver is to
implement SGX1 semantics even on hardware (SGX2) that implements the
instructions needed for Enclave Dynamic Memory Management (EDMM).

As a result, at this stage of the driver's development, the
implementation that walks the page permission intents can be
substituted with a simple denial of mmap and mprotect on an
initialized enclave.  With this prohibition in place, the hardware
itself will enforce the page permission intents that were encoded when
the enclave was loaded, thus making the subsequent scan irrelevant.

The following patch implements this functionality.

Dr. Greg

---------------------------------------------------------------------------
Subject: [PATCH] Unconditionally block permission changes on enclave memory.

In SGX there are two levels of memory protection, the classic
page table mechanism and SGX hardware based page protections
that are codified in the Enclave Page Cache Metadata.  A
successful memory access requires that both mechanisms agree
that the access is permitted.

In the initial implementation of SGX (SGX1), the page permissions
are immutable after the enclave is initialized.  Even if classic
page protections are modified via mprotect, any attempt to access
enclave memory with alternative permissions will be blocked.

One of the architectural changes implemented in the second
generation of SGX (SGX2) is the ability for page access
permissions to be dynamically manipulated after the enclave is
initialized.  This requires coordination between trusted code
running in the enclave and untrusted code using mprotect and
special ring-0 instructions.

One of the security threats associated with SGX2 hardware is that
enclave based code can conspire with its untrusted runtime to make
executable enclave memory writable.  This provides the opportunity for
completely anonymous code execution that the operating system has no
visibility into.

All that is needed to, simply, close this vulnerability is to
eliminate the ability to call mprotect or mmap against the virtual
memory range of an enclave after it is initialized.  Any
permission changes made prior to initialization that are inconsistent
with the permissions codified in the enclave will cause initialization
or execution of the enclave to fail.

Tested-by: Dr. Greg <greg@enjellic.com>
Signed-off-by: Dr. Greg <greg@enjellic.com>
---
 arch/x86/kernel/cpu/sgx/encl.c | 50 +++++++++-------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 5551c7d36483..3bd770fbfc32 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -212,27 +212,25 @@ static void sgx_vma_open(struct vm_area_struct *vma)
  * @end:		upper bound of the address range, exclusive
  * @vm_flags:		VMA flags
  *
- * Iterate through the enclave pages contained within [@start, @end) to verify
- * that the permissions requested by a subset of {VM_READ, VM_WRITE, VM_EXEC}
- * does not contain any permissions that are not contained in the build time
- * permissions of any of the enclave pages within the given address range.
+ * This function provides a method for determining whether or not mmap
+ * or mprotect can be invoked called on any pages in the virtual
+ * address range of an enclave.
  *
- * An enclave creator must declare the strongest permissions that will be
- * needed for each enclave page  This ensures that mappings  have the identical
- * or weaker permissions that the earlier declared permissions.
+ * Since this driver is not designed to support Enclave Dynamic Memory
+ * Management (EDMM), any attempts to modify enclave memory map after
+ * the enclave is initialized are simply blocked.
+ *
+ * The function signature is left intact since future versions of the
+ * driver may implement verifications that the requested permission
+ * changes are consistent with the desire of the enclave author.
  *
  * Return: 0 on success, -EACCES otherwise
  */
 int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
 		     unsigned long end, unsigned long vm_flags)
 {
-	unsigned long vm_prot_bits = vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
-	struct sgx_encl_page *page;
-	unsigned long count = 0;
 	int ret = 0;
 
-	XA_STATE(xas, &encl->page_array, PFN_DOWN(start));
-
 	/*
 	 * Disallow READ_IMPLIES_EXEC tasks as their VMA permissions might
 	 * conflict with the enclave page permissions.
@@ -240,31 +238,9 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
 	if (current->personality & READ_IMPLIES_EXEC)
 		return -EACCES;
 
-	mutex_lock(&encl->lock);
-	xas_lock(&xas);
-	xas_for_each(&xas, page, PFN_DOWN(end - 1)) {
-		if (!page)
-			break;
-
-		if (~page->vm_max_prot_bits & vm_prot_bits) {
-			ret = -EACCES;
-			break;
-		}
-
-		/* Reschedule on every XA_CHECK_SCHED iteration. */
-		if (!(++count % XA_CHECK_SCHED)) {
-			xas_pause(&xas);
-			xas_unlock(&xas);
-			mutex_unlock(&encl->lock);
-
-			cond_resched();
-
-			mutex_lock(&encl->lock);
-			xas_lock(&xas);
-		}
-	}
-	xas_unlock(&xas);
-	mutex_unlock(&encl->lock);
+	/* Disallow mapping on an initialized enclave. */
+	if (test_bit(SGX_ENCL_INITIALIZED, &encl->flags))
+		ret = -EACCES;
 
 	return ret;
 }
-- 
2.19.2

---------------------------------------------------------------------------

As always,
Dr. Greg Wettstein, Ph.D, Worker      Autonomously self-defensive
Enjellic Systems Development, LLC     IOT platforms and edge devices.
4206 N. 19th Ave.
Fargo, ND  58102
PH: 701-281-1686                      EMAIL: dg@enjellic.com
------------------------------------------------------------------------------
"For a successful technology, reality must take precedence over public
 relations, for nature cannot be fooled."
                                -- Richard Feynmann

  parent reply index

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-12 22:01 [PATCH v41 00/24] Intel SGX foundations Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 01/24] x86/sgx: Add SGX architectural data structures Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 02/24] x86/sgx: Add wrappers for ENCLS functions Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 03/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 04/24] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 05/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen
2020-11-16 22:25   ` [PATCH] x86/sgx: clarify 'laundry_list' locking Dave Hansen
2020-11-17 19:29     ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 06/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 07/24] x86/cpu/intel: Detect SGX support Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 08/24] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 09/24] x86/sgx: Add SGX page allocator functions Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-11-13 10:25   ` Mel Gorman
2020-11-17 18:16     ` Jarkko Sakkinen
2020-11-15 17:08   ` Dr. Greg [this message]
2020-11-15 17:32   ` Matthew Wilcox
2020-11-15 18:36     ` Dave Hansen
2020-11-16 10:09       ` Mel Gorman
2020-11-17 19:15         ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 11/24] x86/sgx: Add SGX misc driver interface Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Jarkko Sakkinen
     [not found]   ` <20201115044044.11040-1-hdanton@sina.com>
2020-11-17 17:35     ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 16/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 17/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 18/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 19/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-11-17 13:14   ` Borislav Petkov
2020-11-17 19:41     ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 20/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-11-16 18:19   ` Shuah Khan
2020-11-17 13:22     ` Borislav Petkov
2020-11-17 19:42     ` Jarkko Sakkinen
2020-11-17 17:26   ` Borislav Petkov
2020-11-17 21:27     ` Jarkko Sakkinen
2020-11-17 21:38     ` Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 21/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 22/24] x86/sgx: Add ptrace() support for the SGX driver Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 23/24] docs: x86/sgx: Document SGX kernel architecture Jarkko Sakkinen
2020-11-12 22:01 ` [PATCH v41 24/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-11-16 16:55 ` [PATCH v41 00/24] Intel SGX foundations Borislav Petkov
2020-11-16 17:21   ` Dave Hansen
2020-11-16 17:28     ` Borislav Petkov
2020-11-17 19:20       ` Jarkko Sakkinen
     [not found] ` <20201114084211.5284-1-hdanton@sina.com>
2020-11-16 18:33   ` [PATCH v41 05/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Dave Hansen
     [not found] ` <20201115040127.7804-1-hdanton@sina.com>
2020-11-16 21:11   ` [PATCH v41 11/24] x86/sgx: Add SGX misc driver interface Dave Hansen
     [not found] ` <20201114090708.8684-1-hdanton@sina.com>
2020-11-17 18:12   ` [PATCH v41 06/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
     [not found] ` <20201114093256.7800-1-hdanton@sina.com>
2020-11-17 18:14   ` [PATCH v41 09/24] x86/sgx: Add SGX page allocator functions Jarkko Sakkinen
     [not found] ` <20201115030548.1572-1-hdanton@sina.com>
2020-11-17 18:22   ` [PATCH v41 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-12-15  5:38 ` [PATCH v41 00/24] Intel SGX foundations Hui, Chunyang
2020-12-15  5:43 ` Hui, Chunyang
2020-12-15 15:58   ` 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=20201115170853.GA26176@wind.enjellic.com \
    --to=greg@enjellic.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=asapek@google.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=chenalexchen@google.com \
    --cc=conradparker@google.com \
    --cc=cyhanish@google.com \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=jarkko@kernel.org \
    --cc=jethro@fortanix.com \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=kmoy@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=ludloff@google.com \
    --cc=luto@kernel.org \
    --cc=mgorman@techsingularity.net \
    --cc=mikko.ylinen@intel.com \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=tglx@linutronix.de \
    --cc=willy@infradead.org \
    --cc=x86@kernel.org \
    --cc=yaozhangx@google.com \
    /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

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org
	public-inbox-index linux-sgx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git