diff mbox

[intel-sgx-kernel-dev,v8,10/10] intel_sgx: migrate to radix tree for addressing enclave pages

Message ID 37306EFA9975BE469F115FDE982C075B9B6A92B2@ORSMSX108.amr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Christopherson Dec. 14, 2016, 7:21 p.m. UTC 
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> @@ -730,6 +705,14 @@ static int __encl_add_page(struct sgx_encl *encl,
>  		goto out;
>  	}
>  
> +	ret = radix_tree_insert(&encl->page_tree,
> +				encl_page->addr >> PAGE_SHIFT,
> +				&encl_page);
> +	if (ret) {
> +		sgx_put_backing(backing, false /* write */);
> +		goto out;
> +	}

The call to radix_tree_insert is broken, the third parameter should simply be "encl_page", not "&encl_page".  This results in a bogus pointer in the tree and manifests as sgx_vma_do_fault failing, usually due entry->epc_page being non-null.



> index 5520080..a9ea67f 100644
> --- a/drivers/platform/x86/intel_sgx_vma.c
> +++ b/drivers/platform/x86/intel_sgx_vma.c
> @@ -171,7 +171,7 @@ static struct sgx_encl_page *sgx_vma_do_fault(struct
> vm_area_struct *vma, if (!encl)
>  		return ERR_PTR(-EFAULT);
>  
> -	entry = sgx_encl_find_page(encl, addr);
> +	entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
>  	if (!entry)
>  		return ERR_PTR(-EFAULT);

The call to radix_tree_lookup needs to be done with encl->lock held.  This manifests as "Bus error (Core Dumped)" failures.



Suggested fix for the patch:

Comments

Jarkko Sakkinen Dec. 15, 2016, 7:21 a.m. UTC | #1 
On Wed, Dec 14, 2016 at 07:21:25PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > @@ -730,6 +705,14 @@ static int __encl_add_page(struct sgx_encl *encl,
> >  		goto out;
> >  	}
> >  
> > +	ret = radix_tree_insert(&encl->page_tree,
> > +				encl_page->addr >> PAGE_SHIFT,
> > +				&encl_page);
> > +	if (ret) {
> > +		sgx_put_backing(backing, false /* write */);
> > +		goto out;
> > +	}
> 
> The call to radix_tree_insert is broken, the third parameter should
> simply be "encl_page", not "&encl_page".  This results in a bogus
> pointer in the tree and manifests as sgx_vma_do_fault failing, usually
> due entry->epc_page being non-null.

It's weird that stress test passed that I run passed without issues.
Oh well. I guess I built the kernel without the patch applied. It cannot
be explained otherwise. The test launched hundreds of threads.

/Jarkko
Jarkko Sakkinen Dec. 15, 2016, 7:29 a.m. UTC | #2 
On Thu, Dec 15, 2016 at 09:21:36AM +0200, Jarkko Sakkinen wrote:
> On Wed, Dec 14, 2016 at 07:21:25PM +0000, Christopherson, Sean J wrote:
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > @@ -730,6 +705,14 @@ static int __encl_add_page(struct sgx_encl *encl,
> > >  		goto out;
> > >  	}
> > >  
> > > +	ret = radix_tree_insert(&encl->page_tree,
> > > +				encl_page->addr >> PAGE_SHIFT,
> > > +				&encl_page);
> > > +	if (ret) {
> > > +		sgx_put_backing(backing, false /* write */);
> > > +		goto out;
> > > +	}
> > 
> > The call to radix_tree_insert is broken, the third parameter should
> > simply be "encl_page", not "&encl_page".  This results in a bogus
> > pointer in the tree and manifests as sgx_vma_do_fault failing, usually
> > due entry->epc_page being non-null.
> 
> It's weird that stress test passed that I run passed without issues.
> Oh well. I guess I built the kernel without the patch applied. It cannot
> be explained otherwise. The test launched hundreds of threads.

Mystery resolved. I had a different version of this patch when I tested
:/

/Jarkko
diff mbox

Patch

diff --git a/drivers/platform/x86/intel_sgx_ioctl.c b/drivers/platform/x86/intel_sgx_ioctl.c
index ab31ba6..e090302 100644
--- a/drivers/platform/x86/intel_sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx_ioctl.c
@@ -707,7 +707,7 @@  static int __encl_add_page(struct sgx_encl *encl,

        ret = radix_tree_insert(&encl->page_tree,
                                encl_page->addr >> PAGE_SHIFT,
-                               &encl_page);
+                               encl_page);
        if (ret) {
                sgx_put_backing(backing, false /* write */);
                goto out;
diff --git a/drivers/platform/x86/intel_sgx_vma.c b/drivers/platform/x86/intel_sgx_vma.c
index a9ea67f..cee888d 100644
--- a/drivers/platform/x86/intel_sgx_vma.c
+++ b/drivers/platform/x86/intel_sgx_vma.c
@@ -171,10 +171,6 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
        if (!encl)
                return ERR_PTR(-EFAULT);

-       entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
-       if (!entry)
-               return ERR_PTR(-EFAULT);
-
        epc_page = sgx_alloc_page(encl->tgid_ctx, SGX_ALLOC_ATOMIC);
        if (IS_ERR(epc_page))
                /* reinterpret the type as we return an error */
@@ -193,6 +189,12 @@  static struct sgx_encl_page *sgx_vma_do_fault(struct vm_area_struct *vma,
                goto out;
        }

+       entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
+       if (!entry) {
+               entry = ERR_PTR(-EFAULT);
+               goto out;
+       }
+
        if (reserve && (entry->flags & SGX_ENCL_PAGE_RESERVED)) {
                sgx_dbg(encl, "cannot fault, 0x%p is reserved\n",
                        (void *)entry->addr);