[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191209182219.GA19243@linux.intel.com>
Date: Mon, 9 Dec 2019 21:08:29 +0200
From: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To: Haitao Huang <haitao.huang@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
linux-sgx@...r.kernel.org, akpm@...ux-foundation.org,
dave.hansen@...el.com, sean.j.christopherson@...el.com,
nhorman@...hat.com, npmccallum@...hat.com, serge.ayoun@...el.com,
shay.katz-zamir@...el.com, haitao.huang@...el.com,
andriy.shevchenko@...ux.intel.com, tglx@...utronix.de,
kai.svahn@...el.com, bp@...en8.de, josh@...htriplett.org,
luto@...nel.org, kai.huang@...el.com, rientjes@...gle.com,
cedric.xing@...el.com, puiterwijk@...hat.com,
linux-security-module@...r.kernel.org,
Suresh Siddha <suresh.b.siddha@...el.com>
Subject: Re: [PATCH v24 12/24] x86/sgx: Linux Enclave Driver
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@...ux.intel.com> 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(¤t->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
Powered by blists - more mailing lists