[for,v24,v2,1/4] x86/sgx: Destroy enclave if EADD fails
diff mbox series

Message ID 20191105112056.21452-1-jarkko.sakkinen@linux.intel.com
State New
Headers show
Series
  • [for,v24,v2,1/4] x86/sgx: Destroy enclave if EADD fails
Related show

Commit Message

Jarkko Sakkinen Nov. 5, 2019, 11:20 a.m. UTC
__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(-)

Comments

Sean Christopherson Nov. 5, 2019, 10:58 p.m. UTC | #1
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.
Jarkko Sakkinen Nov. 6, 2019, 11:26 p.m. UTC | #2
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

Patch
diff mbox series

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)
 {