On Fri, Jun 26, 2020 at 11:14:19AM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > +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?
The size of the enclave is checked in sgx_validate_secs() before it is used
to configure the shmem backing.
> > + 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?
ssaframesize is also validated below, and the computations on miscselect and
xfm in sgx_calc_ssaframesize() are bounded such that bad input won't send
the kernel into the weeds.
That being said, I agree that it would be safer to move sgx_calc_ssaframesize()
inside sgx_validate_secs() and only compute encl_size after the secs is
validated.
> 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
