Message-ID: <20200626141627.GA6583@linux.intel.com>
Date:   Fri, 26 Jun 2020 07:16:27 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>, x86@...nel.org,
        linux-sgx@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Jethro Beekman <jethro@...tanix.com>,
        Haitao Huang <haitao.huang@...ux.intel.com>,
        Chunyang Hui <sanqian.hcy@...fin.com>,
        Jordan Hand <jorhand@...ux.microsoft.com>,
        Nathaniel McCallum <npmccallum@...hat.com>,
        Seth Moore <sethmo@...gle.com>,
        Suresh Siddha <suresh.b.siddha@...el.com>,
        akpm@...ux-foundation.org, andriy.shevchenko@...ux.intel.com,
        asapek@...gle.com, cedric.xing@...el.com, chenalexchen@...gle.com,
        conradparker@...gle.com, cyhanish@...gle.com,
        dave.hansen@...el.com, haitao.huang@...el.com,
        josh@...htriplett.org, kai.huang@...el.com, kai.svahn@...el.com,
        kmoy@...gle.com, ludloff@...gle.com, luto@...nel.org,
        nhorman@...hat.com, puiterwijk@...hat.com, rientjes@...gle.com,
        tglx@...utronix.de, yaozhangx@...gle.com
Subject: Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver

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

Powered by blists - more mailing lists