Re: [PATCH v37 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen]

On Tue, Sep 15, 2020 at 12:54:50PM +0300, Jarkko Sakkinen wrote:
> On Sun, Sep 13, 2020 at 09:56:03PM -0500, Haitao Huang wrote:
> > 
> > On Fri, 11 Sep 2020 07:40:08 -0500, Jarkko Sakkinen
> > <jarkko.sakkinen@xxxxxxxxxxxxxxx> wrote:
> > ...
> > 
> > > +/**
> > > + * sgx_ioc_enclave_add_pages() - The handler for
> > > %SGX_IOC_ENCLAVE_ADD_PAGES
> > > + * @encl:       an enclave pointer
> > > + * @arg:	a user pointer to a struct sgx_enclave_add_pages instance
> > > + *
> > > + * Add one or more pages to an uninitialized enclave, and optionally
> > > extend the
> > > + * measurement with the contents of the page. The SECINFO and
> > > measurement mask
> > > + * are applied to all pages.
> > > + *
> > > + * A SECINFO for a TCS is required to always contain zero permissions
> > > because
> > > + * CPU silently zeros them. Allowing anything else would cause a
> > > mismatch in
> > > + * the measurement.
> > > + *
> > > + * mmap()'s protection bits are capped by the page permissions. For
> > > each page
> > > + * address, the maximum protection bits are computed with the following
> > > + * heuristics:
> > > + *
> > > + * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO
> > > permissions.
> > > + * 2. A TCS page: PROT_R | PROT_W.
> > > + *
> > > + * mmap() is not allowed to surpass the minimum of the maximum
> > > protection bits
> > > + * within the given address range.
> > > + *
> > > + * If ENCLS opcode fails, that effectively means that EPC has been
> > > invalidated.
> > > + * When this happens the enclave is destroyed and -EIO is returned to
> > > the
> > > + * caller.
> > > + *
> > > + * Return:
> > > + *   length of the data processed on success,
> > > + *   -EACCES if an executable source page is located in a noexec
> > > partition,
> > > + *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
> > > + *   -errno otherwise
> > > + */
> > > +static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void
> > > __user *arg)
> > > +{
> > > +	struct sgx_enclave_add_pages addp;
> > > +	struct sgx_secinfo secinfo;
> > > +	unsigned long c;
> > > +	int ret;
> > > +
> > > +	if ((atomic_read(&encl->flags) & SGX_ENCL_INITIALIZED) ||
> > > +	    !(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
> > > +		return -EINVAL;
> > > +
> > > +	if (copy_from_user(&addp, arg, sizeof(addp)))
> > > +		return -EFAULT;
> > > +
> > > +	if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
> > > +	    !IS_ALIGNED(addp.src, PAGE_SIZE))
> > > +		return -EINVAL;
> > > +
> > > +	if (!(access_ok(addp.src, PAGE_SIZE)))
> > > +		return -EFAULT;
> > > +
> > > +	if (addp.length & (PAGE_SIZE - 1))
> > > +		return -EINVAL;
> > > +
> > > +	if (addp.offset + addp.length - PAGE_SIZE >= encl->size)
> > > +		return -EINVAL;
> > > +
> > > +	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
> > > +			   sizeof(secinfo)))
> > > +		return -EFAULT;
> > > +
> > > +	if (sgx_validate_secinfo(&secinfo))
> > > +		return -EINVAL;
> > > +
> > > +	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> > > +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
> > > +			ret = c;
> > > +			break;
> > > +		}
> > > +
> > > +		if (need_resched())
> > > +			cond_resched();
> > > +
> > > +		ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
> > > +					addp.length - c, &secinfo, addp.flags);
> > 
> > no need passing addp.length - c?
> 
> True, it is cruft from the past.
> 
> I'll remove.
> 
> > 
> > > +		if (ret)
> > > +			break;
> > 
> > Some error cases here are fatal and should be passed back to user space so
> > that it would not retry.
> 
> I don't comprehend this. 'ret' is passed to the user space.

OK, spotted the regression, sorry about this. I'll fix it for v38, which
I'm sending soon given the email server issues with v37.

/Jarkko