[PATCH for v24 v2 1/4] x86/sgx: Destroy enclave if EADD fails

[PATCH for v24 v2 1/4] x86/sgx: Destroy enclave if EADD fails

[Jarkko Sakkinen]

__sgx_encl_add_page() can only fail in the case of EPCM conflict at least
in non-artificial situations. Also, it consistent semantics in rollback is
something to pursue for. Thus, destroy enclave when the EADD fails as we do
when EEXTEND fails already.

In the cases it is sane to return -EIO. From this the caller can deduce
the failure and knows that the enclave was destroyed. The previous
-EFAULT could happen in numerous situations.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index d53aee5a64c1..289af607f634 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -338,7 +338,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
 	kunmap_atomic((void *)pginfo.contents);
 	put_page(src_page);
 
-	return ret ? -EFAULT : 0;
+	return ret ? -EIO : 0;
 }
 
 static int __sgx_encl_extend(struct sgx_encl *encl,
@@ -353,7 +353,7 @@ static int __sgx_encl_extend(struct sgx_encl *encl,
 		if (ret) {
 			if (encls_failed(ret))
 				ENCLS_WARN(ret, "EEXTEND");
-			return -EFAULT;
+			return -EIO;
 		}
 	}
 
@@ -413,8 +413,10 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 
 	ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
 				  addp->src);
-	if (ret)
+	if (ret) {
+		sgx_encl_destroy(encl);
 		goto err_out;
+	}
 
 	/*
 	 * Complete the "add" before doing the "extend" so that the "add"
@@ -498,10 +500,9 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
  *
  * Return:
  *   0 on success,
- *   -EINVAL if any input param or the SECINFO contains invalid data,
  *   -EACCES if an executable source page is located in a noexec partition,
- *   -ENOMEM if any memory allocation, including EPC, fails,
- *   -ERESTARTSYS if a pending signal is recognized
+ *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
+ *   -errno otherwise
  */
 static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 {
-- 
2.20.1

[PATCH for v24 v2 2/4] x86/sgx: Remove a subordinate clause

[Jarkko Sakkinen]

The subordinate clause of last sentence of the sgx_ioc_enclave_pages()
does not provide any insight not already provided. Thus, remove it.
Also, using "i.e." (and "e.g.") in the documentation should be
considered a bad practice because it leaves it open ended.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 289af607f634..87b2fb62825a 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -468,11 +468,9 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
  * @encl:       pointer to an enclave instance (via ioctl() file pointer)
  * @arg:	a user pointer to a struct sgx_enclave_add_pages instance
  *
- * Add (EADD) one or more pages to an uninitialized enclave, and optionally
- * extend (EEXTEND) the measurement with the contents of the page. The range of
- * pages must be virtually contiguous. The SECINFO and measurement mask are
- * applied to all pages, i.e. pages with different properties must be added in
- * separate calls.
+ * Add one or more pages to an uninitialized enclave, and optionally extend the
+ * measurement with the contents of the page. The address range of pages must
+ * be contiguous. The SECINFO and measurement mask are applied to all pages.
  *
  * A SECINFO for a TCS is required to always contain zero permissions because
  * CPU silently zeros them. Allowing anything else would cause a mismatch in
-- 
2.20.1

Re: [PATCH for v24 v2 2/4] x86/sgx: Remove a subordinate clause

[Jarkko Sakkinen]

On Tue, Nov 05, 2019 at 01:20:54PM +0200, Jarkko Sakkinen wrote:
> The subordinate clause of last sentence of the sgx_ioc_enclave_pages()
> does not provide any insight not already provided. Thus, remove it.
> Also, using "i.e." (and "e.g.") in the documentation should be
> considered a bad practice because it leaves it open ended.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Merged.

/Jarkko

[PATCH for v24 v2 3/4] x86/sgx: Detach sgx_encl_add_page() from struct sgx_enclave_add_pages

[Jarkko Sakkinen]

Internals should not have direct bindings to the ioctl API. Therefore,
unpack &sgx_enclave_add_pages and pass its fields as separate parameters to
sgx_enclave_add_page(). This will also remove an inconsistency: secinfo is
already passed as a separate parameter whereas other fields are read from
the struct.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 87b2fb62825a..deca49bd4f58 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -360,16 +360,16 @@ static int __sgx_encl_extend(struct sgx_encl *encl,
 	return 0;
 }
 
-static int sgx_encl_add_page(struct sgx_encl *encl,
-			     struct sgx_enclave_add_pages *addp,
-			     struct sgx_secinfo *secinfo)
+static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
+			     unsigned long offset, unsigned long length,
+			     struct sgx_secinfo *secinfo, unsigned long flags)
 {
 	struct sgx_encl_page *encl_page;
 	struct sgx_epc_page *epc_page;
 	struct sgx_va_page *va_page;
 	int ret;
 
-	encl_page = sgx_encl_page_alloc(encl, addp->offset, secinfo->flags);
+	encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
 	if (IS_ERR(encl_page))
 		return PTR_ERR(encl_page);
 
@@ -412,7 +412,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 		goto err_out_unlock;
 
 	ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
-				  addp->src);
+				  src);
 	if (ret) {
 		sgx_encl_destroy(encl);
 		goto err_out;
@@ -427,7 +427,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 	encl_page->epc_page = epc_page;
 	encl->secs_child_cnt++;
 
-	if (addp->flags & SGX_PAGE_MEASURE) {
+	if (flags & SGX_PAGE_MEASURE) {
 		ret = __sgx_encl_extend(encl, epc_page);
 
 		/*
@@ -543,7 +543,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 		if (need_resched())
 			cond_resched();
 
-		ret = sgx_encl_add_page(encl, &addp, &secinfo);
+		ret = sgx_encl_add_page(encl, addp.src, addp.offset,
+					addp.length, &secinfo, addp.flags);
 		if (ret)
 			break;
 
-- 
2.20.1

[PATCH for v24 v2 4/4] x86/sgx: add @count to &sgx_enclave_add_pages

[Jarkko Sakkinen]

Add @count write the number of bytes added as there is not any good reason
to overwrite input parameters. Also, three parameters are unnecessarily
overwritten as the amount of change is the same for each of them.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/include/uapi/asm/sgx.h |  2 ++
 arch/x86/kernel/cpu/sgx/ioctl.c | 17 ++++++-----------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 88644b6ad849..e196cfd44b70 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -45,6 +45,7 @@ struct sgx_enclave_create  {
  * @length:	length of the data (multiple of the page size)
  * @secinfo:	address for the SECINFO data
  * @flags:	page control flags
+ * @count:	number of bytes added (multiple of the page size)
  */
 struct sgx_enclave_add_pages {
 	__u64	src;
@@ -52,6 +53,7 @@ struct sgx_enclave_add_pages {
 	__u64	length;
 	__u64	secinfo;
 	__u64	flags;
+	__u64	count;
 };
 
 /**
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index deca49bd4f58..e8697d145dfb 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -491,11 +491,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
  * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek
  * an address range for the enclave that can be then populated into SECS.
  *
- * @arg->addr, @arg->src and @arg->length are adjusted to reflect the
- * remaining pages that need to be added to the enclave, e.g. userspace can
- * re-invoke SGX_IOC_ENCLAVE_ADD_PAGES using the same struct in response to an
- * ERESTARTSYS error.
- *
  * Return:
  *   0 on success,
  *   -EACCES if an executable source page is located in a noexec partition,
@@ -506,6 +501,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 {
 	struct sgx_enclave_add_pages addp;
 	struct sgx_secinfo secinfo;
+	unsigned long c;
 	int ret;
 
 	if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
@@ -534,7 +530,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 	if (sgx_validate_secinfo(&secinfo))
 		return -EINVAL;
 
-	for ( ; addp.length > 0; addp.length -= PAGE_SIZE) {
+	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
 		if (signal_pending(current)) {
 			ret = -ERESTARTSYS;
 			break;
@@ -543,15 +539,14 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
 		if (need_resched())
 			cond_resched();
 
-		ret = sgx_encl_add_page(encl, addp.src, addp.offset,
-					addp.length, &secinfo, addp.flags);
+		ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
+					addp.length - c, &secinfo, addp.flags);
 		if (ret)
 			break;
-
-		addp.offset += PAGE_SIZE;
-		addp.src += PAGE_SIZE;
 	}
 
+	addp.count = c;
+
 	if (copy_to_user(arg, &addp, sizeof(addp)))
 		return -EFAULT;
 
-- 
2.20.1

Re: [PATCH for v24 v2 4/4] x86/sgx: add @count to &sgx_enclave_add_pages

[Sean Christopherson]

On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote:
> Add @count write the number of bytes added as there is not any good reason
> to overwrite input parameters.

I disagree, overwriting the params means userspace doesn't need to adjust
the values to restart the ioctl().  Ditto for printing out the failing
address if the ioctl() fails.

> Also, three parameters are unnecessarily
> overwritten as the amount of change is the same for each of them.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/include/uapi/asm/sgx.h |  2 ++
>  arch/x86/kernel/cpu/sgx/ioctl.c | 17 ++++++-----------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 88644b6ad849..e196cfd44b70 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -45,6 +45,7 @@ struct sgx_enclave_create  {
>   * @length:	length of the data (multiple of the page size)
>   * @secinfo:	address for the SECINFO data
>   * @flags:	page control flags
> + * @count:	number of bytes added (multiple of the page size)
>   */
>  struct sgx_enclave_add_pages {
>  	__u64	src;
> @@ -52,6 +53,7 @@ struct sgx_enclave_add_pages {
>  	__u64	length;
>  	__u64	secinfo;
>  	__u64	flags;
> +	__u64	count;
>  };
>  
>  /**
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index deca49bd4f58..e8697d145dfb 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -491,11 +491,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
>   * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek
>   * an address range for the enclave that can be then populated into SECS.
>   *
> - * @arg->addr, @arg->src and @arg->length are adjusted to reflect the
> - * remaining pages that need to be added to the enclave, e.g. userspace can
> - * re-invoke SGX_IOC_ENCLAVE_ADD_PAGES using the same struct in response to an
> - * ERESTARTSYS error.
> - *
>   * Return:
>   *   0 on success,
>   *   -EACCES if an executable source page is located in a noexec partition,
> @@ -506,6 +501,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  {
>  	struct sgx_enclave_add_pages addp;
>  	struct sgx_secinfo secinfo;
> +	unsigned long c;
>  	int ret;
>  
>  	if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
> @@ -534,7 +530,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  	if (sgx_validate_secinfo(&secinfo))
>  		return -EINVAL;
>  
> -	for ( ; addp.length > 0; addp.length -= PAGE_SIZE) {
> +	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
>  		if (signal_pending(current)) {
>  			ret = -ERESTARTSYS;
>  			break;
> @@ -543,15 +539,14 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
>  		if (need_resched())
>  			cond_resched();
>  
> -		ret = sgx_encl_add_page(encl, addp.src, addp.offset,
> -					addp.length, &secinfo, addp.flags);
> +		ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
> +					addp.length - c, &secinfo, addp.flags);
>  		if (ret)
>  			break;
> -
> -		addp.offset += PAGE_SIZE;
> -		addp.src += PAGE_SIZE;
>  	}
>  
> +	addp.count = c;
> +
>  	if (copy_to_user(arg, &addp, sizeof(addp)))
>  		return -EFAULT;
>  
> -- 
> 2.20.1
> 

Re: [PATCH for v24 v2 4/4] x86/sgx: add @count to &sgx_enclave_add_pages

[Jarkko Sakkinen]

On Tue, Nov 05, 2019 at 02:52:23PM -0800, Sean Christopherson wrote:
> On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote:
> > Add @count write the number of bytes added as there is not any good reason
> > to overwrite input parameters.
> 
> I disagree, overwriting the params means userspace doesn't need to adjust
> the values to restart the ioctl().  Ditto for printing out the failing
> address if the ioctl() fails.

There is three redundant updates. At least only @length must be
updated in order to remove this glitch.

As far as overwriting goes, it should be only done when there is
requiring to do that.

/Jarkko

Re: [PATCH for v24 v2 4/4] x86/sgx: add @count to &sgx_enclave_add_pages

[Jarkko Sakkinen]

On Thu, Nov 07, 2019 at 01:20:30AM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 05, 2019 at 02:52:23PM -0800, Sean Christopherson wrote:
> > On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote:
> > > Add @count write the number of bytes added as there is not any good reason
> > > to overwrite input parameters.
> > 
> > I disagree, overwriting the params means userspace doesn't need to adjust
> > the values to restart the ioctl().  Ditto for printing out the failing
> > address if the ioctl() fails.
> 
> There is three redundant updates. At least only @length must be
> updated in order to remove this glitch.
> 
> As far as overwriting goes, it should be only done when there is
> requiring to do that.

What is obvious is that the current behaviour is wrong. You have
a *single value* to return and you encode the *same value* with
*three encodings*:

1. offset + count
2. length - count
3. src + count

And ironically none of the encodings give you the count of bytes
processed in unpacked form. It is something that must be readily
available as practically all common syscalls that can partially process
the input give that. There is a long history of that pattern and no
history at all with this weird pack of encodings.

/Jarkko

Re: [PATCH for v24 v2 1/4] x86/sgx: Destroy enclave if EADD fails

[Sean Christopherson]

On Tue, Nov 05, 2019 at 01:20:53PM +0200, Jarkko Sakkinen wrote:
> __sgx_encl_add_page() can only fail in the case of EPCM conflict at least
> in non-artificial situations. Also, it consistent semantics in rollback is
> something to pursue for. Thus, destroy enclave when the EADD fails as we do
> when EEXTEND fails already.

I still don't understand the motiviation for this change, EADD can fault
and fail for reasons that are purely under userspace control.  Yes, it's
all but guaranteed to be a userspace bug, but I can't think of another
instance in the kernel where the reaction to what is effectively an invalid
param is to torch the whole thing.  EEXTEND is special cased because the
kernel doesn't have any other sane choice.

Re: [PATCH for v24 v2 1/4] x86/sgx: Destroy enclave if EADD fails

[Jarkko Sakkinen]

On Tue, Nov 05, 2019 at 02:58:33PM -0800, Sean Christopherson wrote:
> On Tue, Nov 05, 2019 at 01:20:53PM +0200, Jarkko Sakkinen wrote:
> > __sgx_encl_add_page() can only fail in the case of EPCM conflict at least
> > in non-artificial situations. Also, it consistent semantics in rollback is
> > something to pursue for. Thus, destroy enclave when the EADD fails as we do
> > when EEXTEND fails already.
> 
> I still don't understand the motiviation for this change, EADD can fault
> and fail for reasons that are purely under userspace control.  Yes, it's
> all but guaranteed to be a userspace bug, but I can't think of another
> instance in the kernel where the reaction to what is effectively an invalid
> param is to torch the whole thing.  EEXTEND is special cased because the
> kernel doesn't have any other sane choice.

-EIO should be returned in both cases so that caller has a way to
determine if the ENCLS operations failed. They sum into a transaction
that is why the rollback must be the same for any sane semantics.

If the caller wants to purposely cause that, it is caller choice. And we
don't want to purposely support completely undeterministic behaviour
when there is no backwards compatibility to maintain.

/Jarkko