[intel-sgx-kernel-dev,v8,08/10] intel_sgx: invalidate enclave when the user threads cease to exist
diff mbox

Message ID 20161208123828.21834-9-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Dec. 8, 2016, 12:38 p.m. UTC
Page table should not be manipulated after all user processes cease
to exist. This can result page table inconsistency errors:

BUG: non-zero nr_ptes on freeing mm: 1

This commit fixes the issue by invalidating the enclave after all
user processes have been died.

Reported-by: Sean Christopherson <sean.j.christopherson@intel.com>
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/platform/x86/Kconfig                |  1 +
 drivers/platform/x86/intel_sgx.h            |  2 ++
 drivers/platform/x86/intel_sgx_ioctl.c      | 23 +++++++++++++++++++++++
 drivers/platform/x86/intel_sgx_page_cache.c |  3 ++-
 drivers/platform/x86/intel_sgx_util.c       |  4 ++++
 5 files changed, 32 insertions(+), 1 deletion(-)

Comments

Sean Christopherson Dec. 13, 2016, 7:01 p.m. UTC | #1
On Thu, 2016-12-08 at 14:38 +0200, Jarkko Sakkinen wrote:
> Page table should not be manipulated after all user processes cease
> to exist. This can result page table inconsistency errors:
> 
> BUG: non-zero nr_ptes on freeing mm: 1
> 
> This commit fixes the issue by invalidating the enclave after all
> user processes have been died.
> 
> Reported-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---

> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> index f2a2ed1..49dd664 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -227,12 +227,13 @@ static bool sgx_ewb(struct sgx_encl *encl,
>  {
>  	int ret = __sgx_ewb(encl, entry, backing);
>  
> -	/* Only kick out threads with an IPI if needed. */
> +	/* fast path */
>  	if (ret == SGX_NOT_TRACKED) {
>  		smp_call_function(sgx_ipi_cb, NULL, 1);
>  		ret = __sgx_ewb(encl, entry, backing);
>  	}
>  
> +	/* slow path */
>  	if (ret) {
>  		/* Make enclave inaccessible. */
>  		sgx_invalidate(encl);

Nitpick, but can we remove the "fast path" and "slow path" comments?  The are potentially misleading as they seem to
imply that the "ret == SGX_NOT_TRACKED" case is the fast path and the fall-through is the slow path, whereas in my view
the first __sgx_ewb is the fast path, the IPI case is the slow path and sgx_invalidate is effectively an abort path.
Jarkko Sakkinen Dec. 13, 2016, 7:14 p.m. UTC | #2
On Tue, Dec 13, 2016 at 11:01:56AM -0800, Sean Christopherson wrote:
> On Thu, 2016-12-08 at 14:38 +0200, Jarkko Sakkinen wrote:
> > Page table should not be manipulated after all user processes cease
> > to exist. This can result page table inconsistency errors:
> > 
> > BUG: non-zero nr_ptes on freeing mm: 1
> > 
> > This commit fixes the issue by invalidating the enclave after all
> > user processes have been died.
> > 
> > Reported-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> > index f2a2ed1..49dd664 100644
> > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > @@ -227,12 +227,13 @@ static bool sgx_ewb(struct sgx_encl *encl,
> >  {
> >  	int ret = __sgx_ewb(encl, entry, backing);
> >  
> > -	/* Only kick out threads with an IPI if needed. */
> > +	/* fast path */
> >  	if (ret == SGX_NOT_TRACKED) {
> >  		smp_call_function(sgx_ipi_cb, NULL, 1);
> >  		ret = __sgx_ewb(encl, entry, backing);
> >  	}
> >  
> > +	/* slow path */
> >  	if (ret) {
> >  		/* Make enclave inaccessible. */
> >  		sgx_invalidate(encl);
> 
> Nitpick, but can we remove the "fast path" and "slow path" comments? 
> The are potentially misleading as they seem to imply that the "ret ==
> SGX_NOT_TRACKED" case is the fast path and the fall-through is the
> slow path, whereas in my view the first __sgx_ewb is the fast path,
> the IPI case is the slow path and sgx_invalidate is effectively an
> abort path.

You are absolutely right. They were incorrectly placed by me.
Thanks for noticing this. Given that the function does not have
much complexity at all they could be simply dropped.

/Jarkko
Jarkko Sakkinen Dec. 13, 2016, 7:16 p.m. UTC | #3
On Tue, Dec 13, 2016 at 09:14:02PM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 13, 2016 at 11:01:56AM -0800, Sean Christopherson wrote:
> > On Thu, 2016-12-08 at 14:38 +0200, Jarkko Sakkinen wrote:
> > > Page table should not be manipulated after all user processes cease
> > > to exist. This can result page table inconsistency errors:
> > > 
> > > BUG: non-zero nr_ptes on freeing mm: 1
> > > 
> > > This commit fixes the issue by invalidating the enclave after all
> > > user processes have been died.
> > > 
> > > Reported-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > > 
> > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> > > index f2a2ed1..49dd664 100644
> > > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > > @@ -227,12 +227,13 @@ static bool sgx_ewb(struct sgx_encl *encl,
> > >  {
> > >  	int ret = __sgx_ewb(encl, entry, backing);
> > >  
> > > -	/* Only kick out threads with an IPI if needed. */
> > > +	/* fast path */
> > >  	if (ret == SGX_NOT_TRACKED) {
> > >  		smp_call_function(sgx_ipi_cb, NULL, 1);
> > >  		ret = __sgx_ewb(encl, entry, backing);
> > >  	}
> > >  
> > > +	/* slow path */
> > >  	if (ret) {
> > >  		/* Make enclave inaccessible. */
> > >  		sgx_invalidate(encl);
> > 
> > Nitpick, but can we remove the "fast path" and "slow path" comments? 
> > The are potentially misleading as they seem to imply that the "ret ==
> > SGX_NOT_TRACKED" case is the fast path and the fall-through is the
> > slow path, whereas in my view the first __sgx_ewb is the fast path,
> > the IPI case is the slow path and sgx_invalidate is effectively an
> > abort path.
> 
> You are absolutely right. They were incorrectly placed by me.
> Thanks for noticing this. Given that the function does not have
> much complexity at all they could be simply dropped.

I do not consider this even a nitpick because they are in wrong
places. It's a valid point.

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index b8db914..f53a759 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1031,6 +1031,7 @@  config INTEL_SGX
 	tristate "Intel(R) SGX Driver"
 	default n
 	depends on X86
+	select MMU_NOTIFIER
 	---help---
 	Intel(R) SGX is a set of CPU instructions that can be used by
 	applications to set aside private regions of code and data.  The code
diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h
index 018cd03..f1ff194 100644
--- a/drivers/platform/x86/intel_sgx.h
+++ b/drivers/platform/x86/intel_sgx.h
@@ -67,6 +67,7 @@ 
 #include <linux/rwsem.h>
 #include <linux/sched.h>
 #include <linux/workqueue.h>
+#include <linux/mmu_notifier.h>
 
 #define SGX_EINIT_SPIN_COUNT	20
 #define SGX_EINIT_SLEEP_COUNT	50
@@ -151,6 +152,7 @@  struct sgx_encl {
 	struct sgx_encl_page secs_page;
 	struct sgx_tgid_ctx *tgid_ctx;
 	struct list_head encl_list;
+	struct mmu_notifier mmu_notifier;
 };
 
 extern struct workqueue_struct *sgx_add_page_wq;
diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
index d0113b0..aaccbc8 100644
--- a/drivers/platform/x86/intel_sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx_ioctl.c
@@ -471,6 +471,21 @@  static int sgx_init_page(struct sgx_encl *encl,
 	return 0;
 }
 
+static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
+				     struct mm_struct *mm)
+{
+	struct sgx_encl *encl =
+		container_of(mn, struct sgx_encl, mmu_notifier);
+
+	mutex_lock(&encl->lock);
+	encl->flags |= SGX_ENCL_DEAD;
+	mutex_unlock(&encl->lock);
+}
+
+static const struct mmu_notifier_ops sgx_mmu_notifier_ops = {
+	.release	= sgx_mmu_notifier_release,
+};
+
 /**
  * sgx_ioc_enclave_create - handler for SGX_IOC_ENCLAVE_CREATE
  *
@@ -572,6 +587,14 @@  static long sgx_ioc_enclave_create(struct file *filep, unsigned int cmd,
 	if (secs->flags & SGX_SECS_A_DEBUG)
 		encl->flags |= SGX_ENCL_DEBUG;
 
+
+	encl->mmu_notifier.ops = &sgx_mmu_notifier_ops;
+	ret = mmu_notifier_register(&encl->mmu_notifier, encl->mm);
+	if (ret) {
+		encl->mmu_notifier.ops = NULL;
+		goto out;
+	}
+
 	down_read(&current->mm->mmap_sem);
 	vma = find_vma(current->mm, secs->base);
 	if (!vma || vma->vm_ops != &sgx_vm_ops ||
diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
index f2a2ed1..49dd664 100644
--- a/drivers/platform/x86/intel_sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx_page_cache.c
@@ -227,12 +227,13 @@  static bool sgx_ewb(struct sgx_encl *encl,
 {
 	int ret = __sgx_ewb(encl, entry, backing);
 
-	/* Only kick out threads with an IPI if needed. */
+	/* fast path */
 	if (ret == SGX_NOT_TRACKED) {
 		smp_call_function(sgx_ipi_cb, NULL, 1);
 		ret = __sgx_ewb(encl, entry, backing);
 	}
 
+	/* slow path */
 	if (ret) {
 		/* Make enclave inaccessible. */
 		sgx_invalidate(encl);
diff --git a/drivers/platform/x86/intel_sgx_util.c b/drivers/platform/x86/intel_sgx_util.c
index d5238c2..f6f7dde0 100644
--- a/drivers/platform/x86/intel_sgx_util.c
+++ b/drivers/platform/x86/intel_sgx_util.c
@@ -237,6 +237,10 @@  void sgx_encl_release(struct kref *ref)
 
 	mutex_unlock(&sgx_tgid_ctx_mutex);
 
+	if (encl->mmu_notifier.ops)
+		mmu_notifier_unregister_no_release(&encl->mmu_notifier,
+						   encl->mm);
+
 	rb1 = rb_first(&encl->encl_rb);
 	while (rb1) {
 		entry = container_of(rb1, struct sgx_encl_page, node);