linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: linux-sgx@vger.kernel.org
Cc: kai.svahn@intel.com, bruce.schlobohm@intel.com,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	luto@kernel.org, Stephen Smalley <sds@tycho.nsa.gov>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>
Subject: [PATCH 1/4] x86/sgx: Remove PROT_NONE branch from sgx_encl_may_map().
Date: Tue, 31 Mar 2020 14:44:29 +0300	[thread overview]
Message-ID: <20200331114432.7593-2-jarkko.sakkinen@linux.intel.com> (raw)
In-Reply-To: <20200331114432.7593-1-jarkko.sakkinen@linux.intel.com>

sgx_encl_may_map() always succeeding when PROT_NONE is given is not that
useful behaviour as one can just well as do an anonymous mapping as
demonstrated by the change in this patch to the test program. As a
consequence, remove the special case.

Pratically any possible way to make sure that you don't overwrite anything
useful in the memory, should be fine. MAP_FIXED does not care what's
underneath (if you want't it to care you ought to use
MAP_FIXED_NO_REPLACE).

After this change, the selftest run called sgx_mmap() only three times
(TCS, text, data) instead of four.

        test_sgx-1811  [002] ....   586.907585: sgx_mmap <-mmap_region
        test_sgx-1811  [002] ....   586.911752: sgx_mmap <-mmap_region
        test_sgx-1811  [002] ....   586.911756: sgx_mmap <-mmap_region

This also gives more angles to segregate enclave building and mapping as
the mmap()'s need to be applied only when the enclave is fully built:

Cc: luto@kernel.org
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Haitao Huang <haitao.huang@linux.intel.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 Documentation/x86/sgx.rst          | 13 +++++--------
 arch/x86/kernel/cpu/sgx/encl.c     |  7 +------
 arch/x86/kernel/cpu/sgx/ioctl.c    |  5 -----
 tools/testing/selftests/sgx/load.c |  3 ++-
 4 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
index 79de1f01aa5b..9609a3409ad1 100644
--- a/Documentation/x86/sgx.rst
+++ b/Documentation/x86/sgx.rst
@@ -168,14 +168,11 @@ accounted to the processes of which behalf the kernel happened to be acting on.
 Access control
 ==============
 
-`mmap()` permissions are capped by the enclave permissions. The consequences of
-this is are:
-
-1. Pages for a VMA must built before `mmap()` can be applied with the
-   permissions required for running the enclave.
-2. An address range for an enclave can be reserved with a `PROT_NONE` mapping
-   (not required but usually makes sense to guarantee that the used address
-   range is available).
+`mmap()` permissions are capped by the enclave permissions. A direct
+consequence of this is that all the pages for an address range must be added
+before `mmap()` can be applied. Effectively an enclave page with minimum
+permission in the address range sets the permission cap for the mapping
+operation.
 
 Usage Models
 ============
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index d6a19bdd1921..e0124a2f22d5 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -288,8 +288,7 @@ static unsigned int sgx_vma_fault(struct vm_fault *vmf)
  *
  * Iterate through the enclave pages contained within [@start, @end) to verify
  * the permissions requested by @vm_prot_bits do not exceed that of any enclave
- * page to be mapped.  Page addresses that do not have an associated enclave
- * page are interpreted to zero permissions.
+ * page to be mapped.
  *
  * Return:
  *   0 on success,
@@ -308,10 +307,6 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
 	if (!!(current->personality & READ_IMPLIES_EXEC))
 		return -EACCES;
 
-	/* PROT_NONE always succeeds. */
-	if (!vm_prot_bits)
-		return 0;
-
 	idx_start = PFN_DOWN(start);
 	idx_end = PFN_DOWN(end - 1);
 
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 12e1496f8a8b..3af0596530a8 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -481,15 +481,10 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
  *
  * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO permissions.
  * 2. A TCS page: PROT_R | PROT_W.
- * 3. No page: PROT_NONE.
  *
  * mmap() is not allowed to surpass the minimum of the maximum protection bits
  * within the given address range.
  *
- * As stated above, a non-existent page is interpreted as a page with no
- * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek
- * an address range for the enclave that can be then populated into SECS.
- *
  * If ENCLS opcode fails, that effectively means that EPC has been invalidated.
  * When this happens the enclave is destroyed and -EIO is returned to the
  * caller.
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 35a2d7a47dd5..53c565347e9e 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -231,7 +231,8 @@ static bool encl_map_area(struct encl *encl)
 	size_t encl_size = encl->encl_size;
 	void *area;
 
-	area = mmap(NULL, encl_size * 2, PROT_NONE, MAP_SHARED, encl->fd, 0);
+	area = mmap(NULL, encl_size * 2, PROT_NONE,
+		    MAP_SHARED | MAP_ANONYMOUS, -1, 0);
 	if (area == MAP_FAILED) {
 		perror("mmap");
 		return false;
-- 
2.25.1


  reply	other threads:[~2020-03-31 11:44 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-31 11:44 [PATCH 0/4] Migrate enclave mapping to an anonymous inode Jarkko Sakkinen
2020-03-31 11:44 ` Jarkko Sakkinen [this message]
2020-03-31 11:44 ` [PATCH 2/4] x86/sgx: Put enclaves into anonymous files Jarkko Sakkinen
2020-03-31 17:39   ` Andy Lutomirski
2020-04-01  0:24     ` Sean Christopherson
2020-04-02 21:41       ` Andy Lutomirski
2020-04-03  6:56         ` Jarkko Sakkinen
2020-04-03  6:59           ` Jarkko Sakkinen
2020-04-03 14:35           ` Casey Schaufler
2020-04-03 15:30             ` Jarkko Sakkinen
2020-04-03 15:50               ` Casey Schaufler
2020-04-03 22:08                 ` Jarkko Sakkinen
2020-04-04  3:54                   ` Andy Lutomirski
2020-04-04  5:46                     ` Jethro Beekman
2020-04-04  7:27                       ` Topi Miettinen
2020-04-04  9:20                         ` Jarkko Sakkinen
2020-04-06  6:42                         ` Jethro Beekman
2020-04-06 11:01                           ` Topi Miettinen
2020-04-06 16:44                             ` Andy Lutomirski
2020-04-06 17:17                               ` Jethro Beekman
2020-04-06 18:55                               ` Jarkko Sakkinen
2020-04-06 19:01                                 ` Jarkko Sakkinen
2020-04-06 19:53                                 ` Andy Lutomirski
2020-04-06 21:24                                   ` Jarkko Sakkinen
2020-04-06 23:18                                     ` Andy Lutomirski
2020-04-06 23:48                                       ` Jarkko Sakkinen
2020-04-07  7:15                                       ` Jethro Beekman
2020-04-07  8:48                                     ` Topi Miettinen
2020-04-07 16:52                                       ` Jarkko Sakkinen
2020-04-07  9:04                                     ` Topi Miettinen
2020-04-07 16:57                                       ` Jarkko Sakkinen
2020-04-07 16:59                                         ` Jarkko Sakkinen
2020-04-07 18:04                                           ` Jarkko Sakkinen
2020-04-07 19:54                                             ` Topi Miettinen
2020-04-08 13:40                                               ` Jarkko Sakkinen
2020-04-08 14:56                                                 ` Sean Christopherson
2020-04-09 18:39                                                   ` Jarkko Sakkinen
2020-04-08 21:15                                                 ` Topi Miettinen
2020-04-08 21:29                                                   ` Sean Christopherson
2020-11-19  7:23                                   ` Jethro Beekman
2020-11-19 16:09                                     ` Andy Lutomirski
2020-04-06 18:47                             ` Jarkko Sakkinen
2020-04-04  9:22                     ` Jarkko Sakkinen
2020-04-01  8:45     ` Jarkko Sakkinen
2020-03-31 11:44 ` [PATCH 3/4] x86/sgx: Move mmap() to the anonymous enclave file Jarkko Sakkinen
2020-03-31 11:44 ` [PATCH 4/4] x86/sgx: Hand over the enclave file to the user space 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=20200331114432.7593-2-jarkko.sakkinen@linux.intel.com \
    --to=jarkko.sakkinen@linux.intel.com \
    --cc=bruce.schlobohm@intel.com \
    --cc=casey@schaufler-ca.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=sds@tycho.nsa.gov \
    --cc=sean.j.christopherson@intel.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
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).