Re: [PATCH v24 12/24] x86/sgx: Linux Enclave Driver

From: Jarkko Sakkinen
Date: Mon Dec 09 2019 - 14:08:44 EST


On Mon, Dec 02, 2019 at 09:48:43AM -0600, Haitao Huang wrote:
> On Fri, 29 Nov 2019 17:13:14 -0600, Jarkko Sakkinen
> <jarkko.sakkinen@xxxxxxxxxxxxxxx> wrote:
>
>
> > +
> > + for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> > + if (signal_pending(current)) {
> > + ret = -ERESTARTSYS;
> > + break;
> > + }
>
> This IOC is not idempotent as pages EADDed at this point can not be
> re-EADDed again. So we can't return ERESTARTSYS

Agreed, should be -EINTR.

I added these entries to the v25 change log based on your recent
reports:

* Fix a double-free issue when SGX_IOC_ENCLAVE_ADD_PAGES
fails on executing ENCLS[EADD]. The rollback path executed
radix_tree_delete() on the same address twice when this happened.
[fixes v24, reported by Haitao]
* Return -EINTR instead of -ERESTARTSYS in SGX_IOC_ENCLAVE_ADD_PAGES when
a signal is pending [fixes v23, reported by Haitao]

The master branch [1] has been updated accordingly. There is also a
tag v24 for looking up easily what has overally changed e.g.

$ git --no-pager diff v24..master
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index ab9e48cd294b..5c9e6e161698 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -413,13 +413,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,

ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
src);
- if (ret) {
- /* ENCLS failure. */
- if (ret == -EIO)
- sgx_encl_destroy(encl);
-
+ if (ret)
goto err_out;
- }

/*
* Complete the "add" before doing the "extend" so that the "add"
@@ -432,17 +427,11 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,

if (flags & SGX_PAGE_MEASURE) {
ret = __sgx_encl_extend(encl, epc_page);
-
- /* ENCLS failure. */
- if (ret) {
- sgx_encl_destroy(encl);
- goto out_unlock;
- }
+ if (ret)
+ goto err_out;
}

sgx_mark_page_reclaimable(encl_page->epc_page);
-
-out_unlock:
mutex_unlock(&encl->lock);
up_read(&current->mm->mmap_sem);
return ret;
@@ -460,6 +449,13 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
sgx_free_page(epc_page);
kfree(encl_page);

+ /*
+ * Destroy enclave on ENCLS failure as this means that EPC has been
+ * invalidated.
+ */
+ if (ret == -EIO)
+ sgx_encl_destroy(encl);
+
return ret;
}

@@ -536,7 +532,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)

for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
if (signal_pending(current)) {
- ret = -ERESTARTSYS;
+ ret = -EINTR;
break;
}

[1] https://github.com/jsakkine-intel/linux-sgx

/Jarkko