| Message ID | 20190827001128.25066-2-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Fix lock ordering bug w/ EADD | expand |
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
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.
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)))
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(-)