[for_v22,07/11] x86/sgx: Check that enclave is created at beginning of EADD/EINIT ioctl
diff mbox series

Message ID 20190808001254.11926-8-sean.j.christopherson@intel.com
State New
Headers show
Series
  • x86/sgx: Bug fixes for v22
Related show

Commit Message

Sean Christopherson Aug. 8, 2019, 12:12 a.m. UTC
Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
the ioctl() flows.  Deferring the check until the core code is fragile
as all code leading up to that point must be careful that it only uses
members of @encl that are initialized at allocation time.  For example,
the flush_work() call in sgx_encl_init() will crash if the enclave has
not been created.

Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
long as SGX_ENCL_CREATED is set only after the enclave is fully
initialized, it's not the kernel's responsibility to guard against
sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
the dependency.

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

Comments

Jarkko Sakkinen Aug. 8, 2019, 3:47 p.m. UTC | #1
On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> the ioctl() flows.  Deferring the check until the core code is fragile
> as all code leading up to that point must be careful that it only uses
> members of @encl that are initialized at allocation time.  For example,
> the flush_work() call in sgx_encl_init() will crash if the enclave has
> not been created.
> 
> Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> long as SGX_ENCL_CREATED is set only after the enclave is fully
> initialized, it's not the kernel's responsibility to guard against
> sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> the dependency.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
Jarkko Sakkinen Aug. 9, 2019, 11:40 p.m. UTC | #2
On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> the ioctl() flows.  Deferring the check until the core code is fragile
> as all code leading up to that point must be careful that it only uses
> members of @encl that are initialized at allocation time.  For example,
> the flush_work() call in sgx_encl_init() will crash if the enclave has
> not been created.
> 
> Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> long as SGX_ENCL_CREATED is set only after the enclave is fully
> initialized, it's not the kernel's responsibility to guard against
> sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> the dependency.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

07/11, 08/11 and 09/11 have been squashed and pushed.

I'm now observing this kind of behavior with the self-test:

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
ECREATE failed rc=-1, err=22.

jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
$ sudo tools/testing/selftests/x86/sgx/test_sgx
Binary size 24576 (0x6000), SIGSTRUCT size 1808
Loading the enclave.
Input: 0x1122334455667788
Output: 0x1122334455667788

Any ideas what could cause this? This happen with the current master.

/Jarkko
Sean Christopherson Aug. 10, 2019, 12:03 a.m. UTC | #3
On Sat, Aug 10, 2019 at 02:40:32AM +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> > Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> > the ioctl() flows.  Deferring the check until the core code is fragile
> > as all code leading up to that point must be careful that it only uses
> > members of @encl that are initialized at allocation time.  For example,
> > the flush_work() call in sgx_encl_init() will crash if the enclave has
> > not been created.
> > 
> > Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> > long as SGX_ENCL_CREATED is set only after the enclave is fully
> > initialized, it's not the kernel's responsibility to guard against
> > sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> > the dependency.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> 07/11, 08/11 and 09/11 have been squashed and pushed.
> 
> I'm now observing this kind of behavior with the self-test:
> 
> jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
> $ sudo tools/testing/selftests/x86/sgx/test_sgx
> Binary size 24576 (0x6000), SIGSTRUCT size 1808
> Loading the enclave.
> ECREATE failed rc=-1, err=22.

Doh, I forgot to update/test the selftest.  I'll do so now.
Sean Christopherson Aug. 10, 2019, 12:10 a.m. UTC | #4
On Fri, Aug 09, 2019 at 05:03:55PM -0700, Sean Christopherson wrote:
> On Sat, Aug 10, 2019 at 02:40:32AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Aug 07, 2019 at 05:12:50PM -0700, Sean Christopherson wrote:
> > > Move the EADD/EINIT checks on SGX_ENCL_CREATED to the very beginning of
> > > the ioctl() flows.  Deferring the check until the core code is fragile
> > > as all code leading up to that point must be careful that it only uses
> > > members of @encl that are initialized at allocation time.  For example,
> > > the flush_work() call in sgx_encl_init() will crash if the enclave has
> > > not been created.
> > > 
> > > Note, there is no need to take encl->lock to check SGX_ENCL_CREATED so
> > > long as SGX_ENCL_CREATED is set only after the enclave is fully
> > > initialized, it's not the kernel's responsibility to guard against
> > > sgx_encl_create() racing with EADD/EINIT.  Add a comment to highlight
> > > the dependency.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > 07/11, 08/11 and 09/11 have been squashed and pushed.
> > 
> > I'm now observing this kind of behavior with the self-test:
> > 
> > jsakkine at jsakkine-lab2 in ~/devel/linux-sgx (master●)
> > $ sudo tools/testing/selftests/x86/sgx/test_sgx
> > Binary size 24576 (0x6000), SIGSTRUCT size 1808
> > Loading the enclave.
> > ECREATE failed rc=-1, err=22.
> 
> Doh, I forgot to update/test the selftest.  I'll do so now.

Oh, it's probably due to changing sgx_get_unmapped_area() to not align
the address, i.e. the selftest is likely passing in an unaligned ELRANGE.

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 6a580361e20e..700d65c96b9a 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -326,6 +326,12 @@  static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	encl->base = secs->base;
 	encl->size = secs->size;
 	encl->ssaframesize = secs->ssa_frame_size;
+
+	/*
+	 * 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.
+	 */
 	encl->flags |= SGX_ENCL_CREATED;
 
 	mutex_unlock(&encl->lock);
@@ -516,8 +522,7 @@  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 
 	mutex_lock(&encl->lock);
 
-	if (!(encl->flags & SGX_ENCL_CREATED) ||
-	    (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD))) {
+	if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
 		ret = -EFAULT;
 		goto out;
 	}
@@ -597,6 +602,9 @@  static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 	void *data;
 	int ret;
 
+	if (!(encl->flags & SGX_ENCL_CREATED))
+		return -EINVAL;
+
 	if (copy_from_user(&addp, arg, sizeof(addp)))
 		return -EFAULT;
 
@@ -685,8 +693,7 @@  static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
 
 	mutex_lock(&encl->lock);
 
-	if (!(encl->flags & SGX_ENCL_CREATED) ||
-	    (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD))) {
+	if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
 		ret = -EFAULT;
 		goto err_out;
 	}
@@ -753,6 +760,9 @@  static long sgx_ioc_enclave_init(struct file *filep, void __user *arg)
 	struct page *initp_page;
 	int ret;
 
+	if (!(encl->flags & SGX_ENCL_CREATED))
+		return -EINVAL;
+
 	if (copy_from_user(&einit, arg, sizeof(einit)))
 		return -EFAULT;