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

[Sean Christopherson <sean.j.christopherson@xxxxxxxxx>]

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@xxxxxxxxx>
> Reported-by: Huang Haitao <haitao.huang@xxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> ---
>  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
>