[PATCH] x86/sgx: Fix double-free when EADD fails

[PATCH] x86/sgx: Fix double-free when EADD fails

[Jarkko Sakkinen]

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(&current->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

Re: [PATCH] x86/sgx: Fix double-free when EADD fails

[Sean Christopherson]

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(&current->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
> 

Re: [PATCH] x86/sgx: Fix double-free when EADD fails

[Jarkko Sakkinen]

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(&current->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

Re: [PATCH] x86/sgx: Fix double-free when EADD fails

[Sean Christopherson]

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.

Re: [PATCH] x86/sgx: Fix double-free when EADD fails

[Jarkko Sakkinen]

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