Re: [PATCH v30 10/20] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen <hidden>
Date: 2020-05-22 19:39:22
Also in:
lkml
On Thu, May 21, 2020 at 12:12:36PM -0700, Sean Christopherson wrote:
quoted hunk ↗ jump to hunk
On Fri, May 15, 2020 at 03:44:00AM +0300, Jarkko Sakkinen wrote:quoted
+long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) +{ + struct sgx_encl *encl = filep->private_data; + int ret, encl_flags; + + encl_flags = atomic_fetch_or(SGX_ENCL_IOCTL, &encl->flags); + if (encl_flags & SGX_ENCL_IOCTL) + return -EBUSY; + + if (encl_flags & SGX_ENCL_DEAD) + return -EFAULT;Returning immediately is wrong as it leaves SGX_ENCL_IOCTL set. This results in the application seeing -EBUSY on future ioctls() instead of -EFAULT. Can be fixed as below. Do you want me to send a formal patch on linux-sgx?diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 77757a74644d..df35a79e915c 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c@@ -751,8 +751,10 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) if (encl_flags & SGX_ENCL_IOCTL) return -EBUSY; - if (encl_flags & SGX_ENCL_DEAD) - return -EFAULT; + if (encl_flags & SGX_ENCL_DEAD) { + ret = -EFAULT; + goto out; + } switch (cmd) { case SGX_IOC_ENCLAVE_CREATE:@@ -772,6 +774,7 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) break; } +out: atomic_andnot(SGX_ENCL_IOCTL, &encl->flags); return ret;quoted
+ + switch (cmd) { + case SGX_IOC_ENCLAVE_CREATE: + ret = sgx_ioc_enclave_create(encl, (void __user *)arg); + break; + case SGX_IOC_ENCLAVE_ADD_PAGES: + ret = sgx_ioc_enclave_add_pages(encl, (void __user *)arg); + break; + case SGX_IOC_ENCLAVE_INIT: + ret = sgx_ioc_enclave_init(encl, (void __user *)arg); + break; + default: + ret = -ENOIOCTLCMD; + break; + } + + atomic_andnot(SGX_ENCL_IOCTL, &encl->flags); + + return ret; +}
Thanks. Fixed in my tree: v31: * Unset SGX_ENCL_IOCTL in the error path of checking encl->flags in order to prevent leaving it set, and thus block any further ioctl calls. * Added missing cleanup_srcu_struct() call to sgx_encl_release(). * Take encl->lock in sgx_encl_add_page() in order to prevent races with the page reclaimer. /Jarkko