[for_v22,10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held
diff mbox series

Message ID 20190808001254.11926-11-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/sgx: Bug fixes for v22
Related show

Commit Message

Sean Christopherson Aug. 8, 2019, 12:12 a.m. UTC
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(-)

Comments

Jarkko Sakkinen Aug. 8, 2019, 3:52 p.m. UTC | #1
On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote:
> 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>

Would be cleaner to check the flags just before the call. Otherwise,
no problems with this.

/Jarkko
Sean Christopherson Aug. 8, 2019, 3:55 p.m. UTC | #2
On Thu, Aug 08, 2019 at 06:52:29PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote:
> > 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>
> 
> Would be cleaner to check the flags just before the call. Otherwise,
> no problems with this.

That's not sufficient in the case that sgx_encl_grow() drops encl->lock
to allocate an EPC page, as the flags need to be rechecked after the
lock is reacquired.  I'm not a huge fan of the code either, but it was
the least ugly solution I could come up with and kinda fit since
sgx_encl_grow() was already checking SGX_ENCL_DEAD.
Jarkko Sakkinen Aug. 9, 2019, 4:12 p.m. UTC | #3
On Thu, 2019-08-08 at 08:55 -0700, Sean Christopherson wrote:
> On Thu, Aug 08, 2019 at 06:52:29PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote:
> > > 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>
> > 
> > Would be cleaner to check the flags just before the call. Otherwise,
> > no problems with this.
> 
> That's not sufficient in the case that sgx_encl_grow() drops encl->lock
> to allocate an EPC page, as the flags need to be rechecked after the
> lock is reacquired.  I'm not a huge fan of the code either, but it was
> the least ugly solution I could come up with and kinda fit since
> sgx_encl_grow() was already checking SGX_ENCL_DEAD.

Right, I was too hazy. We'll go what you proposed.

/Jarkko
Jarkko Sakkinen Aug. 10, 2019, 11:32 a.m. UTC | #4
On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote:
> 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>

Merged.

/Jarkko

Patch
diff mbox series

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)) {