On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote: > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst > index 59472cd6a11d..35f713e3a267 100644 > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > @@ -323,6 +323,7 @@ Code Seq# Include File Comments > <mailto:tlewis@xxxxxxxxxxxxxx> > 0xA3 90-9F linux/dtlk.h > 0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem > +0xA4 00-1F uapi/asm/sgx.h Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist) Maybe add <mailto:linux-sgx@xxxxxxxxxxxxxxx> ? > 0xAA 00-3F linux/uapi/linux/userfaultfd.h > 0xAB 00-1F linux/nbd.h > 0xAC 00-1F linux/raw.h ... > +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) > +{ > + unsigned long encl_size = secs->size + PAGE_SIZE; Wait, you just copied @secs from user memory in sgx_ioc_enclave_create() and now use ->size unverified? You're kidding, right? > + struct sgx_epc_page *secs_epc; > + unsigned long ssaframesize; > + struct sgx_pageinfo pginfo; > + struct sgx_secinfo secinfo; > + struct file *backing; > + long ret; > + > + if (atomic_read(&encl->flags) & SGX_ENCL_CREATED) > + return -EINVAL; > + > + ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm); So this is using more un-validated user input to do further calculations. What can possibly go wrong? I sure hope *I* am wrong and am missing something here. If not, please, for the next version, audit all your user input and validate it before using it. Srsly. > + if (sgx_validate_secs(secs, ssaframesize)) { > + pr_debug("invalid SECS\n"); > + return -EINVAL; > + } > + > + backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5), > + VM_NORESERVE); > + if (IS_ERR(backing)) > + return PTR_ERR(backing); > + > + encl->backing = backing; > + > + secs_epc = __sgx_alloc_epc_page(); > + if (IS_ERR(secs_epc)) { > + ret = PTR_ERR(secs_epc); > + goto err_out_backing; > + } > + > + encl->secs.epc_page = secs_epc; > + > + pginfo.addr = 0; > + pginfo.contents = (unsigned long)secs; > + pginfo.metadata = (unsigned long)&secinfo; > + pginfo.secs = 0; > + memset(&secinfo, 0, sizeof(secinfo)); > + > + ret = __ecreate((void *)&pginfo, sgx_get_epc_addr(secs_epc)); > + if (ret) { > + pr_debug("ECREATE returned %ld\n", ret); > + goto err_out; > + } > + > + if (secs->attributes & SGX_ATTR_DEBUG) > + atomic_or(SGX_ENCL_DEBUG, &encl->flags); > + > + encl->secs.encl = encl; > + encl->secs_attributes = secs->attributes; > + encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK; > + 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 setting and checking enclave creation without having to take > + * encl->lock. > + */ > + atomic_or(SGX_ENCL_CREATED, &encl->flags); > + > + return 0; > + > +err_out: > + sgx_free_epc_page(encl->secs.epc_page); > + encl->secs.epc_page = NULL; > + > +err_out_backing: > + fput(encl->backing); > + encl->backing = NULL; > + > + return ret; > +} > + > +/** > + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE > + * @filep: open file to /dev/sgx That's @encl: enclave pointer or so. > + * @arg: userspace pointer to a struct sgx_enclave_create instance > + * > + * Allocate kernel data structures for a new enclave and execute ECREATE after > + * verifying the correctness of the provided SECS. > + * > + * Note, enforcement of restricted and disallowed attributes is deferred until > + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is > + * checked by sgx_ioc_enclave_create(). Well, I don't see that checking. Where is it? > + * > + * Return: > + * 0 on success, > + * -errno otherwise > + */ > +static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg) > +{ > + struct sgx_enclave_create ecreate; > + struct page *secs_page; > + struct sgx_secs *secs; > + int ret; > + > + if (copy_from_user(&ecreate, arg, sizeof(ecreate))) > + return -EFAULT; > + > + secs_page = alloc_page(GFP_KERNEL); > + if (!secs_page) > + return -ENOMEM; > + > + secs = kmap(secs_page); > + if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) { > + ret = -EFAULT; > + goto out; > + } > + > + ret = sgx_encl_create(encl, secs); > + > +out: > + kunmap(secs_page); > + __free_page(secs_page); > + return ret; > +} -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
