[PATCH] x86/sgx: Remove PROT_NONE branch from sgx_encl_may_map().

* [PATCH] x86/sgx: Remove PROT_NONE branch from sgx_encl_may_map().
@ 2020-03-30 18:54 Jarkko Sakkinen
  2020-03-30 21:13 ` Jarkko Sakkinen
  0 siblings, 1 reply; 4+ messages in thread
From: Jarkko Sakkinen @ 2020-03-30 18:54 UTC (permalink / raw)
  To: linux-sgx; +Cc: Jarkko Sakkinen, luto, Haitao Huang, Sean Christopherson

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: 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


^ permalink raw reply related	[flat|nested] 4+ messages in thread