| Message ID | 20190808001254.11926-11-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Bug fixes for v22 | expand |
On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote: > 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@intel.com> Would be cleaner to check the flags just before the call. Otherwise, no problems with this. /Jarkko
On Thu, Aug 08, 2019 at 06:52:29PM +0300, Jarkko Sakkinen wrote: > On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote: > > 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@intel.com> > > Would be cleaner to check the flags just before the call. Otherwise, > no problems with this. That's not sufficient in the case that sgx_encl_grow() drops encl->lock to allocate an EPC page, as the flags need to be rechecked after the lock is reacquired. I'm not a huge fan of the code either, but it was the least ugly solution I could come up with and kinda fit since sgx_encl_grow() was already checking SGX_ENCL_DEAD.
On Thu, 2019-08-08 at 08:55 -0700, Sean Christopherson wrote: > On Thu, Aug 08, 2019 at 06:52:29PM +0300, Jarkko Sakkinen wrote: > > On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote: > > > 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@intel.com> > > > > Would be cleaner to check the flags just before the call. Otherwise, > > no problems with this. > > That's not sufficient in the case that sgx_encl_grow() drops encl->lock > to allocate an EPC page, as the flags need to be rechecked after the > lock is reacquired. I'm not a huge fan of the code either, but it was > the least ugly solution I could come up with and kinda fit since > sgx_encl_grow() was already checking SGX_ENCL_DEAD. Right, I was too hazy. We'll go what you proposed. /Jarkko
On Wed, Aug 07, 2019 at 05:12:53PM -0700, Sean Christopherson wrote: > 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@intel.com> Merged. /Jarkko
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)) {
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@intel.com> --- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 39 +++++++++----------------- 1 file changed, 13 insertions(+), 26 deletions(-)