Move the taking of the enclave's lock outside of sgx_encl_grow() in
preparation for adding sgx_encl_shrink(), which will decrement the
number of enclave pages and free any allocated VA page. When freeing
a VA page, the enclave's lock needs to be held for the entire time
between adding the VA page to the enclave's list and freeing the VA
page so as to prevent it from being used by reclaim, e.g. to avoid a
use-after-free scenario.
Because sgx_encl_grow() can temporarily drop encl->lock, calling it
with encl->lock held adds a subtle dependency on the ordering of
checks against encl->flags, e.g. checking for SGX_ENCL_CREATED prior
to calling sgx_encl_grow() could lead to a TOCTOU on ECREATE. Avoid
this by passing in the disallowed flags to sgx_encl_grow() so that the
the dependency is clear.
Retaking encl->lock in the failure paths is a bit ugly, but the
alternative is to have sgx_encl_grow() drop encl->lock in all failure
paths, which is arguably worse since the caller has to know which
paths do/don't drop the lock.
Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
---
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 39 +++++++++-----------------
1 file changed, 13 insertions(+), 26 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index fec5e0a346f5..a531cf615f3c 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -22,7 +22,7 @@ struct sgx_add_page_req {
struct list_head list;
};
-static int sgx_encl_grow(struct sgx_encl *encl)
+static int sgx_encl_grow(struct sgx_encl *encl, unsigned int disallowed_flags)
{
struct sgx_va_page *va_page;
int ret;
@@ -30,30 +30,28 @@ static int sgx_encl_grow(struct sgx_encl *encl)
BUILD_BUG_ON(SGX_VA_SLOT_COUNT !=
(SGX_ENCL_PAGE_VA_OFFSET_MASK >> 3) + 1);
- mutex_lock(&encl->lock);
- if (encl->flags & SGX_ENCL_DEAD) {
- mutex_unlock(&encl->lock);
+ if (encl->flags & disallowed_flags)
return -EFAULT;
- }
if (!(encl->page_cnt % SGX_VA_SLOT_COUNT)) {
mutex_unlock(&encl->lock);
va_page = kzalloc(sizeof(*va_page), GFP_KERNEL);
- if (!va_page)
+ if (!va_page) {
+ mutex_lock(&encl->lock);
return -ENOMEM;
+ }
+
va_page->epc_page = sgx_alloc_va_page();
+ mutex_lock(&encl->lock);
+
if (IS_ERR(va_page->epc_page)) {
ret = PTR_ERR(va_page->epc_page);
kfree(va_page);
return ret;
- }
-
- mutex_lock(&encl->lock);
- if (encl->flags & SGX_ENCL_DEAD) {
+ } else if (encl->flags & disallowed_flags) {
sgx_free_page(va_page->epc_page);
kfree(va_page);
- mutex_unlock(&encl->lock);
return -EFAULT;
} else if (encl->page_cnt % SGX_VA_SLOT_COUNT) {
sgx_free_page(va_page->epc_page);
@@ -63,7 +61,6 @@ static int sgx_encl_grow(struct sgx_encl *encl)
}
}
encl->page_cnt++;
- mutex_unlock(&encl->lock);
return 0;
}
@@ -269,16 +266,11 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
struct file *backing;
long ret;
- ret = sgx_encl_grow(encl);
- if (ret)
- return ret;
-
mutex_lock(&encl->lock);
- if (encl->flags & SGX_ENCL_CREATED) {
- ret = -EFAULT;
+ ret = sgx_encl_grow(encl, SGX_ENCL_CREATED | SGX_ENCL_DEAD);
+ if (ret)
goto err_out_unlock;
- }
ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
if (sgx_validate_secs(secs, ssaframesize)) {
@@ -514,16 +506,11 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
return ret;
}
- ret = sgx_encl_grow(encl);
- if (ret)
- return ret;
-
mutex_lock(&encl->lock);
- if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
- ret = -EFAULT;
+ ret = sgx_encl_grow(encl, SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD);
+ if (ret)
goto err_out_unlock;
- }
encl_page = sgx_encl_page_alloc(encl, addr, prot);
if (IS_ERR(encl_page)) {
--
2.22.0
