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

[Haitao Huang]

On Thu, 17 Sep 2020 11:02:06 -0500, Jarkko Sakkinen  
<jarkko.sakkinen@xxxxxxxxxxxxxxx> wrote:

> On Thu, Sep 17, 2020 at 12:34:18AM -0500, Haitao Huang wrote:
> > On Tue, 15 Sep 2020 06:05:11 -0500, Jarkko Sakkinen
> > <jarkko.sakkinen@xxxxxxxxxxxxxxx> wrote:
> > ...
> >
> > > +static int __sgx_encl_add_page(struct sgx_encl *encl,
> > > +			       struct sgx_encl_page *encl_page,
> > > +			       struct sgx_epc_page *epc_page,
> > > +			       struct sgx_secinfo *secinfo, unsigned long src)
> > > +{
> > > +	struct sgx_pageinfo pginfo;
> > > +	struct vm_area_struct *vma;
> > > +	struct page *src_page;
> > > +	int ret;
> > > +
> > > +	/* Query vma's VM_MAYEXEC as an indirect path_noexec() check. */
> > > +	if (encl_page->vm_max_prot_bits & VM_EXEC) {
> > > +		vma = find_vma(current->mm, src);
> > > +		if (!vma)
> > > +			return -EFAULT;
> > > +
> > > +		if (!(vma->vm_flags & VM_MAYEXEC))
> > > +			return -EACCES;
> > > +	}
> > > +
> > > +	ret = get_user_pages(src, 1, 0, &src_page, NULL);
> > > +	if (ret < 1)
> > > +		return ret;
> > > +
> > > +	pginfo.secs = (unsigned long)sgx_get_epc_addr(encl->secs.epc_page);
> > > +	pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
> > > +	pginfo.metadata = (unsigned long)secinfo;
> > > +	pginfo.contents = (unsigned long)kmap_atomic(src_page);
> > > +
> > > +	ret = __eadd(&pginfo, sgx_get_epc_addr(epc_page));
> > > +
> > > +	kunmap_atomic((void *)pginfo.contents);
> > > +	put_page(src_page);
> > > +
> > > +	return ret ? -EIO : 0;
> > > +}
> > > +
> > > +/*
> > > + * If the caller requires measurement of the page as a proof for the
> > > content,
> > > + * use EEXTEND to add a measurement for 256 bytes of the page. Repeat
> > > this
> > > + * operation until the entire page is measured."
> > > + */
> > > +static int __sgx_encl_extend(struct sgx_encl *encl,
> > > +			     struct sgx_epc_page *epc_page)
> > > +{
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < 16; i++) {
> > > +		ret = __eextend(sgx_get_epc_addr(encl->secs.epc_page),
> > > +				sgx_get_epc_addr(epc_page) + (i * 0x100));
> > > +		if (ret) {
> > > +			if (encls_failed(ret))
> > > +				ENCLS_WARN(ret, "EEXTEND");
> > > +			return -EIO;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long  
> > src,
> > > +			     unsigned long offset, struct sgx_secinfo *secinfo,
> > > +			     unsigned long flags)
> > > +{
> > > +	struct sgx_encl_page *encl_page;
> > > +	struct sgx_epc_page *epc_page;
> > > +	int ret;
> > > +
> > > +	encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
> > > +	if (IS_ERR(encl_page))
> > > +		return PTR_ERR(encl_page);
> > > +
> > > +	epc_page = __sgx_alloc_epc_page();
> > > +	if (IS_ERR(epc_page)) {
> > > +		kfree(encl_page);
> > > +		return PTR_ERR(epc_page);
> > > +	}
> > > +
> > > +	mmap_read_lock(current->mm);
> > > +	mutex_lock(&encl->lock);
> > > +
> > > +	/*
> > > +	 * Insert prior to EADD in case of OOM.  EADD modifies MRENCLAVE,  
> > i.e.
> > > +	 * can't be gracefully unwound, while failure on EADD/EXTEND is  
> > limited
> > > +	 * to userspace errors (or kernel/hardware bugs).
> > > +	 */
> > > +	ret = xa_insert(&encl->page_array, PFN_DOWN(encl_page->desc),
> > > +			encl_page, GFP_KERNEL);
> > > +	if (ret)
> > > +		goto err_out_unlock;
> > > +
> > > +	ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
> > > +				  src);
> > > +	if (ret)
> > > +		goto err_out;
> > > +
> > > +	/*
> > > +	 * Complete the "add" before doing the "extend" so that the "add"
> > > +	 * isn't in a half-baked state in the extremely unlikely scenario
> > > +	 * the enclave will be destroyed in response to EEXTEND failure.
> > > +	 */
> > > +	encl_page->encl = encl;
> > > +	encl_page->epc_page = epc_page;
> > > +	encl->secs_child_cnt++;
> > > +
> > > +	if (flags & SGX_PAGE_MEASURE) {
> > > +		ret = __sgx_encl_extend(encl, epc_page);
> > > +		if (ret)
> > > +			goto err_out;
> > > +	}
> > > +
> > > +	mutex_unlock(&encl->lock);
> > > +	mmap_read_unlock(current->mm);
> > > +	return ret;
> > > +
> > > +err_out:
> > > +	xa_erase(&encl->page_array, PFN_DOWN(encl_page->desc));
> > > +
> > > +err_out_unlock:
> > > +	mutex_unlock(&encl->lock);
> > > +	mmap_read_unlock(current->mm);
> > > +
> > > +	sgx_free_epc_page(epc_page);
> > > +	kfree(encl_page);
> > > +
> > > +	/*
> > > +	 * Destroy enclave on ENCLS failure as this means that EPC has been
> > > +	 * invalidated.
> > > +	 */
> > > +	if (ret == -EIO) {
> > > +		mutex_lock(&encl->lock);
> > > +		sgx_encl_destroy(encl);
> > > +		mutex_unlock(&encl->lock);
> > > +	}
> > > +
> >
> > I think originally we return both count of succeeded EADDs and the  
> > errors.
> > So we only destroy enclaves in cases of fatal ENCLS failures.
> >
> > Now we only return errors in all failures other than interrupted  
> > operations
> > or SGX_MAX_ADD_PAGES_LENGTH is reached.
> >
> > So, for the new design we should destroy enclaves in all cases here, not
> > just for -EIO.
> >
> > On the other hand, I do like the old way returning both the count and  
> > error
> > better. It helps greatly for debugging any issues in enclave image or  
> > user
> > space code, and also keeps flexibility for user space to recover in  
> > certain
> > errors, such as out of EPC.
>
> Right, I do get the OOM case but wouldn't in that case the reasonable
> thing to do destroy the enclave that is not even running? I mean that
> means that we are globally out of EPC.

I would say it could be a policy, but not the only one. If it does not  
make much difference to kernel, IMHO we should  not set it in stone now.   
Debugging is also huge benefit to me.