Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH for_v23 0/4] x86/sgx: Fix add page bugs
@ 2019-10-10 22:55 Sean Christopherson
  2019-10-10 22:55 ` [PATCH for_v23 1/4] x86/sgx: Pass EADD the kernel's virtual address for the source page Sean Christopherson
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Sean Christopherson @ 2019-10-10 22:55 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

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


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

* [PATCH for_v23 1/4] x86/sgx: Pass EADD the kernel's virtual address for the source page
  2019-10-10 22:55 [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Sean Christopherson
@ 2019-10-10 22:55 ` Sean Christopherson
  2019-10-10 22:55 ` [PATCH for_v23 2/4] x86/sgx: Check the validity of the source page address for EADD Sean Christopherson
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2019-10-10 22:55 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

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


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

* [PATCH for_v23 2/4] x86/sgx: Check the validity of the source page address for EADD
  2019-10-10 22:55 [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Sean Christopherson
  2019-10-10 22:55 ` [PATCH for_v23 1/4] x86/sgx: Pass EADD the kernel's virtual address for the source page Sean Christopherson
@ 2019-10-10 22:55 ` Sean Christopherson
  2019-10-10 22:55 ` [PATCH for_v23 3/4] x86/sgx: Fix EEXTEND error handling Sean Christopherson
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2019-10-10 22:55 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

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


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

* [PATCH for_v23 3/4] x86/sgx: Fix EEXTEND error handling
  2019-10-10 22:55 [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Sean Christopherson
  2019-10-10 22:55 ` [PATCH for_v23 1/4] x86/sgx: Pass EADD the kernel's virtual address for the source page Sean Christopherson
  2019-10-10 22:55 ` [PATCH for_v23 2/4] x86/sgx: Check the validity of the source page address for EADD Sean Christopherson
@ 2019-10-10 22:55 ` Sean Christopherson
  2019-10-10 22:55 ` [PATCH for_v23 4/4] x86/sgx: Drop mmap_sem before EEXTENDing an enclave page Sean Christopherson
  2019-10-14 21:03 ` [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Jarkko Sakkinen
  4 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2019-10-10 22:55 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

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


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

* [PATCH for_v23 4/4] x86/sgx: Drop mmap_sem before EEXTENDing an enclave page
  2019-10-10 22:55 [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Sean Christopherson
                   ` (2 preceding siblings ...)
  2019-10-10 22:55 ` [PATCH for_v23 3/4] x86/sgx: Fix EEXTEND error handling Sean Christopherson
@ 2019-10-10 22:55 ` Sean Christopherson
  2019-10-14 21:03 ` [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Jarkko Sakkinen
  4 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2019-10-10 22:55 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

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


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

* Re: [PATCH for_v23 0/4] x86/sgx: Fix add page bugs
  2019-10-10 22:55 [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Sean Christopherson
                   ` (3 preceding siblings ...)
  2019-10-10 22:55 ` [PATCH for_v23 4/4] x86/sgx: Drop mmap_sem before EEXTENDing an enclave page Sean Christopherson
@ 2019-10-14 21:03 ` Jarkko Sakkinen
  2019-10-16 16:39   ` Sean Christopherson
  4 siblings, 1 reply; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-10-14 21:03 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx

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

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

* Re: [PATCH for_v23 0/4] x86/sgx: Fix add page bugs
  2019-10-14 21:03 ` [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Jarkko Sakkinen
@ 2019-10-16 16:39   ` Sean Christopherson
  2019-10-17 16:25     ` Jarkko Sakkinen
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2019-10-16 16:39 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx

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.

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

* Re: [PATCH for_v23 0/4] x86/sgx: Fix add page bugs
  2019-10-16 16:39   ` Sean Christopherson
@ 2019-10-17 16:25     ` Jarkko Sakkinen
  0 siblings, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2019-10-17 16:25 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx

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

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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 22:55 [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Sean Christopherson
2019-10-10 22:55 ` [PATCH for_v23 1/4] x86/sgx: Pass EADD the kernel's virtual address for the source page Sean Christopherson
2019-10-10 22:55 ` [PATCH for_v23 2/4] x86/sgx: Check the validity of the source page address for EADD Sean Christopherson
2019-10-10 22:55 ` [PATCH for_v23 3/4] x86/sgx: Fix EEXTEND error handling Sean Christopherson
2019-10-10 22:55 ` [PATCH for_v23 4/4] x86/sgx: Drop mmap_sem before EEXTENDing an enclave page Sean Christopherson
2019-10-14 21:03 ` [PATCH for_v23 0/4] x86/sgx: Fix add page bugs Jarkko Sakkinen
2019-10-16 16:39   ` Sean Christopherson
2019-10-17 16:25     ` Jarkko Sakkinen

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
	public-inbox-index linux-sgx

Example config snippet for mirrors

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.git