[PATCH for v24 v3 1/4] x86/sgx: %SGX_IOC_ENCLAVE_ADD_PAGES: Return -EIO when ENCLS fails

[PATCH for v24 v3 1/4] x86/sgx: %SGX_IOC_ENCLAVE_ADD_PAGES: Return -EIO when ENCLS fails

[Jarkko Sakkinen]

Use a common error code for all ENCLS failures so they can be easily
recognized from user space code.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index bbdc24d50169..2d6f7b8cc429 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;
 		}
 	}
 
@@ -496,10 +496,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 v3 2/4] x86/sgx: %SGX_IOC_ENCLAVE_ADD_PAGES: Destroy enclave when ENCLS fails

[Jarkko Sakkinen]

Destroy enclave on ENCLS[EADD] failure in order to get consistent
behavior when any ENCLS fails in this ioctl.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 2d6f7b8cc429..a2b411a8236d 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -413,8 +413,13 @@ 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) {
+		/* ENCLS failure. */
+		if (ret == -EIO)
+			sgx_encl_destroy(encl);
+
 		goto err_out;
+	}
 
 	/*
 	 * Complete the "add" before doing the "extend" so that the "add"
@@ -428,10 +433,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
 	if (addp->flags & SGX_PAGE_MEASURE) {
 		ret = __sgx_encl_extend(encl, epc_page);
 
-		/*
-		 * Destroy the enclave if EEXTEND fails, EADD can't be undone.
-		 * Note, destroy() also frees the resources for the added page.
-		 */
+		/* ENCLS failure. */
 		if (ret) {
 			sgx_encl_destroy(encl);
 			goto out_unlock;
@@ -494,6 +496,10 @@ static int sgx_encl_add_page(struct sgx_encl *encl,
  * re-invoke SGX_IOC_ENCLAVE_ADD_PAGES using the same struct in response to an
  * ERESTARTSYS error.
  *
+ * If ENCLS opcode fails, that effectively means that EPC has been invalidated.
+ * When this happens the enclave is destroyed and -EIO is returned to the
+ * caller.
+ *
  * Return:
  *   0 on success,
  *   -EACCES if an executable source page is located in a noexec partition,
-- 
2.20.1

[PATCH for v24 v3 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.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
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 a2b411a8236d..f08008bef943 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) {
 		/* ENCLS failure. */
 		if (ret == -EIO)
@@ -430,7 +430,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);
 
 		/* ENCLS failure. */
@@ -547,7 +547,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 v3 4/4] x86/sgx: Add @count to &sgx_enclave_add_pages

[Jarkko Sakkinen]

Add a single @count output variable to write the number of bytes
processed instead of encoding @count into three values and overwriting
input variable with those encodings.

This also more identical API to the comparable POSIX APIs (files and
sockets mainly).

Cc: Mark Shanahan <mark.shanahan@intel.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
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 f08008bef943..ab9e48cd294b 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.
- *
  * If ENCLS opcode fails, that effectively means that EPC has been invalidated.
  * When this happens the enclave is destroyed and -EIO is returned to the
  * caller.
@@ -510,6 +505,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))
@@ -538,7 +534,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;
@@ -547,15 +543,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 v3 1/4] x86/sgx: %SGX_IOC_ENCLAVE_ADD_PAGES: Return -EIO when ENCLS fails

[Jarkko Sakkinen]

On Tue, Nov 19, 2019 at 08:41:34PM +0200, Jarkko Sakkinen wrote:
> Use a common error code for all ENCLS failures so they can be easily
> recognized from user space code.
> 
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Merging this series to master.

/Jarkko