[PATCH for_v23 0/4] x86/sgx: Fix add page bugs

[PATCH for_v23 0/4] x86/sgx: Fix add page bugs

[Sean Christopherson]

Three bug fixes in the add page flow, along with a related enhancement.
Note, patch 4/4 is effectively a revert.

Sean Christopherson (4):
  x86/sgx: Pass EADD the kernel's virtual address for the source page
  x86/sgx: Check the validity of the source page address for EADD
  x86/sgx: Fix EEXTEND error handling
  x86/sgx: Drop mmap_sem before EEXTENDing an enclave page

 arch/x86/kernel/cpu/sgx/ioctl.c | 46 ++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 21 deletions(-)

-- 
2.22.0

[PATCH for_v23 1/4] x86/sgx: Pass EADD the kernel's virtual address for the source page

[Sean Christopherson]

Use the kernel's virtual address to reference the source page when
EADDing a page to the enclave.  gup() "does not guarantee that the page
exists in the user mappings", i.e. EADD can still fault and deadlock
due to mmap_sem contention if it consumes the userspace address.

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

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index eeed919ed3e2..62965d003fda 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -328,16 +328,14 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
 	if (ret < 1)
 		return ret;
 
-	__uaccess_begin();
-
 	pginfo.secs = (unsigned long)sgx_epc_addr(encl->secs.epc_page);
 	pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
 	pginfo.metadata = (unsigned long)secinfo;
-	pginfo.contents = (unsigned long)src;
+	pginfo.contents = (unsigned long)kmap_atomic(src_page);
 
 	ret = __eadd(&pginfo, sgx_epc_addr(epc_page));
 
-	__uaccess_end();
+	kunmap_atomic((void *)pginfo.contents);
 	put_page(src_page);
 
 	return ret ? -EFAULT : 0;
-- 
2.22.0

[PATCH for_v23 2/4] x86/sgx: Check the validity of the source page address for EADD

[Sean Christopherson]

Add an explicit access_ok() check on EADD's source page to avoid passing
garbage to gup().

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

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 62965d003fda..61a2a7b6677e 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -492,6 +492,9 @@ static long sgx_ioc_enclave_add_page(struct sgx_encl *encl, void __user *arg)
 	    !IS_ALIGNED(addp.src, PAGE_SIZE))
 		return -EINVAL;
 
+	if (!(access_ok(addp.src, PAGE_SIZE)))
+		return -EFAULT;
+
 	if (addp.addr < encl->base || addp.addr - encl->base >= encl->size)
 		return -EINVAL;
 
-- 
2.22.0

[PATCH for_v23 3/4] x86/sgx: Fix EEXTEND error handling

[Sean Christopherson]

Rework EEXTEND error handling to fix issues related to destroying the
enclave in response to EEXTEND failure.  At the time of EEXTEND, the
page is already visibile in the sense that it has been added to the
radix tree, and therefore will be processed by sgx_encl_destroy().  This
means the "add" part needs to be fully completed prior to invoking
sgx_encl_destroy() in order to avoid consuming half-baked state.

Move sgx_encl_destroy() to the call site of __sgx_encl_extend() so that
it is somewhat more obvious why the add needs to complete before doing
EEXTEND.

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

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 61a2a7b6677e..fd4117f18564 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -351,18 +351,14 @@ static int __sgx_encl_extend(struct sgx_encl *encl,
 	for_each_set_bit(i, &mrmask, 16) {
 		ret = __eextend(sgx_epc_addr(encl->secs.epc_page),
 				sgx_epc_addr(epc_page) + (i * 0x100));
-		if (ret)
-			goto err_out;
+		if (ret) {
+			if (encls_failed(ret))
+				ENCLS_WARN(ret, "EEXTEND");
+			return -EFAULT;
+		}
 	}
 
 	return 0;
-
-err_out:
-	if (encls_failed(ret))
-		ENCLS_WARN(ret, "EEXTEND");
-
-	sgx_encl_destroy(encl);
-	return -EFAULT;
 }
 
 static int sgx_encl_add_page(struct sgx_encl *encl,
@@ -415,19 +411,24 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 	if (ret)
 		goto err_out;
 
-	ret = __sgx_encl_extend(encl, epc_page, addp->mrmask);
-	if (ret)
-		goto err_out;
-
+	/*
+	 * Complete the "add" before doing the "extend" so that the "add"
+	 * isn't in a half-baked state in the extremely unlikely scenario the
+	 * the enclave will be destroyed in response to EEXTEND failure.
+	 */
 	encl_page->encl = encl;
 	encl_page->epc_page = epc_page;
 	encl->secs_child_cnt++;
 
-	sgx_mark_page_reclaimable(encl_page->epc_page);
+	ret = __sgx_encl_extend(encl, epc_page, addp->mrmask);
+	if (ret)
+		sgx_encl_destroy(encl);
+	else
+		sgx_mark_page_reclaimable(encl_page->epc_page);
 
 	mutex_unlock(&encl->lock);
 	up_read(&current->mm->mmap_sem);
-	return 0;
+	return ret;
 
 err_out:
 	radix_tree_delete(&encl_page->encl->page_tree,
-- 
2.22.0

[PATCH for_v23 4/4] x86/sgx: Drop mmap_sem before EEXTENDing an enclave page

[Sean Christopherson]

Drop mmap_sem, which needs to be held for read across EADD, prior to
doing EEXTEND on the newly added page to avoid holding mmap_sem for an
extended duration.  EEXTEND doesn't access user pages and holding
encl->lock without mmap_sem is perfectly ok, while EEXTEND is a _slow_
operation, to the point where it operates on 256-byte chunks
instead of 4k pages to maintain a reasonable latency for a single
instruction.

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

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index fd4117f18564..46f2769d16fe 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -403,11 +403,15 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 	 */
 	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
 				encl_page);
-	if (ret)
+	if (ret) {
+		up_read(&current->mm->mmap_sem);
 		goto err_out_unlock;
+	}
 
 	ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
 				  addp->src);
+	up_read(&current->mm->mmap_sem);
+
 	if (ret)
 		goto err_out;
 
@@ -427,7 +431,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 		sgx_mark_page_reclaimable(encl_page->epc_page);
 
 	mutex_unlock(&encl->lock);
-	up_read(&current->mm->mmap_sem);
 	return ret;
 
 err_out:
@@ -437,7 +440,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 err_out_unlock:
 	sgx_encl_shrink(encl, va_page);
 	mutex_unlock(&encl->lock);
-	up_read(&current->mm->mmap_sem);
 
 err_out_free:
 	sgx_free_page(epc_page);
-- 
2.22.0

Re: [PATCH for_v23 0/4] x86/sgx: Fix add page bugs

[Jarkko Sakkinen]

On Thu, Oct 10, 2019 at 03:55:26PM -0700, Sean Christopherson wrote:
> Three bug fixes in the add page flow, along with a related enhancement.
> Note, patch 4/4 is effectively a revert.
> 
> Sean Christopherson (4):
>   x86/sgx: Pass EADD the kernel's virtual address for the source page
>   x86/sgx: Check the validity of the source page address for EADD
>   x86/sgx: Fix EEXTEND error handling
>   x86/sgx: Drop mmap_sem before EEXTENDing an enclave page
> 
>  arch/x86/kernel/cpu/sgx/ioctl.c | 46 ++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 21 deletions(-)

So, which one should be merged first. This or v2 of the other series.

Can you send a single "for-v23 fixes" series with everything needed just
so that there is no room guess work? You can call v3 of the series that
you first started.

You already know that I applied 1/9 and 2/9 so please don't include
them.

/Jarkko

Re: [PATCH for_v23 0/4] x86/sgx: Fix add page bugs

[Sean Christopherson]

On Tue, Oct 15, 2019 at 12:03:17AM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 10, 2019 at 03:55:26PM -0700, Sean Christopherson wrote:
> > Three bug fixes in the add page flow, along with a related enhancement.
> > Note, patch 4/4 is effectively a revert.
> > 
> > Sean Christopherson (4):
> >   x86/sgx: Pass EADD the kernel's virtual address for the source page
> >   x86/sgx: Check the validity of the source page address for EADD
> >   x86/sgx: Fix EEXTEND error handling
> >   x86/sgx: Drop mmap_sem before EEXTENDing an enclave page
> > 
> >  arch/x86/kernel/cpu/sgx/ioctl.c | 46 ++++++++++++++++++---------------
> >  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> So, which one should be merged first. This or v2 of the other series.

Shouldn't matter.

> Can you send a single "for-v23 fixes" series with everything needed just
> so that there is no room guess work? You can call v3 of the series that
> you first started.

Will do, I'll get it sent out today.

Re: [PATCH for_v23 0/4] x86/sgx: Fix add page bugs

[Jarkko Sakkinen]

On Wed, Oct 16, 2019 at 09:39:14AM -0700, Sean Christopherson wrote:
> On Tue, Oct 15, 2019 at 12:03:17AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 10, 2019 at 03:55:26PM -0700, Sean Christopherson wrote:
> > > Three bug fixes in the add page flow, along with a related enhancement.
> > > Note, patch 4/4 is effectively a revert.
> > > 
> > > Sean Christopherson (4):
> > >   x86/sgx: Pass EADD the kernel's virtual address for the source page
> > >   x86/sgx: Check the validity of the source page address for EADD
> > >   x86/sgx: Fix EEXTEND error handling
> > >   x86/sgx: Drop mmap_sem before EEXTENDing an enclave page
> > > 
> > >  arch/x86/kernel/cpu/sgx/ioctl.c | 46 ++++++++++++++++++---------------
> > >  1 file changed, 25 insertions(+), 21 deletions(-)
> > 
> > So, which one should be merged first. This or v2 of the other series.
> 
> Shouldn't matter.
> 
> > Can you send a single "for-v23 fixes" series with everything needed just
> > so that there is no room guess work? You can call v3 of the series that
> > you first started.
> 
> Will do, I'll get it sent out today.

Thank you. I'll start to go through and merging (have the patch set
now).

Rather than send any new versions lets just discuss if there is
something that looks weird and I'll either pick the patch unmodified or
do whatever mods needs depending on the resolution.

/Jarkko