[intel-sgx-kernel-dev,2/6] intel_sgx: Lock the enlcave when updating va_pages
diff mbox

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

Commit Message

Sean Christopherson Dec. 22, 2016, 8:15 p.m. UTC
The unlocked modification of encl->va_pages can cause segfaults if
two or more threads trigger a VA page allocation at the same time.

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

Comments

Jarkko Sakkinen Dec. 29, 2016, 12:55 p.m. UTC | #1
On Thu, Dec 22, 2016 at 12:15:31PM -0800, Sean Christopherson wrote:
> The unlocked modification of encl->va_pages can cause segfaults if
> two or more threads trigger a VA page allocation at the same time.

Rather take the lock one step earlier in __encl_add_page.

/Jarkko

> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  drivers/platform/x86/intel_sgx_ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
> index 3a4a8fa..b78c552 100644
> --- a/drivers/platform/x86/intel_sgx_ioctl.c
> +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> @@ -435,7 +435,10 @@ static int sgx_init_page(struct sgx_encl *encl,
>  
>  		va_page->epc_page = epc_page;
>  		va_offset = sgx_alloc_va_slot(va_page);
> +
> +		mutex_lock(&encl->lock);
>  		list_add(&va_page->list, &encl->va_pages);
> +		mutex_unlock(&encl->lock);
>  	}
>  
>  	entry->va_page = va_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
Sean Christopherson Jan. 3, 2017, 4:35 p.m. UTC | #2
On Thu, 2016-12-29 at 14:55 +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 22, 2016 at 12:15:31PM -0800, Sean Christopherson wrote:
> > 
> > The unlocked modification of encl->va_pages can cause segfaults if
> > two or more threads trigger a VA page allocation at the same time.
> Rather take the lock one step earlier in __encl_add_page.
> 
> /Jarkko
> 
> > 
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  drivers/platform/x86/intel_sgx_ioctl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
> > index 3a4a8fa..b78c552 100644
> > --- a/drivers/platform/x86/intel_sgx_ioctl.c
> > +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> > @@ -435,7 +435,10 @@ static int sgx_init_page(struct sgx_encl *encl,
> >  
> >  		va_page->epc_page = epc_page;
> >  		va_offset = sgx_alloc_va_slot(va_page);
> > +
> > +		mutex_lock(&encl->lock);
> >  		list_add(&va_page->list, &encl->va_pages);
> > +		mutex_unlock(&encl->lock);
> >  	}
> >  
> >  	entry->va_page = va_page;

Allocating a VA page involves calling sgx_alloc_page, which cannot be done while holding encl->lock.  A later patch in
the series, which allocates VA slots at eviction time and reserves VA pages at creation time, makes the on-demand
locking a little less awkward as the sgx_init_page function becomes sgx_alloc_va_page, i.e. it's more obvious why encl-
>lock cannot be held.
Jarkko Sakkinen Jan. 3, 2017, 5:28 p.m. UTC | #3
On Tue, Jan 03, 2017 at 08:35:54AM -0800, Sean Christopherson wrote:
> On Thu, 2016-12-29 at 14:55 +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 22, 2016 at 12:15:31PM -0800, Sean Christopherson wrote:
> > > 
> > > The unlocked modification of encl->va_pages can cause segfaults if
> > > two or more threads trigger a VA page allocation at the same time.
> > Rather take the lock one step earlier in __encl_add_page.
> > 
> > /Jarkko
> > 
> > > 
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > ---
> > >  drivers/platform/x86/intel_sgx_ioctl.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
> > > index 3a4a8fa..b78c552 100644
> > > --- a/drivers/platform/x86/intel_sgx_ioctl.c
> > > +++ b/drivers/platform/x86/intel_sgx_ioctl.c
> > > @@ -435,7 +435,10 @@ static int sgx_init_page(struct sgx_encl *encl,
> > >  
> > >  		va_page->epc_page = epc_page;
> > >  		va_offset = sgx_alloc_va_slot(va_page);
> > > +
> > > +		mutex_lock(&encl->lock);
> > >  		list_add(&va_page->list, &encl->va_pages);
> > > +		mutex_unlock(&encl->lock);
> > >  	}
> > >  
> > >  	entry->va_page = va_page;
> 
> Allocating a VA page involves calling sgx_alloc_page, which cannot be done while holding encl->lock.  A later patch in
> the series, which allocates VA slots at eviction time and reserves VA pages at creation time, makes the on-demand
> locking a little less awkward as the sgx_init_page function becomes sgx_alloc_va_page, i.e. it's more obvious why encl-
> >lock cannot be held.

I overlooked when i reviewed this so this is fine. I'm not
sure what you mean it being more obvious in a later patch?
Why it is less obvious in this?

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
index 3a4a8fa..b78c552 100644
--- a/drivers/platform/x86/intel_sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx_ioctl.c
@@ -435,7 +435,10 @@  static int sgx_init_page(struct sgx_encl *encl,
 
 		va_page->epc_page = epc_page;
 		va_offset = sgx_alloc_va_slot(va_page);
+
+		mutex_lock(&encl->lock);
 		list_add(&va_page->list, &encl->va_pages);
+		mutex_unlock(&encl->lock);
 	}
 
 	entry->va_page = va_page;