lkml.org 
[lkml]   [2020]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v38 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES
Date
From
On Thu, 17 Sep 2020 11:02:06 -0500, Jarkko Sakkinen  
<jarkko.sakkinen@linux.intel.com> 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@linux.intel.com> 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.

\
 
 \ /
  Last update: 2020-09-17 20:36    [W:0.323 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site