[1/4] x86/sgx: Ensure enclave state is visible before marking it created
diff mbox series

Message ID 20190827001128.25066-2-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/sgx: Fix lock ordering bug w/ EADD
Related show

Commit Message

Sean Christopherson Aug. 27, 2019, 12:11 a.m. UTC
Add a memory barrier pair to ensure all enclave state is visible in
memory prior to SGX_ENCL_CREATED being set.  Without the barries, adding
pages and/or initializing the enclaves could theoretically consume stale
data.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/ioctl.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Jarkko Sakkinen Aug. 27, 2019, 11:20 a.m. UTC | #1
On Mon, Aug 26, 2019 at 05:11:25PM -0700, Sean Christopherson wrote:
> Add a memory barrier pair to ensure all enclave state is visible in
> memory prior to SGX_ENCL_CREATED being set.  Without the barries, adding
> pages and/or initializing the enclaves could theoretically consume stale
> data.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/ioctl.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 911ff3b0f061..7134d68aecb3 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -163,6 +163,15 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
>  	return encl_page;
>  }
>  
> +static bool is_encl_created(struct sgx_encl *encl)
> +{
> +	bool created = encl->flags & SGX_ENCL_CREATED;
> +
> +	/* Pairs with smp_wmb() in sgx_encl_create(). */
> +	smp_rmb();
> +	return created;
> +}

what if you just convert the flags to atomic_t? That would fix this
issue and would prevent analogous issues from occuring.

/Jarkko
Sean Christopherson Aug. 27, 2019, 4:42 p.m. UTC | #2
On Tue, Aug 27, 2019 at 02:20:44PM +0300, Jarkko Sakkinen wrote:
> On Mon, Aug 26, 2019 at 05:11:25PM -0700, Sean Christopherson wrote:
> > Add a memory barrier pair to ensure all enclave state is visible in
> > memory prior to SGX_ENCL_CREATED being set.  Without the barries, adding
> > pages and/or initializing the enclaves could theoretically consume stale
> > data.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/ioctl.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> > index 911ff3b0f061..7134d68aecb3 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -163,6 +163,15 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
> >  	return encl_page;
> >  }
> >  
> > +static bool is_encl_created(struct sgx_encl *encl)
> > +{
> > +	bool created = encl->flags & SGX_ENCL_CREATED;
> > +
> > +	/* Pairs with smp_wmb() in sgx_encl_create(). */
> > +	smp_rmb();
> > +	return created;
> > +}
> 
> what if you just convert the flags to atomic_t? That would fix this
> issue and would prevent analogous issues from occuring.

I thought about that too, but originally discarded the idea because I
was worried doing so would negatively impact the other uses of flags.
After actually implementing the change, I think the positives outweigh
the negatives, so I'll send a v2 with this suggestion.

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index 911ff3b0f061..7134d68aecb3 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -163,6 +163,15 @@  static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 	return encl_page;
 }
 
+static bool is_encl_created(struct sgx_encl *encl)
+{
+	bool created = encl->flags & SGX_ENCL_CREATED;
+
+	/* Pairs with smp_wmb() in sgx_encl_create(). */
+	smp_rmb();
+	return created;
+}
+
 static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 {
 	unsigned long encl_size = secs->size + PAGE_SIZE;
@@ -231,8 +240,9 @@  static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	/*
 	 * Set SGX_ENCL_CREATED only after the enclave is fully prepped.  This
 	 * allows other flows to check if the enclave has been created without
-	 * taking encl->lock.
+	 * taking encl->lock.  Pairs with smp_rmb() in is_encl_created().
 	 */
+	smp_wmb();
 	encl->flags |= SGX_ENCL_CREATED;
 
 	mutex_unlock(&encl->lock);
@@ -478,7 +488,7 @@  static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 	struct sgx_enclave_add_page addp;
 	struct sgx_secinfo secinfo;
 
-	if (!(encl->flags & SGX_ENCL_CREATED))
+	if (!is_encl_created(encl))
 		return -EINVAL;
 
 	if (copy_from_user(&addp, arg, sizeof(addp)))
@@ -611,7 +621,7 @@  static long sgx_ioc_enclave_init(struct file *filep, void __user *arg)
 	struct page *initp_page;
 	int ret;
 
-	if (!(encl->flags & SGX_ENCL_CREATED))
+	if (!is_encl_created(encl))
 		return -EINVAL;
 
 	if (copy_from_user(&einit, arg, sizeof(einit)))