From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org, Huang Haitao <haitao.huang@intel.com>
Subject: Re: [PATCH] x86/sgx: Fix double-free when EADD fails
Date: Mon, 9 Dec 2019 12:52:55 -0800 [thread overview]
Message-ID: <20191209205254.GE4042@linux.intel.com> (raw)
In-Reply-To: <20191205100151.18950-1-jarkko.sakkinen@linux.intel.com>
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
>
next prev parent reply other threads:[~2019-12-09 21:22 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-05 10:01 [PATCH] x86/sgx: Fix double-free when EADD fails Jarkko Sakkinen
2019-12-09 20:52 ` Sean Christopherson [this message]
2019-12-11 11:11 ` Jarkko Sakkinen
2019-12-11 16:07 ` Sean Christopherson
2019-12-12 23:59 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191209205254.GE4042@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=haitao.huang@intel.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=linux-sgx@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).