Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org,
	Shay Katz-zamir <shay.katz-zamir@intel.com>,
	Serge Ayoun <serge.ayoun@intel.com>
Subject: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held
Date: Wed,  7 Aug 2019 17:12:53 -0700
Message-ID: <20190808001254.11926-11-sean.j.christopherson@intel.com> (raw)
In-Reply-To: <20190808001254.11926-1-sean.j.christopherson@intel.com>

Move the taking of the enclave's lock outside of sgx_encl_grow() in
preparation for adding sgx_encl_shrink(), which will decrement the
number of enclave pages and free any allocated VA page.  When freeing
a VA page, the enclave's lock needs to be held for the entire time
between adding the VA page to the enclave's list and freeing the VA
page so as to prevent it from being used by reclaim, e.g. to avoid a
use-after-free scenario.

Because sgx_encl_grow() can temporarily drop encl->lock, calling it
with encl->lock held adds a subtle dependency on the ordering of
checks against encl->flags, e.g. checking for SGX_ENCL_CREATED prior
to calling sgx_encl_grow() could lead to a TOCTOU on ECREATE.  Avoid
this by passing in the disallowed flags to sgx_encl_grow() so that the
the dependency is clear.

Retaking encl->lock in the failure paths is a bit ugly, but the
alternative is to have sgx_encl_grow() drop encl->lock in all failure
paths, which is arguably worse since the caller has to know which
paths do/don't drop the lock.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 39 +++++++++-----------------
 1 file changed, 13 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index fec5e0a346f5..a531cf615f3c 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -22,7 +22,7 @@ struct sgx_add_page_req {
 	struct list_head list;
 };
 
-static int sgx_encl_grow(struct sgx_encl *encl)
+static int sgx_encl_grow(struct sgx_encl *encl, unsigned int disallowed_flags)
 {
 	struct sgx_va_page *va_page;
 	int ret;
@@ -30,30 +30,28 @@ static int sgx_encl_grow(struct sgx_encl *encl)
 	BUILD_BUG_ON(SGX_VA_SLOT_COUNT !=
 		(SGX_ENCL_PAGE_VA_OFFSET_MASK >> 3) + 1);
 
-	mutex_lock(&encl->lock);
-	if (encl->flags & SGX_ENCL_DEAD) {
-		mutex_unlock(&encl->lock);
+	if (encl->flags & disallowed_flags)
 		return -EFAULT;
-	}
 
 	if (!(encl->page_cnt % SGX_VA_SLOT_COUNT)) {
 		mutex_unlock(&encl->lock);
 
 		va_page = kzalloc(sizeof(*va_page), GFP_KERNEL);
-		if (!va_page)
+		if (!va_page) {
+			mutex_lock(&encl->lock);
 			return -ENOMEM;
+		}
+
 		va_page->epc_page = sgx_alloc_va_page();
+		mutex_lock(&encl->lock);
+
 		if (IS_ERR(va_page->epc_page)) {
 			ret = PTR_ERR(va_page->epc_page);
 			kfree(va_page);
 			return ret;
-		}
-
-		mutex_lock(&encl->lock);
-		if (encl->flags & SGX_ENCL_DEAD) {
+		} else if (encl->flags & disallowed_flags) {
 			sgx_free_page(va_page->epc_page);
 			kfree(va_page);
-			mutex_unlock(&encl->lock);
 			return -EFAULT;
 		} else if (encl->page_cnt % SGX_VA_SLOT_COUNT) {
 			sgx_free_page(va_page->epc_page);
@@ -63,7 +61,6 @@ static int sgx_encl_grow(struct sgx_encl *encl)
 		}
 	}
 	encl->page_cnt++;
-	mutex_unlock(&encl->lock);
 	return 0;
 }
 
@@ -269,16 +266,11 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	struct file *backing;
 	long ret;
 
-	ret = sgx_encl_grow(encl);
-	if (ret)
-		return ret;
-
 	mutex_lock(&encl->lock);
 
-	if (encl->flags & SGX_ENCL_CREATED) {
-		ret = -EFAULT;
+	ret = sgx_encl_grow(encl, SGX_ENCL_CREATED | SGX_ENCL_DEAD);
+	if (ret)
 		goto err_out_unlock;
-	}
 
 	ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
 	if (sgx_validate_secs(secs, ssaframesize)) {
@@ -514,16 +506,11 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 			return ret;
 	}
 
-	ret = sgx_encl_grow(encl);
-	if (ret)
-		return ret;
-
 	mutex_lock(&encl->lock);
 
-	if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
-		ret = -EFAULT;
+	ret = sgx_encl_grow(encl, SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD);
+	if (ret)
 		goto err_out_unlock;
-	}
 
 	encl_page = sgx_encl_page_alloc(encl, addr, prot);
 	if (IS_ERR(encl_page)) {
-- 
2.22.0


  parent reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
2019-08-08  0:12 ` [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0 Sean Christopherson
2019-08-08 15:34   ` Jarkko Sakkinen
2019-08-08 15:44     ` Sean Christopherson
2019-08-09 15:13       ` Jarkko Sakkinen
2019-08-09 20:44   ` Jarkko Sakkinen
2019-08-09 20:59     ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check Sean Christopherson
2019-08-08 15:36   ` Jarkko Sakkinen
2019-08-09 21:16     ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 03/11] x86/sgx: Return '0' when sgx_ioc_enclave_set_attribute() succeeds Sean Christopherson
2019-08-08 15:37   ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 04/11] x86/sgx: x86/sgx: Require EADD destination to be page aligned Sean Christopherson
2019-08-08 15:38   ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 05/11] x86/sgx: Require EADD source " Sean Christopherson
2019-08-08 15:44   ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 06/11] x86/sgx: Check the bounds of the enclave address against ELRANGE Sean Christopherson
2019-08-08 15:45   ` Jarkko Sakkinen
2019-08-09 21:21     ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl Sean Christopherson
2019-08-08 15:47   ` Jarkko Sakkinen
2019-08-09 23:40   ` Jarkko Sakkinen
2019-08-10  0:03     ` Sean Christopherson
2019-08-10  0:10       ` Sean Christopherson
2019-08-08  0:12 ` [PATCH for_v22 08/11] x86/sgx: Do not free enclave resources on redundant ECREATE Sean Christopherson
2019-08-08 15:48   ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 09/11] x86/sgx: Refactor error handling for user of sgx_encl_grow() Sean Christopherson
2019-08-08 15:49   ` Jarkko Sakkinen
2019-08-08  0:12 ` Sean Christopherson [this message]
2019-08-08 15:52   ` [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held Jarkko Sakkinen
2019-08-08 15:55     ` Sean Christopherson
2019-08-09 16:12       ` Jarkko Sakkinen
2019-08-10 11:32   ` Jarkko Sakkinen
2019-08-08  0:12 ` [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails Sean Christopherson
2019-08-08 15:50   ` Jarkko Sakkinen
2019-08-08 18:03     ` Sean Christopherson
2019-08-09 16:13       ` Jarkko Sakkinen
2019-08-10 11:37       ` Jarkko Sakkinen
2019-08-08 15:18 ` [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Jarkko Sakkinen
2019-08-08 15:57 ` Jarkko Sakkinen
2019-08-10 11:44 ` Jarkko Sakkinen

Reply instructions:

You may reply publically 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=20190808001254.11926-11-sean.j.christopherson@intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=linux-sgx@vger.kernel.org \
    --cc=serge.ayoun@intel.com \
    --cc=shay.katz-zamir@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

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 linux-sgx@archiver.kernel.org
	public-inbox-index linux-sgx


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