[intel-sgx-kernel-dev,3/4] intel_sgx: Avoid EREMOVE in fail path during fault
diff mbox

Message ID 1483544024-6154-4-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson Jan. 4, 2017, 3:33 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.

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 | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Jarkko Sakkinen Jan. 10, 2017, 12:10 p.m. UTC | #1
On Wed, Jan 04, 2017 at 07:33:43AM -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.
> 
> 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>

Thanks this much better now.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Commit log really is the only valid documentation for most of the
time...

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx_vma.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
> index e670405..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,18 @@ 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;
>  	}
> @@ -274,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
Sean Christopherson Jan. 10, 2017, 6:40 p.m. UTC | #2
On Tue, 2017-01-10 at 14:10 +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 04, 2017 at 07:33:43AM -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.
> > 
> > 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>
> Thanks this much better now.
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> Commit log really is the only valid documentation for most of the
> time...
> 
> /Jarkko
> 

Egad!  Don't merge this commit, performing do_eldu before vm_insert_pfn does
break the driver.  The processor will signal a #GP(0) instead of the expected
#PF if the page is accessed after inserting the PFN but before ELDU.

> > 
> > ---
> >  drivers/platform/x86/intel_sgx_vma.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_vma.c
> > b/drivers/platform/x86/intel_sgx_vma.c
> > index e670405..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,18 @@ 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;
> >  	}
> > @@ -274,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;
Jarkko Sakkinen Jan. 11, 2017, 11:28 a.m. UTC | #3
On Tue, Jan 10, 2017 at 10:40:17AM -0800, Sean Christopherson wrote:
> On Tue, 2017-01-10 at 14:10 +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 04, 2017 at 07:33:43AM -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.
> > > 
> > > 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>
> > Thanks this much better now.
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > Commit log really is the only valid documentation for most of the
> > time...
> > 
> > /Jarkko
> > 
> 
> Egad!  Don't merge this commit, performing do_eldu before vm_insert_pfn does
> break the driver.  The processor will signal a #GP(0) instead of the expected
> #PF if the page is accessed after inserting the PFN but before ELDU.

Hey, it's in my master but I'll drop it and leave rest of the
three patches. Not much harm done as we are not in the mainline
:-)

I'll do it within couple of days as this is not really critical
at this point.

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
index e670405..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,18 @@  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;
 	}
@@ -274,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;