[intel-sgx-kernel-dev,2/3] intel_sgx: bug fixes for vm_insert_pfn in fault
diff mbox

Message ID 1483477647-12054-3-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson Jan. 3, 2017, 9:07 p.m. UTC
Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU
the last action in the fault handling sequence, which eleminates the
need to do EREMOVE of the page if vm_insert_pfn fails.  EREMOVE fails
if there are active threads in the enclave, i.e. the previous code
could result in kernel panics due to EREMOVE failure.

Update the return value if vm_insert_pfn fails in order to capture
vm_insert_pfn's error code.

Inserting the page before ELDU does not create a race condition
as accesses to the page will still #PF due to failing the EPCM
checks, i.e. user-visible behavior is identical whether an access
faults due to an invalid PTE or an invalid EPCM entry.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx_vma.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Jarkko Sakkinen Jan. 4, 2017, 1:49 p.m. UTC | #1
On Tue, Jan 03, 2017 at 01:07:26PM -0800, Sean Christopherson wrote:
> Swap the order of the calls to vm_insert_pfn and do_eldu to make ELDU
> the last action in the fault handling sequence, which eleminates the
> need to do EREMOVE of the page if vm_insert_pfn fails.  EREMOVE fails
> if there are active threads in the enclave, i.e. the previous code
> could result in kernel panics due to EREMOVE failure.
> 
> Update the return value if vm_insert_pfn fails in order to capture
> vm_insert_pfn's error code.
> 
> Inserting the page before ELDU does not create a race condition
> as accesses to the page will still #PF due to failing the EPCM
> checks, i.e. user-visible behavior is identical whether an access
> faults due to an invalid PTE or an invalid EPCM entry.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

NAK. I did not even read the code because the short description says
"bug fixes". A single patch per bug fix please.

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx_vma.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
> index e8a67b6..f356eed 100644
> --- a/drivers/platform/x86/intel_sgx_vma.c
> +++ b/drivers/platform/x86/intel_sgx_vma.c
> @@ -160,7 +160,6 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
>  	struct sgx_epc_page *epc_page = NULL;
>  	struct sgx_epc_page *secs_epc_page = NULL;
>  	struct page *backing;
> -	unsigned int free_flags = SGX_FREE_SKIP_EREMOVE;
>  	int rc;
>  
>  	/* If process was forked, VMA is still there but vm_private_data is set
> @@ -243,18 +242,19 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
>  		goto out;
>  	}
>  
> -	rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */);
> +	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
>  	if (rc) {
>  		sgx_put_backing(backing, 0);
>  		entry = ERR_PTR(rc);
>  		goto out;
>  	}
>  
> -	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
> +	rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */);
>  	sgx_put_backing(backing, 0);
>  
>  	if (rc) {
> -		free_flags = 0;
> +		zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
> +		entry = ERR_PTR(rc);
>  		goto out;
>  	}
>  
> @@ -273,7 +273,7 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
>  out:
>  	mutex_unlock(&encl->lock);
>  	if (epc_page)
> -		sgx_free_page(epc_page, encl, free_flags);
> +		sgx_free_page(epc_page, encl, SGX_FREE_SKIP_EREMOVE);
>  	if (secs_epc_page)
>  		sgx_free_page(secs_epc_page, encl, SGX_FREE_SKIP_EREMOVE);
>  	return entry;
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
index e8a67b6..f356eed 100644
--- a/drivers/platform/x86/intel_sgx_vma.c
+++ b/drivers/platform/x86/intel_sgx_vma.c
@@ -160,7 +160,6 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
 	struct sgx_epc_page *epc_page = NULL;
 	struct sgx_epc_page *secs_epc_page = NULL;
 	struct page *backing;
-	unsigned int free_flags = SGX_FREE_SKIP_EREMOVE;
 	int rc;
 
 	/* If process was forked, VMA is still there but vm_private_data is set
@@ -243,18 +242,19 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
 		goto out;
 	}
 
-	rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */);
+	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
 	if (rc) {
 		sgx_put_backing(backing, 0);
 		entry = ERR_PTR(rc);
 		goto out;
 	}
 
-	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
+	rc = do_eldu(encl, entry, epc_page, backing, false /* is_secs */);
 	sgx_put_backing(backing, 0);
 
 	if (rc) {
-		free_flags = 0;
+		zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
+		entry = ERR_PTR(rc);
 		goto out;
 	}
 
@@ -273,7 +273,7 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
 out:
 	mutex_unlock(&encl->lock);
 	if (epc_page)
-		sgx_free_page(epc_page, encl, free_flags);
+		sgx_free_page(epc_page, encl, SGX_FREE_SKIP_EREMOVE);
 	if (secs_epc_page)
 		sgx_free_page(secs_epc_page, encl, SGX_FREE_SKIP_EREMOVE);
 	return entry;