| Message ID | 20190808001254.11926-8-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Bug fixes for v22 | expand |
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
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
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.
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.
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;
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(-)