Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22
@ 2019-08-08  0:12 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
                   ` (13 more replies)
  0 siblings, 14 replies; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

A variety of bug fixes, some for issues reported by Shay, the rest for
issues found by inspection.  Sent as a bundle because most of the patches
conflict in one way or another, and so that others can apply on master to
get a stable build.

Sean Christopherson (11):
  x86/sgx: Fix an SECS collision with enclave page at VA=0
  x86/sgx: Fix incorrect NULL pointer check
  x86/sgx: Return '0' when sgx_ioc_enclave_set_attribute() succeeds
  x86/sgx: x86/sgx: Require EADD destination to be page aligned
  x86/sgx: Require EADD source to be page aligned
  x86/sgx: Check the bounds of the enclave address against ELRANGE
  x86/sgx: Check that enclave is created at beginning of EADD/EINIT
    ioctl
  x86/sgx: Do not free enclave resources on redundant ECREATE
  x86/sgx: Refactor error handling for user of sgx_encl_grow()
  x86/sgx: Call sgx_encl_grow() with the enclave's lock held
  x86/sgx: Shrink the enclave if ECREATE/EADD fails

 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 143 +++++++++++++++----------
 arch/x86/kernel/cpu/sgx/encl.c         |  14 ++-
 2 files changed, 97 insertions(+), 60 deletions(-)

-- 
2.22.0


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

* [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
@ 2019-08-08  0:12 ` Sean Christopherson
  2019-08-08 15:34   ` Jarkko Sakkinen
  2019-08-09 20:44   ` Jarkko Sakkinen
  2019-08-08  0:12 ` [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check Sean Christopherson
                   ` (12 subsequent siblings)
  13 siblings, 2 replies; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Detect the SECS in paging related flows by explicitly checking the page
against the enclave's SECS page.  Assuming a page with VA=0 is the SECS
will break enclaves that actually use VA=0, which is extremely unlikely
but theoretically possible.

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

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 909af9a664f0..6da1c36a01e6 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -12,10 +12,14 @@
 #include "encls.h"
 #include "sgx.h"
 
+static bool sgx_encl_is_secs(struct sgx_encl *encl, struct sgx_encl_page *page)
+{
+	return page == &encl->secs;
+}
+
 static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
 			   struct sgx_epc_page *epc_page)
 {
-	unsigned long addr = SGX_ENCL_PAGE_ADDR(encl_page);
 	unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page);
 	struct sgx_encl *encl = encl_page->encl;
 	pgoff_t page_index = sgx_encl_get_index(encl, encl_page);
@@ -38,11 +42,11 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
 		goto err_pcmd;
 	}
 
-	pginfo.addr = addr;
+	pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
 	pginfo.contents = (unsigned long)kmap_atomic(backing);
 	pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset;
-	pginfo.secs = addr ? (unsigned long)sgx_epc_addr(encl->secs.epc_page) :
-		      0;
+	pginfo.secs = sgx_encl_is_secs(encl, encl_page) ? 0 :
+			(unsigned long)sgx_epc_addr(encl->secs.epc_page);
 
 	ret = __eldu(&pginfo, sgx_epc_addr(epc_page),
 		     sgx_epc_addr(encl_page->va_page->epc_page) + va_offset);
@@ -546,7 +550,7 @@ void sgx_encl_release(struct kref *ref)
  */
 pgoff_t sgx_encl_get_index(struct sgx_encl *encl, struct sgx_encl_page *page)
 {
-	if (!PFN_DOWN(page->desc))
+	if (sgx_encl_is_secs(encl, page))
 		return PFN_DOWN(encl->size);
 
 	return PFN_DOWN(page->desc - encl->base);
-- 
2.22.0


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

* [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check
  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  0:12 ` Sean Christopherson
  2019-08-08 15:36   ` 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
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

The file pointer returned from fget() can be NULL, whereas a file's ops
are guaranteed to be non-NULL.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index f4a80585a519..89b3fb81c15b 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -801,7 +801,7 @@ static long sgx_ioc_enclave_set_attribute(struct file *filep, void __user *arg)
 		return -EFAULT;
 
 	attribute_file = fget(params.attribute_fd);
-	if (!attribute_file->f_op)
+	if (!attribute_file)
 		return -EINVAL;
 
 	if (attribute_file->f_op != &sgx_provision_fops) {
-- 
2.22.0


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

* [PATCH for_v22 03/11] x86/sgx: Return '0' when sgx_ioc_enclave_set_attribute() succeeds
  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  0:12 ` [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check Sean Christopherson
@ 2019-08-08  0:12 ` 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
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Ensure the local ret variable is set to zero when being returned via the
success path.

Reported-by: Shay Katz-zamir <shay.katz-zamir@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 89b3fb81c15b..ebb71eb3323a 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -810,6 +810,7 @@ static long sgx_ioc_enclave_set_attribute(struct file *filep, void __user *arg)
 	}
 
 	encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
+	ret = 0;
 
 out:
 	fput(attribute_file);
-- 
2.22.0


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

* [PATCH for_v22 04/11] x86/sgx: x86/sgx: Require EADD destination to be page aligned
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (2 preceding siblings ...)
  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  0:12 ` 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
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Check that the destination enclave address is page aligned, i.e. bits
11:0 are zero.  The userspace controlled address is used to initialize
page->desc, e.g. userspace can set kernel-internal flags by passing in
an unaligned address.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index ebb71eb3323a..ae381bf4cfd7 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -600,6 +600,9 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 	if (copy_from_user(&addp, arg, sizeof(addp)))
 		return -EFAULT;
 
+	if (!IS_ALIGNED(addp.addr, PAGE_SIZE))
+		return -EINVAL;
+
 	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
 			   sizeof(secinfo)))
 		return -EFAULT;
-- 
2.22.0


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

* [PATCH for_v22 05/11] x86/sgx: Require EADD source to be page aligned
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (3 preceding siblings ...)
  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  0:12 ` " 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
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Reject the EADD ioctl() if the source address provided by userspace is
not page aligned.  Page alignment is required by hardware, but this is
not enforced on userspace as the kernel first copies the source page to
an internal (page aligned) buffer.  Require the userspace address to be
page aligned so that the driver can, in the future, directly consume the
userspace address via EADD without breaking backwards compatibility,
e.g. to avoid the overhead of alloc+memcpy.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index ae381bf4cfd7..11d90a31e7c2 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -600,7 +600,8 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 	if (copy_from_user(&addp, arg, sizeof(addp)))
 		return -EFAULT;
 
-	if (!IS_ALIGNED(addp.addr, PAGE_SIZE))
+	if (!IS_ALIGNED(addp.addr, PAGE_SIZE) ||
+	    !IS_ALIGNED(addp.src, PAGE_SIZE))
 		return -EINVAL;
 
 	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
-- 
2.22.0


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

* [PATCH for_v22 06/11] x86/sgx: Check the bounds of the enclave address against ELRANGE
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (4 preceding siblings ...)
  2019-08-08  0:12 ` [PATCH for_v22 05/11] x86/sgx: Require EADD source " Sean Christopherson
@ 2019-08-08  0:12 ` Sean Christopherson
  2019-08-08 15:45   ` 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
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Reject EADD if the destination address lies outside the bounds of the
enclave's ELRANGE as tracked by encl->base and encl->size.  Lack of a
check allows userspace to induce a #GP on EADD.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 11d90a31e7c2..6a580361e20e 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -604,6 +604,9 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 	    !IS_ALIGNED(addp.src, PAGE_SIZE))
 		return -EINVAL;
 
+	if (addp.addr < encl->base || addp.addr - encl->base >= encl->size)
+		return -EINVAL;
+
 	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
 			   sizeof(secinfo)))
 		return -EFAULT;
-- 
2.22.0


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

* [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (5 preceding siblings ...)
  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  0:12 ` Sean Christopherson
  2019-08-08 15:47   ` Jarkko Sakkinen
  2019-08-09 23:40   ` Jarkko Sakkinen
  2019-08-08  0:12 ` [PATCH for_v22 08/11] x86/sgx: Do not free enclave resources on redundant ECREATE Sean Christopherson
                   ` (6 subsequent siblings)
  13 siblings, 2 replies; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
the ioctl() flows.  Deferring the check until the core code is fragile
as all code leading up to that point must be careful that it only uses
members of @encl that are initialized at allocation time.  For example,
the flush_work() call in sgx_encl_init() will crash if the enclave has
not been created.

Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
long as SGX_ENCL_CREATED is set only after the enclave is fully
initialized, it's not the kernel's responsibility to guard against
sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
the dependency.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 6a580361e20e..700d65c96b9a 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -326,6 +326,12 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	encl->base = secs->base;
 	encl->size = secs->size;
 	encl->ssaframesize = secs->ssa_frame_size;
+
+	/*
+	 * Set SGX_ENCL_CREATED only after the enclave is fully prepped.  This
+	 * allows other flows to check if the enclave has been created without
+	 * taking encl->lock.
+	 */
 	encl->flags |= SGX_ENCL_CREATED;
 
 	mutex_unlock(&encl->lock);
@@ -516,8 +522,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 
 	mutex_lock(&encl->lock);
 
-	if (!(encl->flags & SGX_ENCL_CREATED) ||
-	    (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD))) {
+	if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
 		ret = -EFAULT;
 		goto out;
 	}
@@ -597,6 +602,9 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 	void *data;
 	int ret;
 
+	if (!(encl->flags & SGX_ENCL_CREATED))
+		return -EINVAL;
+
 	if (copy_from_user(&addp, arg, sizeof(addp)))
 		return -EFAULT;
 
@@ -685,8 +693,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
 
 	mutex_lock(&encl->lock);
 
-	if (!(encl->flags & SGX_ENCL_CREATED) ||
-	    (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD))) {
+	if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
 		ret = -EFAULT;
 		goto err_out;
 	}
@@ -753,6 +760,9 @@ static long sgx_ioc_enclave_init(struct file *filep, void __user *arg)
 	struct page *initp_page;
 	int ret;
 
+	if (!(encl->flags & SGX_ENCL_CREATED))
+		return -EINVAL;
+
 	if (copy_from_user(&einit, arg, sizeof(einit)))
 		return -EFAULT;
 
-- 
2.22.0


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

* [PATCH for_v22 08/11] x86/sgx: Do not free enclave resources on redundant ECREATE
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (6 preceding siblings ...)
  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  0:12 ` 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
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Fix a bug where sgx_encl_create() incorrectly frees the enclave's SECS
and backing storage when the enclave has already been created.  Freeing
the structures leads to various forms of faults due to dereferencing
null pointers.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 700d65c96b9a..18f6925ab2ed 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -277,7 +277,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 
 	if (encl->flags & SGX_ENCL_CREATED) {
 		ret = -EFAULT;
-		goto err_out;
+		goto err_out_unlock;
 	}
 
 	ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
@@ -348,6 +348,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 		encl->backing = NULL;
 	}
 
+err_out_unlock:
 	mutex_unlock(&encl->lock);
 	return ret;
 }
-- 
2.22.0


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

* [PATCH for_v22 09/11] x86/sgx: Refactor error handling for user of sgx_encl_grow()
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (7 preceding siblings ...)
  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  0:12 ` Sean Christopherson
  2019-08-08 15:49   ` Jarkko Sakkinen
  2019-08-08  0:12 ` [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held Sean Christopherson
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Refactor sgx_encl_add_page() and sgx_encl_create() to prepare for
changes to sgx_encl_grow() that will introduce additional error paths.
Neither approach scales well as is, and it'll help readability for both
flows to use similar style error handling.

No functional change intended.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 18f6925ab2ed..fec5e0a346f5 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -283,14 +283,14 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
 	if (sgx_validate_secs(secs, ssaframesize)) {
 		ret = -EINVAL;
-		goto err_out;
+		goto err_out_unlock;
 	}
 
 	backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
 				   VM_NORESERVE);
 	if (IS_ERR(backing)) {
 		ret = PTR_ERR(backing);
-		goto err_out;
+		goto err_out_unlock;
 	}
 
 	encl->backing = backing;
@@ -300,7 +300,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	secs_epc = sgx_alloc_page(&encl->secs, true);
 	if (IS_ERR(secs_epc)) {
 		ret = PTR_ERR(secs_epc);
-		goto err_out;
+		goto err_out_backing;
 	}
 
 	encl->secs.epc_page = secs_epc;
@@ -338,15 +338,12 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	return 0;
 
 err_out:
-	if (encl->secs.epc_page) {
-		sgx_free_page(encl->secs.epc_page);
-		encl->secs.epc_page = NULL;
-	}
+	sgx_free_page(encl->secs.epc_page);
+	encl->secs.epc_page = NULL;
 
-	if (encl->backing) {
-		fput(encl->backing);
-		encl->backing = NULL;
-	}
+err_out_backing:
+	fput(encl->backing);
+	encl->backing = NULL;
 
 err_out_unlock:
 	mutex_unlock(&encl->lock);
@@ -525,23 +522,28 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 
 	if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
 		ret = -EFAULT;
-		goto out;
+		goto err_out_unlock;
 	}
 
 	encl_page = sgx_encl_page_alloc(encl, addr, prot);
 	if (IS_ERR(encl_page)) {
 		ret = PTR_ERR(encl_page);
-		goto out;
+		goto err_out_unlock;
 	}
 
 	ret = __sgx_encl_add_page(encl, encl_page, data, secinfo, mrmask);
-	if (ret) {
-		radix_tree_delete(&encl_page->encl->page_tree,
-				  PFN_DOWN(encl_page->desc));
-		kfree(encl_page);
-	}
+	if (ret)
+		goto err_out;
 
-out:
+	mutex_unlock(&encl->lock);
+	return 0;
+
+err_out:
+	radix_tree_delete(&encl_page->encl->page_tree,
+			  PFN_DOWN(encl_page->desc));
+	kfree(encl_page);
+
+err_out_unlock:
 	mutex_unlock(&encl->lock);
 	return ret;
 }
-- 
2.22.0


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

* [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (8 preceding siblings ...)
  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  0:12 ` Sean Christopherson
  2019-08-08 15:52   ` 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
                   ` (3 subsequent siblings)
  13 siblings, 2 replies; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

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


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

* [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (9 preceding siblings ...)
  2019-08-08  0:12 ` [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held Sean Christopherson
@ 2019-08-08  0:12 ` Sean Christopherson
  2019-08-08 15:50   ` Jarkko Sakkinen
  2019-08-08 15:18 ` [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Jarkko Sakkinen
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08  0:12 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

Add sgx_encl_shrink() to pair with sgx_encl_grow() and use it to adjust
the VA page count when ECREATE or EADD fails.  Return the allocated VA
page from sgx_encl_grow() so that it can be freed during shrink.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index a531cf615f3c..173a405d59a5 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -22,16 +22,17 @@ struct sgx_add_page_req {
 	struct list_head list;
 };
 
-static int sgx_encl_grow(struct sgx_encl *encl, unsigned int disallowed_flags)
+static struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl,
+					 unsigned int disallowed_flags)
 {
-	struct sgx_va_page *va_page;
-	int ret;
+	struct sgx_va_page *va_page = NULL;
+	void *err;
 
 	BUILD_BUG_ON(SGX_VA_SLOT_COUNT !=
 		(SGX_ENCL_PAGE_VA_OFFSET_MASK >> 3) + 1);
 
 	if (encl->flags & disallowed_flags)
-		return -EFAULT;
+		return ERR_PTR(-EFAULT);
 
 	if (!(encl->page_cnt % SGX_VA_SLOT_COUNT)) {
 		mutex_unlock(&encl->lock);
@@ -46,22 +47,34 @@ static int sgx_encl_grow(struct sgx_encl *encl, unsigned int disallowed_flags)
 		mutex_lock(&encl->lock);
 
 		if (IS_ERR(va_page->epc_page)) {
-			ret = PTR_ERR(va_page->epc_page);
+			err = ERR_CAST(va_page->epc_page);
 			kfree(va_page);
-			return ret;
+			return err;
 		} else if (encl->flags & disallowed_flags) {
 			sgx_free_page(va_page->epc_page);
 			kfree(va_page);
-			return -EFAULT;
+			return ERR_PTR(-EFAULT);
 		} else if (encl->page_cnt % SGX_VA_SLOT_COUNT) {
 			sgx_free_page(va_page->epc_page);
 			kfree(va_page);
+			va_page = NULL;
 		} else {
 			list_add(&va_page->list, &encl->va_pages);
 		}
 	}
 	encl->page_cnt++;
-	return 0;
+	return va_page;
+}
+
+static void sgx_encl_shrink(struct sgx_encl *encl, struct sgx_va_page *va_page)
+{
+	encl->page_cnt--;
+
+	if (va_page) {
+		sgx_free_page(va_page->epc_page);
+		list_del(&va_page->list);
+		kfree(va_page);
+	}
 }
 
 static bool sgx_process_add_page_req(struct sgx_add_page_req *req,
@@ -260,6 +273,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 {
 	unsigned long encl_size = secs->size + PAGE_SIZE;
 	struct sgx_epc_page *secs_epc;
+	struct sgx_va_page *va_page;
 	unsigned long ssaframesize;
 	struct sgx_pageinfo pginfo;
 	struct sgx_secinfo secinfo;
@@ -268,21 +282,23 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 
 	mutex_lock(&encl->lock);
 
-	ret = sgx_encl_grow(encl, SGX_ENCL_CREATED | SGX_ENCL_DEAD);
-	if (ret)
+	va_page = sgx_encl_grow(encl, SGX_ENCL_CREATED | SGX_ENCL_DEAD);
+	if (IS_ERR(va_page)) {
+		ret = PTR_ERR(va_page);
 		goto err_out_unlock;
+	}
 
 	ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
 	if (sgx_validate_secs(secs, ssaframesize)) {
 		ret = -EINVAL;
-		goto err_out_unlock;
+		goto err_out_shrink;
 	}
 
 	backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
 				   VM_NORESERVE);
 	if (IS_ERR(backing)) {
 		ret = PTR_ERR(backing);
-		goto err_out_unlock;
+		goto err_out_shrink;
 	}
 
 	encl->backing = backing;
@@ -337,6 +353,9 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	fput(encl->backing);
 	encl->backing = NULL;
 
+err_out_shrink:
+	sgx_encl_shrink(encl, va_page);
+
 err_out_unlock:
 	mutex_unlock(&encl->lock);
 	return ret;
@@ -496,6 +515,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 {
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_encl_page *encl_page;
+	struct sgx_va_page *va_page;
 	int ret;
 
 	if (sgx_validate_secinfo(secinfo))
@@ -508,14 +528,16 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 
 	mutex_lock(&encl->lock);
 
-	ret = sgx_encl_grow(encl, SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD);
-	if (ret)
+	va_page = sgx_encl_grow(encl, SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD);
+	if (IS_ERR(va_page)) {
+		ret = PTR_ERR(va_page);
 		goto err_out_unlock;
+	}
 
 	encl_page = sgx_encl_page_alloc(encl, addr, prot);
 	if (IS_ERR(encl_page)) {
 		ret = PTR_ERR(encl_page);
-		goto err_out_unlock;
+		goto err_out_shrink;
 	}
 
 	ret = __sgx_encl_add_page(encl, encl_page, data, secinfo, mrmask);
@@ -530,6 +552,9 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 			  PFN_DOWN(encl_page->desc));
 	kfree(encl_page);
 
+err_out_shrink:
+	sgx_encl_shrink(encl, va_page);
+
 err_out_unlock:
 	mutex_unlock(&encl->lock);
 	return ret;
-- 
2.22.0


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

* Re: [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (10 preceding siblings ...)
  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:18 ` Jarkko Sakkinen
  2019-08-08 15:57 ` Jarkko Sakkinen
  2019-08-10 11:44 ` Jarkko Sakkinen
  13 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:18 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:43PM -0700, Sean Christopherson wrote:
> A variety of bug fixes, some for issues reported by Shay, the rest for
> issues found by inspection.  Sent as a bundle because most of the patches
> conflict in one way or another, and so that others can apply on master to
> get a stable build.

Thanks for taking care of these! My test environment for SGX is working
again after some issues during last few weeks so should be also albe to
test them.

/Jarkko

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

* Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0
  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 20:44   ` Jarkko Sakkinen
  1 sibling, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:34 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote:
> Detect the SECS in paging related flows by explicitly checking the page
> against the enclave's SECS page.  Assuming a page with VA=0 is the SECS
> will break enclaves that actually use VA=0, which is extremely unlikely
> but theoretically possible.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I would define a macro to the same place where SGX_ENCL_PAGE_ADDR() is
defined and also SGX_ENCL_PAGE_IS_SECS() would definitely more
self-describing name.

Can't you BTW just use the backpointer in struct sgx_encl_page to the
enclave since we have it there? It is even set for SECS in
sgx_encl_create().

Also, lets try to avoid VA acronym in SGX context for other than version
array. I had a brief moment of confusion when reading the commit message
:-)

/Jarkko

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

* Re: [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check
  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
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:36 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:45PM -0700, Sean Christopherson wrote:
> The file pointer returned from fget() can be NULL, whereas a file's ops
> are guaranteed to be non-NULL.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 03/11] x86/sgx: Return '0' when sgx_ioc_enclave_set_attribute() succeeds
  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
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:37 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:46PM -0700, Sean Christopherson wrote:
> Ensure the local ret variable is set to zero when being returned via the
> success path.
> 
> Reported-by: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 04/11] x86/sgx: x86/sgx: Require EADD destination to be page aligned
  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
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:38 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:47PM -0700, Sean Christopherson wrote:
> Check that the destination enclave address is page aligned, i.e. bits
> 11:0 are zero.  The userspace controlled address is used to initialize
> page->desc, e.g. userspace can set kernel-internal flags by passing in
> an unaligned address.
> 
> Reported-by: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0
  2019-08-08 15:34   ` Jarkko Sakkinen
@ 2019-08-08 15:44     ` Sean Christopherson
  2019-08-09 15:13       ` Jarkko Sakkinen
  0 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08 15:44 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Thu, Aug 08, 2019 at 06:34:59PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote:
> > Detect the SECS in paging related flows by explicitly checking the page
> > against the enclave's SECS page.  Assuming a page with VA=0 is the SECS
> > will break enclaves that actually use VA=0, which is extremely unlikely
> > but theoretically possible.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I would define a macro to the same place where SGX_ENCL_PAGE_ADDR() is
> defined and also SGX_ENCL_PAGE_IS_SECS() would definitely more
> self-describing name.
> 
> Can't you BTW just use the backpointer in struct sgx_encl_page to the
> enclave since we have it there? It is even set for SECS in
> sgx_encl_create().

Yeah, that would work too.  I passed in @encl to match the format of
sgx_encl_get_index(), perhaps it makes sense to use the backpointer there
as well?

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

* Re: [PATCH for_v22 05/11] x86/sgx: Require EADD source to be page aligned
  2019-08-08  0:12 ` [PATCH for_v22 05/11] x86/sgx: Require EADD source " Sean Christopherson
@ 2019-08-08 15:44   ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:44 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:48PM -0700, Sean Christopherson wrote:
> Reject the EADD ioctl() if the source address provided by userspace is
> not page aligned.  Page alignment is required by hardware, but this is
> not enforced on userspace as the kernel first copies the source page to
> an internal (page aligned) buffer.  Require the userspace address to be
> page aligned so that the driver can, in the future, directly consume the
> userspace address via EADD without breaking backwards compatibility,
> e.g. to avoid the overhead of alloc+memcpy.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Not sure about this. Why not just implement a fast path when the address is
aligned (in future)?

/Jarkko

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

* Re: [PATCH for_v22 06/11] x86/sgx: Check the bounds of the enclave address against ELRANGE
  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
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:45 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:49PM -0700, Sean Christopherson wrote:
> Reject EADD if the destination address lies outside the bounds of the
> enclave's ELRANGE as tracked by encl->base and encl->size.  Lack of a
> check allows userspace to induce a #GP on EADD.
> 
> Reported-by: Shay Katz-zamir <shay.katz-zamir@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl
  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
  1 sibling, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:47 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> the ioctl() flows.  Deferring the check until the core code is fragile
> as all code leading up to that point must be careful that it only uses
> members of @encl that are initialized at allocation time.  For example,
> the flush_work() call in sgx_encl_init() will crash if the enclave has
> not been created.
> 
> Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> long as SGX_ENCL_CREATED is set only after the enclave is fully
> initialized, it's not the kernel's responsibility to guard against
> sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> the dependency.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 08/11] x86/sgx: Do not free enclave resources on redundant ECREATE
  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
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:48 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:51PM -0700, Sean Christopherson wrote:
> Fix a bug where sgx_encl_create() incorrectly frees the enclave's SECS
> and backing storage when the enclave has already been created.  Freeing
> the structures leads to various forms of faults due to dereferencing
> null pointers.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Good catch, thanks!

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 09/11] x86/sgx: Refactor error handling for user of sgx_encl_grow()
  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
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:49 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:52PM -0700, Sean Christopherson wrote:
> Refactor sgx_encl_add_page() and sgx_encl_create() to prepare for
> changes to sgx_encl_grow() that will introduce additional error paths.
> Neither approach scales well as is, and it'll help readability for both
> flows to use similar style error handling.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails
  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
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:50 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:54PM -0700, Sean Christopherson wrote:
> Add sgx_encl_shrink() to pair with sgx_encl_grow() and use it to adjust
> the VA page count when ECREATE or EADD fails.  Return the allocated VA
> page from sgx_encl_grow() so that it can be freed during shrink.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

* Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held
  2019-08-08  0:12 ` [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held Sean Christopherson
@ 2019-08-08 15:52   ` Jarkko Sakkinen
  2019-08-08 15:55     ` Sean Christopherson
  2019-08-10 11:32   ` Jarkko Sakkinen
  1 sibling, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:52 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

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

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

* Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held
  2019-08-08 15:52   ` Jarkko Sakkinen
@ 2019-08-08 15:55     ` Sean Christopherson
  2019-08-09 16:12       ` Jarkko Sakkinen
  0 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08 15:55 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

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.

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

* Re: [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (11 preceding siblings ...)
  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
  13 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-08 15:57 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:43PM -0700, Sean Christopherson wrote:
> A variety of bug fixes, some for issues reported by Shay, the rest for
> issues found by inspection.  Sent as a bundle because most of the patches
> conflict in one way or another, and so that others can apply on master to
> get a stable build.

Summary (once with A, I'm ready to squash):

https://patchwork.kernel.org/project/intel-sgx/list/?series=156821

Two of unacked patches just need just some minor rework. 4/11 was the
only with some opens at this point.

Thank you to both you and also to Shay who has done marvelous job catching
those regressions.

/Jarkko

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

* Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails
  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
  0 siblings, 2 replies; 41+ messages in thread
From: Sean Christopherson @ 2019-08-08 18:03 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Thu, Aug 08, 2019 at 06:50:08PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:54PM -0700, Sean Christopherson wrote:
> > Add sgx_encl_shrink() to pair with sgx_encl_grow() and use it to adjust
> > the VA page count when ECREATE or EADD fails.  Return the allocated VA
> > page from sgx_encl_grow() so that it can be freed during shrink.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

This missed wrapping -ENOMEM with ERR_PTR() when va_page alloation fails.
Let me know if you want me to send a v2 or if you'll fix it up when
applying.

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

* Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0
  2019-08-08 15:44     ` Sean Christopherson
@ 2019-08-09 15:13       ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 15:13 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Thu, 2019-08-08 at 08:44 -0700, Sean Christopherson wrote:
> On Thu, Aug 08, 2019 at 06:34:59PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote:
> > > Detect the SECS in paging related flows by explicitly checking the page
> > > against the enclave's SECS page.  Assuming a page with VA=0 is the SECS
> > > will break enclaves that actually use VA=0, which is extremely unlikely
> > > but theoretically possible.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > I would define a macro to the same place where SGX_ENCL_PAGE_ADDR() is
> > defined and also SGX_ENCL_PAGE_IS_SECS() would definitely more
> > self-describing name.
> > 
> > Can't you BTW just use the backpointer in struct sgx_encl_page to the
> > enclave since we have it there? It is even set for SECS in
> > sgx_encl_create().
> 
> Yeah, that would work too.  I passed in @encl to match the format of
> sgx_encl_get_index(), perhaps it makes sense to use the backpointer there
> as well?

Yes, it does of course. Probably have just forgotten to add it.
This kind of inconsistencies exist because backpointer has not
been always existing.

/Jarkko


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

* Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held
  2019-08-08 15:55     ` Sean Christopherson
@ 2019-08-09 16:12       ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 16:12 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

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


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

* Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails
  2019-08-08 18:03     ` Sean Christopherson
@ 2019-08-09 16:13       ` Jarkko Sakkinen
  2019-08-10 11:37       ` Jarkko Sakkinen
  1 sibling, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 16:13 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Thu, 2019-08-08 at 11:03 -0700, Sean Christopherson wrote:
> On Thu, Aug 08, 2019 at 06:50:08PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 07, 2019 at 05:12:54PM -0700, Sean Christopherson wrote:
> > > Add sgx_encl_shrink() to pair with sgx_encl_grow() and use it to adjust
> > > the VA page count when ECREATE or EADD fails.  Return the allocated VA
> > > page from sgx_encl_grow() so that it can be freed during shrink.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> This missed wrapping -ENOMEM with ERR_PTR() when va_page alloation fails.
> Let me know if you want me to send a v2 or if you'll fix it up when
> applying.

I can fix it, no problem.

/Jarkko


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

* Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0
  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-09 20:44   ` Jarkko Sakkinen
  2019-08-09 20:59     ` Jarkko Sakkinen
  1 sibling, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 20:44 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote:
> Detect the SECS in paging related flows by explicitly checking the page
> against the enclave's SECS page.  Assuming a page with VA=0 is the SECS
> will break enclaves that actually use VA=0, which is extremely unlikely
> but theoretically possible.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I applied this with the tweaks mentioned in the discussion:

* SGX_ENCL_PAGE_IS_SECS() as a macro.
* Removed @encl both sgx_get_index() and SGX_ENCL_PAGE_IS_SECS().

Not yet pushed. Just noting that I'm taking care of it.

/Jarkko

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

* Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0
  2019-08-09 20:44   ` Jarkko Sakkinen
@ 2019-08-09 20:59     ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 20:59 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Fri, Aug 09, 2019 at 11:44:56PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:44PM -0700, Sean Christopherson wrote:
> > Detect the SECS in paging related flows by explicitly checking the page
> > against the enclave's SECS page.  Assuming a page with VA=0 is the SECS
> > will break enclaves that actually use VA=0, which is extremely unlikely
> > but theoretically possible.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I applied this with the tweaks mentioned in the discussion:
> 
> * SGX_ENCL_PAGE_IS_SECS() as a macro.
> * Removed @encl both sgx_get_index() and SGX_ENCL_PAGE_IS_SECS().
> 
> Not yet pushed. Just noting that I'm taking care of it.

Not it is also pushed.

/Jarkko

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

* Re: [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check
  2019-08-08 15:36   ` Jarkko Sakkinen
@ 2019-08-09 21:16     ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 21:16 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Thu, Aug 08, 2019 at 06:36:43PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:45PM -0700, Sean Christopherson wrote:
> > The file pointer returned from fget() can be NULL, whereas a file's ops
> > are guaranteed to be non-NULL.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

2/11, 3/11 and 4/11 have been squashed and pushed.

/Jarkko

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

* Re: [PATCH for_v22 06/11] x86/sgx: Check the bounds of the enclave address against ELRANGE
  2019-08-08 15:45   ` Jarkko Sakkinen
@ 2019-08-09 21:21     ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 21:21 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Thu, Aug 08, 2019 at 06:45:30PM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:49PM -0700, Sean Christopherson wrote:
> > Reject EADD if the destination address lies outside the bounds of the
> > enclave's ELRANGE as tracked by encl->base and encl->size.  Lack of a
> > check allows userspace to induce a #GP on EADD.
> > 
> > Reported-by: Shay Katz-zamir <shay.katz-zamir@intel.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Applied with "patch -p1 -u" as my earlier merges screwed the ancestors.
Merged and pushed.

/Jarkko

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

* Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl
  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
  1 sibling, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-09 23:40 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> the ioctl() flows.  Deferring the check until the core code is fragile
> as all code leading up to that point must be careful that it only uses
> members of @encl that are initialized at allocation time.  For example,
> the flush_work() call in sgx_encl_init() will crash if the enclave has
> not been created.
> 
> Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> long as SGX_ENCL_CREATED is set only after the enclave is fully
> initialized, it's not the kernel's responsibility to guard against
> sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> the dependency.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

07/11, 08/11 and 09/11 have been squashed and pushed.

I'm now observing this kind of behavior with the self-test:

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

Any ideas what could cause this? This happen with the current master.

/Jarkko

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

* Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl
  2019-08-09 23:40   ` Jarkko Sakkinen
@ 2019-08-10  0:03     ` Sean Christopherson
  2019-08-10  0:10       ` Sean Christopherson
  0 siblings, 1 reply; 41+ messages in thread
From: Sean Christopherson @ 2019-08-10  0:03 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Sat, Aug 10, 2019 at 02:40:32AM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> > Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> > the ioctl() flows.  Deferring the check until the core code is fragile
> > as all code leading up to that point must be careful that it only uses
> > members of @encl that are initialized at allocation time.  For example,
> > the flush_work() call in sgx_encl_init() will crash if the enclave has
> > not been created.
> > 
> > Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> > long as SGX_ENCL_CREATED is set only after the enclave is fully
> > initialized, it's not the kernel's responsibility to guard against
> > sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> > the dependency.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> 07/11, 08/11 and 09/11 have been squashed and pushed.
> 
> I'm now observing this kind of behavior with the self-test:
> 
> jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
> $ sudo tools/testing/selftests/x86/sgx/test_sgx
> Binary size 24576 (0x6000), SIGSTRUCT size 1808
> Loading the enclave.
> ECREATE failed rc=-1, err=22.

Doh, I forgot to update/test the selftest.  I'll do so now.

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

* Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl
  2019-08-10  0:03     ` Sean Christopherson
@ 2019-08-10  0:10       ` Sean Christopherson
  0 siblings, 0 replies; 41+ messages in thread
From: Sean Christopherson @ 2019-08-10  0:10 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Fri, Aug 09, 2019 at 05:03:55PM -0700, Sean Christopherson wrote:
> On Sat, Aug 10, 2019 at 02:40:32AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> > > Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> > > the ioctl() flows.  Deferring the check until the core code is fragile
> > > as all code leading up to that point must be careful that it only uses
> > > members of @encl that are initialized at allocation time.  For example,
> > > the flush_work() call in sgx_encl_init() will crash if the enclave has
> > > not been created.
> > > 
> > > Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> > > long as SGX_ENCL_CREATED is set only after the enclave is fully
> > > initialized, it's not the kernel's responsibility to guard against
> > > sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> > > the dependency.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > 07/11, 08/11 and 09/11 have been squashed and pushed.
> > 
> > I'm now observing this kind of behavior with the self-test:
> > 
> > jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
> > $ sudo tools/testing/selftests/x86/sgx/test_sgx
> > Binary size 24576 (0x6000), SIGSTRUCT size 1808
> > Loading the enclave.
> > ECREATE failed rc=-1, err=22.
> 
> Doh, I forgot to update/test the selftest.  I'll do so now.

Oh, it's probably due to changing sgx_get_unmapped_area() to not align
the address, i.e. the selftest is likely passing in an unaligned ELRANGE.

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

* Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held
  2019-08-08  0:12 ` [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held Sean Christopherson
  2019-08-08 15:52   ` Jarkko Sakkinen
@ 2019-08-10 11:32   ` Jarkko Sakkinen
  1 sibling, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-10 11:32 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

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

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

* Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails
  2019-08-08 18:03     ` Sean Christopherson
  2019-08-09 16:13       ` Jarkko Sakkinen
@ 2019-08-10 11:37       ` Jarkko Sakkinen
  1 sibling, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-10 11:37 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Thu, Aug 08, 2019 at 11:03:55AM -0700, Sean Christopherson wrote:
> On Thu, Aug 08, 2019 at 06:50:08PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 07, 2019 at 05:12:54PM -0700, Sean Christopherson wrote:
> > > Add sgx_encl_shrink() to pair with sgx_encl_grow() and use it to adjust
> > > the VA page count when ECREATE or EADD fails.  Return the allocated VA
> > > page from sgx_encl_grow() so that it can be freed during shrink.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> This missed wrapping -ENOMEM with ERR_PTR() when va_page alloation fails.
> Let me know if you want me to send a v2 or if you'll fix it up when
> applying.

Merged.

/Jarkko

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

* Re: [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22
  2019-08-08  0:12 [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22 Sean Christopherson
                   ` (12 preceding siblings ...)
  2019-08-08 15:57 ` Jarkko Sakkinen
@ 2019-08-10 11:44 ` Jarkko Sakkinen
  13 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2019-08-10 11:44 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: linux-sgx, Shay Katz-zamir, Serge Ayoun

On Wed, Aug 07, 2019 at 05:12:43PM -0700, Sean Christopherson wrote:
>   x86/sgx: Require EADD source to be page aligned

Everything else merged expect this.

/Jarkko

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

end of thread, back to index

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held Sean Christopherson
2019-08-08 15:52   ` 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

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