[intel-sgx-kernel-dev,v3] intel_sgx: kill encl if vm_insert_pfn fails in fault
diff mbox

Message ID 1491937510-11937-1-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson April 11, 2017, 7:05 p.m. UTC
Update the EPC page tracking immediately after sgx_eldu and kill the
encl if vm_insert_pfn fails.  Previously we tried to EREMOVE the EPC
page if vm_insert_pfn returned an error, but EREMOVE fails if there
are active cpus in the enclave, in which case the driver would lose
track of the EPC page.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx/sgx.h            |  3 ++-
 drivers/platform/x86/intel_sgx/sgx_main.c       |  2 +-
 drivers/platform/x86/intel_sgx/sgx_page_cache.c | 18 +++----------
 drivers/platform/x86/intel_sgx/sgx_util.c       | 36 ++++++++++++++++++++-----
 4 files changed, 36 insertions(+), 23 deletions(-)

Comments

Jarkko Sakkinen April 12, 2017, 8:40 p.m. UTC | #1
On Tue, Apr 11, 2017 at 12:05:10PM -0700, Sean Christopherson wrote:
> Update the EPC page tracking immediately after sgx_eldu and kill the
> encl if vm_insert_pfn fails.  Previously we tried to EREMOVE the EPC
> page if vm_insert_pfn returned an error, but EREMOVE fails if there
> are active cpus in the enclave, in which case the driver would lose
> track of the EPC page.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Thanks this makes a lot more sense.

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx/sgx.h            |  3 ++-
>  drivers/platform/x86/intel_sgx/sgx_main.c       |  2 +-
>  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 18 +++----------
>  drivers/platform/x86/intel_sgx/sgx_util.c       | 36 ++++++++++++++++++++-----
>  4 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
> index 82355bb..e59ff01 100644
> --- a/drivers/platform/x86/intel_sgx/sgx.h
> +++ b/drivers/platform/x86/intel_sgx/sgx.h
> @@ -198,7 +198,8 @@ int sgx_eremove(struct sgx_epc_page *epc_page);
>  struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr);
>  void sgx_zap_tcs_ptes(struct sgx_encl *encl,
>  		      struct vm_area_struct *vma);
> -void sgx_invalidate(struct sgx_encl *encl);
> +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus);
> +void sgx_flush_cpus(struct sgx_encl *encl);
>  int sgx_find_encl(struct mm_struct *mm, unsigned long addr,
>  		  struct vm_area_struct **vma);
>  
> diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c
> index b2a1bc4..5890643 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_main.c
> @@ -243,7 +243,7 @@ static int sgx_pm_suspend(struct device *dev)
>  
>  	list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) {
>  		list_for_each_entry(encl, &ctx->encl_list, encl_list) {
> -			sgx_invalidate(encl);
> +			sgx_invalidate(encl, false);
>  			encl->flags |= SGX_ENCL_SUSPEND;
>  			flush_work(&encl->add_page_work);
>  		}
> diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> index 4ffde27..59a67cb 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> @@ -221,15 +221,6 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
>  	mutex_unlock(&encl->lock);
>  }
>  
> -static void sgx_ipi_cb(void *info)
> -{
> -}
> -
> -static void sgx_flush_cpus(struct sgx_encl *encl)
> -{
> -	on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1);
> -}
> -
>  static void sgx_eblock(struct sgx_encl *encl,
>  		       struct sgx_epc_page *epc_page)
>  {
> @@ -242,8 +233,7 @@ static void sgx_eblock(struct sgx_encl *encl,
>  
>  	if (ret) {
>  		sgx_crit(encl, "EBLOCK returned %d\n", ret);
> -		sgx_invalidate(encl);
> -		sgx_flush_cpus(encl);
> +		sgx_invalidate(encl, true);
>  	}
>  
>  }
> @@ -259,8 +249,7 @@ static void sgx_etrack(struct sgx_encl *encl)
>  
>  	if (ret) {
>  		sgx_crit(encl, "ETRACK returned %d\n", ret);
> -		sgx_invalidate(encl);
> -		sgx_flush_cpus(encl);
> +		sgx_invalidate(encl, true);
>  	}
>  }
>  
> @@ -327,8 +316,7 @@ static bool sgx_ewb(struct sgx_encl *encl,
>  
>  	if (ret) {
>  		/* make enclave inaccessible */
> -		sgx_invalidate(encl);
> -		sgx_flush_cpus(encl);
> +		sgx_invalidate(encl, true);
>  		if (ret > 0)
>  			sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
>  		return false;
> diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
> index 25e949d..1e9fa18 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_util.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_util.c
> @@ -120,7 +120,7 @@ void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma)
>  	}
>  }
>  
> -void sgx_invalidate(struct sgx_encl *encl)
> +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus)
>  {
>  	struct vm_area_struct *vma;
>  	unsigned long addr;
> @@ -135,6 +135,18 @@ void sgx_invalidate(struct sgx_encl *encl)
>  	}
>  
>  	encl->flags |= SGX_ENCL_DEAD;
> +
> +	if (flush_cpus)
> +		sgx_flush_cpus(encl);
> +}
> +
> +static void sgx_ipi_cb(void *info)
> +{
> +}
> +
> +void sgx_flush_cpus(struct sgx_encl *encl)
> +{
> +	on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1);
>  }
>  
>  /**
> @@ -315,10 +327,12 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  	if (rc)
>  		goto out;
>  
> -	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
> -	if (rc)
> -		goto out;
> -
> +	/* Track the EPC page even if vm_insert_pfn fails; we need to ensure
> +	 * the EPC page is properly freed and we can't do EREMOVE right away
> +	 * because EREMOVE may fail due to an active cpu in the enclave.  We
> +	 * can't call vm_insert_pfn before sgx_eldu because SKL signals #GP
> +	 * instead of #PF if the EPC page is invalid.
> +	 */
>  	encl->secs_child_cnt++;
>  
>  	entry->epc_page = epc_page;
> @@ -328,9 +342,19 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  
>  	/* Do not free */
>  	epc_page = NULL;
> +	list_add_tail(&entry->load_list, &encl->load_list);
> +
> +	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa));
> +	if (rc) {
> +		/* Kill the enclave if vm_insert_pfn fails; failure only occurs
> +		 * if there is a driver bug or an unrecoverable issue, e.g. OOM.
> +		 */
> +		sgx_crit(encl, "vm_insert_pfn returned %d\n", rc);
> +		sgx_invalidate(encl, true);
> +		goto out;
> +	}
>  
>  	sgx_test_and_clear_young(entry, encl);
> -	list_add_tail(&entry->load_list, &encl->load_list);
>  out:
>  	mutex_unlock(&encl->lock);
>  	if (epc_page)
> -- 
> 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
Jarkko Sakkinen April 13, 2017, 12:55 p.m. UTC | #2
On Tue, Apr 11, 2017 at 12:05:10PM -0700, Sean Christopherson wrote:
> Update the EPC page tracking immediately after sgx_eldu and kill the
> encl if vm_insert_pfn fails.  Previously we tried to EREMOVE the EPC
> page if vm_insert_pfn returned an error, but EREMOVE fails if there
> are active cpus in the enclave, in which case the driver would lose
> track of the EPC page.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Thanks. Applied

> ---
>  drivers/platform/x86/intel_sgx/sgx.h            |  3 ++-
>  drivers/platform/x86/intel_sgx/sgx_main.c       |  2 +-
>  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 18 +++----------
>  drivers/platform/x86/intel_sgx/sgx_util.c       | 36 ++++++++++++++++++++-----
>  4 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
> index 82355bb..e59ff01 100644
> --- a/drivers/platform/x86/intel_sgx/sgx.h
> +++ b/drivers/platform/x86/intel_sgx/sgx.h
> @@ -198,7 +198,8 @@ int sgx_eremove(struct sgx_epc_page *epc_page);
>  struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr);
>  void sgx_zap_tcs_ptes(struct sgx_encl *encl,
>  		      struct vm_area_struct *vma);
> -void sgx_invalidate(struct sgx_encl *encl);
> +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus);
> +void sgx_flush_cpus(struct sgx_encl *encl);
>  int sgx_find_encl(struct mm_struct *mm, unsigned long addr,
>  		  struct vm_area_struct **vma);
>  
> diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c
> index b2a1bc4..5890643 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_main.c
> @@ -243,7 +243,7 @@ static int sgx_pm_suspend(struct device *dev)
>  
>  	list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) {
>  		list_for_each_entry(encl, &ctx->encl_list, encl_list) {
> -			sgx_invalidate(encl);
> +			sgx_invalidate(encl, false);
>  			encl->flags |= SGX_ENCL_SUSPEND;
>  			flush_work(&encl->add_page_work);
>  		}
> diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> index 4ffde27..59a67cb 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> @@ -221,15 +221,6 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
>  	mutex_unlock(&encl->lock);
>  }
>  
> -static void sgx_ipi_cb(void *info)
> -{
> -}
> -
> -static void sgx_flush_cpus(struct sgx_encl *encl)
> -{
> -	on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1);
> -}
> -
>  static void sgx_eblock(struct sgx_encl *encl,
>  		       struct sgx_epc_page *epc_page)
>  {
> @@ -242,8 +233,7 @@ static void sgx_eblock(struct sgx_encl *encl,
>  
>  	if (ret) {
>  		sgx_crit(encl, "EBLOCK returned %d\n", ret);
> -		sgx_invalidate(encl);
> -		sgx_flush_cpus(encl);
> +		sgx_invalidate(encl, true);
>  	}
>  
>  }
> @@ -259,8 +249,7 @@ static void sgx_etrack(struct sgx_encl *encl)
>  
>  	if (ret) {
>  		sgx_crit(encl, "ETRACK returned %d\n", ret);
> -		sgx_invalidate(encl);
> -		sgx_flush_cpus(encl);
> +		sgx_invalidate(encl, true);
>  	}
>  }
>  
> @@ -327,8 +316,7 @@ static bool sgx_ewb(struct sgx_encl *encl,
>  
>  	if (ret) {
>  		/* make enclave inaccessible */
> -		sgx_invalidate(encl);
> -		sgx_flush_cpus(encl);
> +		sgx_invalidate(encl, true);
>  		if (ret > 0)
>  			sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
>  		return false;
> diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
> index 25e949d..1e9fa18 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_util.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_util.c
> @@ -120,7 +120,7 @@ void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma)
>  	}
>  }
>  
> -void sgx_invalidate(struct sgx_encl *encl)
> +void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus)
>  {
>  	struct vm_area_struct *vma;
>  	unsigned long addr;
> @@ -135,6 +135,18 @@ void sgx_invalidate(struct sgx_encl *encl)
>  	}
>  
>  	encl->flags |= SGX_ENCL_DEAD;
> +
> +	if (flush_cpus)
> +		sgx_flush_cpus(encl);
> +}
> +
> +static void sgx_ipi_cb(void *info)
> +{
> +}
> +
> +void sgx_flush_cpus(struct sgx_encl *encl)
> +{
> +	on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1);
>  }
>  
>  /**
> @@ -315,10 +327,12 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  	if (rc)
>  		goto out;
>  
> -	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
> -	if (rc)
> -		goto out;
> -
> +	/* Track the EPC page even if vm_insert_pfn fails; we need to ensure
> +	 * the EPC page is properly freed and we can't do EREMOVE right away
> +	 * because EREMOVE may fail due to an active cpu in the enclave.  We
> +	 * can't call vm_insert_pfn before sgx_eldu because SKL signals #GP
> +	 * instead of #PF if the EPC page is invalid.
> +	 */
>  	encl->secs_child_cnt++;
>  
>  	entry->epc_page = epc_page;
> @@ -328,9 +342,19 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  
>  	/* Do not free */
>  	epc_page = NULL;
> +	list_add_tail(&entry->load_list, &encl->load_list);
> +
> +	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa));
> +	if (rc) {
> +		/* Kill the enclave if vm_insert_pfn fails; failure only occurs
> +		 * if there is a driver bug or an unrecoverable issue, e.g. OOM.
> +		 */
> +		sgx_crit(encl, "vm_insert_pfn returned %d\n", rc);
> +		sgx_invalidate(encl, true);
> +		goto out;
> +	}
>  
>  	sgx_test_and_clear_young(entry, encl);
> -	list_add_tail(&entry->load_list, &encl->load_list);
>  out:
>  	mutex_unlock(&encl->lock);
>  	if (epc_page)
> -- 
> 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/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
index 82355bb..e59ff01 100644
--- a/drivers/platform/x86/intel_sgx/sgx.h
+++ b/drivers/platform/x86/intel_sgx/sgx.h
@@ -198,7 +198,8 @@  int sgx_eremove(struct sgx_epc_page *epc_page);
 struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr);
 void sgx_zap_tcs_ptes(struct sgx_encl *encl,
 		      struct vm_area_struct *vma);
-void sgx_invalidate(struct sgx_encl *encl);
+void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus);
+void sgx_flush_cpus(struct sgx_encl *encl);
 int sgx_find_encl(struct mm_struct *mm, unsigned long addr,
 		  struct vm_area_struct **vma);
 
diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c
index b2a1bc4..5890643 100644
--- a/drivers/platform/x86/intel_sgx/sgx_main.c
+++ b/drivers/platform/x86/intel_sgx/sgx_main.c
@@ -243,7 +243,7 @@  static int sgx_pm_suspend(struct device *dev)
 
 	list_for_each_entry(ctx, &sgx_tgid_ctx_list, list) {
 		list_for_each_entry(encl, &ctx->encl_list, encl_list) {
-			sgx_invalidate(encl);
+			sgx_invalidate(encl, false);
 			encl->flags |= SGX_ENCL_SUSPEND;
 			flush_work(&encl->add_page_work);
 		}
diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
index 4ffde27..59a67cb 100644
--- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
@@ -221,15 +221,6 @@  static void sgx_isolate_pages(struct sgx_encl *encl,
 	mutex_unlock(&encl->lock);
 }
 
-static void sgx_ipi_cb(void *info)
-{
-}
-
-static void sgx_flush_cpus(struct sgx_encl *encl)
-{
-	on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1);
-}
-
 static void sgx_eblock(struct sgx_encl *encl,
 		       struct sgx_epc_page *epc_page)
 {
@@ -242,8 +233,7 @@  static void sgx_eblock(struct sgx_encl *encl,
 
 	if (ret) {
 		sgx_crit(encl, "EBLOCK returned %d\n", ret);
-		sgx_invalidate(encl);
-		sgx_flush_cpus(encl);
+		sgx_invalidate(encl, true);
 	}
 
 }
@@ -259,8 +249,7 @@  static void sgx_etrack(struct sgx_encl *encl)
 
 	if (ret) {
 		sgx_crit(encl, "ETRACK returned %d\n", ret);
-		sgx_invalidate(encl);
-		sgx_flush_cpus(encl);
+		sgx_invalidate(encl, true);
 	}
 }
 
@@ -327,8 +316,7 @@  static bool sgx_ewb(struct sgx_encl *encl,
 
 	if (ret) {
 		/* make enclave inaccessible */
-		sgx_invalidate(encl);
-		sgx_flush_cpus(encl);
+		sgx_invalidate(encl, true);
 		if (ret > 0)
 			sgx_err(encl, "EWB returned %d, enclave killed\n", ret);
 		return false;
diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
index 25e949d..1e9fa18 100644
--- a/drivers/platform/x86/intel_sgx/sgx_util.c
+++ b/drivers/platform/x86/intel_sgx/sgx_util.c
@@ -120,7 +120,7 @@  void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma)
 	}
 }
 
-void sgx_invalidate(struct sgx_encl *encl)
+void sgx_invalidate(struct sgx_encl *encl, bool flush_cpus)
 {
 	struct vm_area_struct *vma;
 	unsigned long addr;
@@ -135,6 +135,18 @@  void sgx_invalidate(struct sgx_encl *encl)
 	}
 
 	encl->flags |= SGX_ENCL_DEAD;
+
+	if (flush_cpus)
+		sgx_flush_cpus(encl);
+}
+
+static void sgx_ipi_cb(void *info)
+{
+}
+
+void sgx_flush_cpus(struct sgx_encl *encl)
+{
+	on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1);
 }
 
 /**
@@ -315,10 +327,12 @@  static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
 	if (rc)
 		goto out;
 
-	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(epc_page->pa));
-	if (rc)
-		goto out;
-
+	/* Track the EPC page even if vm_insert_pfn fails; we need to ensure
+	 * the EPC page is properly freed and we can't do EREMOVE right away
+	 * because EREMOVE may fail due to an active cpu in the enclave.  We
+	 * can't call vm_insert_pfn before sgx_eldu because SKL signals #GP
+	 * instead of #PF if the EPC page is invalid.
+	 */
 	encl->secs_child_cnt++;
 
 	entry->epc_page = epc_page;
@@ -328,9 +342,19 @@  static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
 
 	/* Do not free */
 	epc_page = NULL;
+	list_add_tail(&entry->load_list, &encl->load_list);
+
+	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa));
+	if (rc) {
+		/* Kill the enclave if vm_insert_pfn fails; failure only occurs
+		 * if there is a driver bug or an unrecoverable issue, e.g. OOM.
+		 */
+		sgx_crit(encl, "vm_insert_pfn returned %d\n", rc);
+		sgx_invalidate(encl, true);
+		goto out;
+	}
 
 	sgx_test_and_clear_young(entry, encl);
-	list_add_tail(&entry->load_list, &encl->load_list);
 out:
 	mutex_unlock(&encl->lock);
 	if (epc_page)