* [PATCH] x86/sgx: Fix double-free when EADD fails @ 2019-12-05 10:01 Jarkko Sakkinen 2019-12-09 20:52 ` Sean Christopherson 0 siblings, 1 reply; 5+ messages in thread From: Jarkko Sakkinen @ 2019-12-05 10:01 UTC (permalink / raw) To: linux-sgx; +Cc: Jarkko Sakkinen, Sean Christopherson, Huang Haitao radix_tree_delete() gets called twice for the same page when EADD fails. This commit fixes the issue. Cc: Sean Christopherson <sean.j.christopherson@intel.com> Reported-by: Huang Haitao <haitao.huang@intel.com> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/ioctl.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index ab9e48cd294b..2ff12038a8a4 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,12 @@ 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 +450,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; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/sgx: Fix double-free when EADD fails 2019-12-05 10:01 [PATCH] x86/sgx: Fix double-free when EADD fails Jarkko Sakkinen @ 2019-12-09 20:52 ` Sean Christopherson 2019-12-11 11:11 ` Jarkko Sakkinen 0 siblings, 1 reply; 5+ messages in thread From: Sean Christopherson @ 2019-12-09 20:52 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx, Huang Haitao On Thu, Dec 05, 2019 at 12:01:51PM +0200, Jarkko Sakkinen wrote: > radix_tree_delete() gets called twice for the same page when EADD > fails. This commit fixes the issue. > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Reported-by: Huang Haitao <haitao.huang@intel.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/ioctl.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index ab9e48cd294b..2ff12038a8a4 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,12 @@ 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 +450,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. This comment is wrong, EADD can fail due to bad userspace input, and both EADD and EEXTEND can fail due to hardware/software bugs. > + */ > + if (ret == -EIO) Not a fan of making this dependent on -EIO, IMO invalidating iff EEXTEND fails is cleaner. In other words, I still think killing the enclave on on EADD failure is unnecessary. Side topic, __sgx_encl_add_page() doesn't correctly get_user_pages() returning zero, e.g. the code should be something like: ret = get_user_pages(src, 1, 0, &src_page, NULL); if (!ret) return -EBUSY: if (ret < 0) return ret; > + sgx_encl_destroy(encl); > + > return ret; > } > > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/sgx: Fix double-free when EADD fails 2019-12-09 20:52 ` Sean Christopherson @ 2019-12-11 11:11 ` Jarkko Sakkinen 2019-12-11 16:07 ` Sean Christopherson 0 siblings, 1 reply; 5+ messages in thread From: Jarkko Sakkinen @ 2019-12-11 11:11 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx, Huang Haitao On Mon, Dec 09, 2019 at 12:52:55PM -0800, Sean Christopherson wrote: > On Thu, Dec 05, 2019 at 12:01:51PM +0200, Jarkko Sakkinen wrote: > > radix_tree_delete() gets called twice for the same page when EADD > > fails. This commit fixes the issue. > > > > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > > Reported-by: Huang Haitao <haitao.huang@intel.com> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > arch/x86/kernel/cpu/sgx/ioctl.c | 23 ++++++++++------------- > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > > index ab9e48cd294b..2ff12038a8a4 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,12 @@ 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 +450,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. > > This comment is wrong, EADD can fail due to bad userspace input, and both > EADD and EEXTEND can fail due to hardware/software bugs. Any piece of kernel code can fail because of either hardware or software bug. Still almost none of the comments explicitly state this. However, userspace input is a valid remark. > > > + */ > > + if (ret == -EIO) > > Not a fan of making this dependent on -EIO, IMO invalidating iff EEXTEND > fails is cleaner. In other words, I still think killing the enclave on > on EADD failure is unnecessary. This comes down to whether you consider them as a transaction. I do and it makes a coherent API. If user code wants to give on purpose bad input for EADD, I think it is sane to kill the enclave also in that case. There is no legit use case where user code would do that. Potentially that could be malicious behaviour (maybe nothing useful can be done with that primitive but that is only thing that it could be theoretically used for since legit use cases do not exist). > Side topic, __sgx_encl_add_page() doesn't correctly get_user_pages() > returning zero, e.g. the code should be something like: > > ret = get_user_pages(src, 1, 0, &src_page, NULL); > if (!ret) > return -EBUSY: > if (ret < 0) > return ret; Thanks! I'll update the GIT based on your feedback (it's probably better that at this point for small scoped changes you just describe the change and I'll update. With all squashing and rebasing that is fastest). /Jarkko ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/sgx: Fix double-free when EADD fails 2019-12-11 11:11 ` Jarkko Sakkinen @ 2019-12-11 16:07 ` Sean Christopherson 2019-12-12 23:59 ` Jarkko Sakkinen 0 siblings, 1 reply; 5+ messages in thread From: Sean Christopherson @ 2019-12-11 16:07 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx, Huang Haitao On Wed, Dec 11, 2019 at 01:11:52PM +0200, Jarkko Sakkinen wrote: > On Mon, Dec 09, 2019 at 12:52:55PM -0800, Sean Christopherson wrote: > > Not a fan of making this dependent on -EIO, IMO invalidating iff EEXTEND > > fails is cleaner. In other words, I still think killing the enclave on > > on EADD failure is unnecessary. > > This comes down to whether you consider them as a transaction. I do > and it makes a coherent API. What's your definition of transaction in this context? My interpretation of transaction here would be that each ioctl() should either succeed, fail without modifying persistent (enclave) state, or fail and kill the enclave (because its state modifications are irreversible). EEXTEND falls into the last case because EADD can't be unwound. EADD falls into the middle case because everything up to EADD can be cleanly undone. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/sgx: Fix double-free when EADD fails 2019-12-11 16:07 ` Sean Christopherson @ 2019-12-12 23:59 ` Jarkko Sakkinen 0 siblings, 0 replies; 5+ messages in thread From: Jarkko Sakkinen @ 2019-12-12 23:59 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx, Huang Haitao On Wed, Dec 11, 2019 at 08:07:55AM -0800, Sean Christopherson wrote: > On Wed, Dec 11, 2019 at 01:11:52PM +0200, Jarkko Sakkinen wrote: > > On Mon, Dec 09, 2019 at 12:52:55PM -0800, Sean Christopherson wrote: > > > Not a fan of making this dependent on -EIO, IMO invalidating iff EEXTEND > > > fails is cleaner. In other words, I still think killing the enclave on > > > on EADD failure is unnecessary. > > > > This comes down to whether you consider them as a transaction. I do > > and it makes a coherent API. > > What's your definition of transaction in this context? My interpretation > of transaction here would be that each ioctl() should either succeed, fail > without modifying persistent (enclave) state, or fail and kill the enclave > (because its state modifications are irreversible). > > EEXTEND falls into the last case because EADD can't be unwound. EADD falls > into the middle case because everything up to EADD can be cleanly undone. My definition is that if any of EADD/EEXTEND fails the enclave should be killed as there is no good reason to support that kind of use in any possible way. As long as it is documented, it is fine. /Jarkko ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-12 23:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-05 10:01 [PATCH] x86/sgx: Fix double-free when EADD fails Jarkko Sakkinen 2019-12-09 20:52 ` Sean Christopherson 2019-12-11 11:11 ` Jarkko Sakkinen 2019-12-11 16:07 ` Sean Christopherson 2019-12-12 23:59 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).