[PATCH for_v22 00/11] x86/sgx: Bug fixes for v22

[PATCH for_v22 00/11] x86/sgx: Bug fixes for v22

[Sean Christopherson]

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

[PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0

[Sean Christopherson]

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

Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0

[Sean Christopherson]

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?

Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 01/11] x86/sgx: Fix an SECS collision with enclave page at VA=0

[Jarkko Sakkinen]

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

[PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check

[Sean Christopherson]

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

Re: [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 02/11] x86/sgx: Fix incorrect NULL pointer check

[Jarkko Sakkinen]

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

[PATCH for_v22 03/11] x86/sgx: Return '0' when sgx_ioc_enclave_set_attribute() succeeds

[Sean Christopherson]

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

Re: [PATCH for_v22 03/11] x86/sgx: Return '0' when sgx_ioc_enclave_set_attribute() succeeds

[Jarkko Sakkinen]

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

[PATCH for_v22 04/11] x86/sgx: x86/sgx: Require EADD destination to be page aligned

[Sean Christopherson]

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

Re: [PATCH for_v22 04/11] x86/sgx: x86/sgx: Require EADD destination to be page aligned

[Jarkko Sakkinen]

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

[PATCH for_v22 05/11] x86/sgx: Require EADD source to be page aligned

[Sean Christopherson]

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

Re: [PATCH for_v22 05/11] x86/sgx: Require EADD source to be page aligned

[Jarkko Sakkinen]

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

[PATCH for_v22 06/11] x86/sgx: Check the bounds of the enclave address against ELRANGE

[Sean Christopherson]

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

Re: [PATCH for_v22 06/11] x86/sgx: Check the bounds of the enclave address against ELRANGE

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 06/11] x86/sgx: Check the bounds of the enclave address against ELRANGE

[Jarkko Sakkinen]

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

[PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl

[Sean Christopherson]

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

Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl

[Sean Christopherson]

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.

Re: [PATCH for_v22 07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl

[Sean Christopherson]

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.

[PATCH for_v22 08/11] x86/sgx: Do not free enclave resources on redundant ECREATE

[Sean Christopherson]

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

Re: [PATCH for_v22 08/11] x86/sgx: Do not free enclave resources on redundant ECREATE

[Jarkko Sakkinen]

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

[PATCH for_v22 09/11] x86/sgx: Refactor error handling for user of sgx_encl_grow()

[Sean Christopherson]

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

Re: [PATCH for_v22 09/11] x86/sgx: Refactor error handling for user of sgx_encl_grow()

[Jarkko Sakkinen]

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

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

[Sean Christopherson]

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

Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held

[Sean Christopherson]

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.

Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 10/11] x86/sgx: Call sgx_encl_grow() with the enclave's lock held

[Jarkko Sakkinen]

On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote:
> Move the taking of the enclave's lock outside of sgx_encl_grow() in
> preparation for adding sgx_encl_shrink(), which will decrement the
> number of enclave pages and free any allocated VA page.  When freeing
> a VA page, the enclave's lock needs to be held for the entire time
> between adding the VA page to the enclave's list and freeing the VA
> page so as to prevent it from being used by reclaim, e.g. to avoid a
> use-after-free scenario.
> 
> Because sgx_encl_grow() can temporarily drop encl->lock, calling it
> with encl->lock held adds a subtle dependency on the ordering of
> checks against encl->flags, e.g. checking for SGX_ENCL_CREATED prior
> to calling sgx_encl_grow() could lead to a TOCTOU on ECREATE.  Avoid
> this by passing in the disallowed flags to sgx_encl_grow() so that the
> the dependency is clear.
> 
> Retaking encl->lock in the failure paths is a bit ugly, but the
> alternative is to have sgx_encl_grow() drop encl->lock in all failure
> paths, which is arguably worse since the caller has to know which
> paths do/don't drop the lock.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Merged.

/Jarkko

[PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails

[Sean Christopherson]

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

Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails

[Sean Christopherson]

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.

Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 11/11] x86/sgx: Shrink the enclave if ECREATE/EADD fails

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22

[Jarkko Sakkinen]

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

Re: [PATCH for_v22 00/11] x86/sgx: Bug fixes for v22

[Jarkko Sakkinen]

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