Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org, linux-sgx@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Jethro Beekman <jethro@fortanix.com>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Chunyang Hui <sanqian.hcy@antfin.com>,
	Jordan Hand <jorhand@linux.microsoft.com>,
	Nathaniel McCallum <npmccallum@redhat.com>,
	Seth Moore <sethmo@google.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Suresh Siddha <suresh.b.siddha@intel.com>,
	akpm@linux-foundation.org, andriy.shevchenko@linux.intel.com,
	asapek@google.com, cedric.xing@intel.com,
	chenalexchen@google.com, conradparker@google.com,
	cyhanish@google.com, dave.hansen@intel.com,
	haitao.huang@intel.com, josh@joshtriplett.org,
	kai.huang@intel.com, kai.svahn@intel.com, kmoy@google.com,
	ludloff@google.com, luto@kernel.org, nhorman@redhat.com,
	puiterwijk@redhat.com, rientjes@google.com, tglx@linutronix.de,
	yaozhangx@google.com
Subject: Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
Date: Sat, 4 Jul 2020 03:13:56 +0300
Message-ID: <20200704001356.GB104749@linux.intel.com> (raw)
In-Reply-To: <20200626153400.GE27151@zn.tnic>

On Fri, Jun 26, 2020 at 05:34:00PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> 
> ...
> 
> This could use some commenting along the lines of:
> 
> "— If the enclave developer 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."
> 
> At least this text from the SDM maps to the 256 bytes below. Otherwise
> it is magic.

Copied with pride:

/*
 * 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_add_page(struct sgx_encl *encl, unsigned long src,
> > +			     unsigned long offset, unsigned long length,
> > +			     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);
> > +	}
> > +
> > +	if (atomic_read(&encl->flags) &
> > +	    (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
> > +		ret = -EFAULT;
> > +		goto err_out_free;
> > +	}
> 
> You can do this first thing when you enter the function so that
> you don't have to allocate needlessly in the error case, when
> SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD is set.

Updated version:

static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
			     unsigned long offset, unsigned long length,
			     struct sgx_secinfo *secinfo, unsigned long flags)
{
	struct sgx_encl_page *encl_page;
	struct sgx_epc_page *epc_page;
	struct sgx_va_page *va_page;
	int ret;

	if (atomic_read(&encl->flags) & SGX_ENCL_INITIALIZED)
		return -EFAULT;

SGX_ENCL_DEAD check is unnecessary altogether as this flag cannot be
possibly be unset inside ioctl. 'sgx_release()' will set it which is
the release callback for the enclave file.

'sgx_ioctl()' also unnecessarily has this check I just noticed (and
removed).

> "uninitialized"?
> 
> Where is the test for SGX_ENCL_INITIALIZED and erroring out otherwise?
> 
> I.e., what happens if you add pages to an initialized enclave?

Because of historical reasons it is in sgx_encl_add_page(). Then we
allowed ioctl's operate on enclave concurrently. Today we enforce
sequential operation on a single enclave with SGX_ENCL_IOCTL flag
because that is the only sane way to use the construction operations.

Therefore the check can be moved to sgx_ioc_encl_add_pages() if you
request so but first I have one remark to discuss.

I noticed that sometimes wrong state flags turn into -EINVAL and
sometimes into -EFAULT (like in the previous case). I'd suggest
that when the ioctl is blocked based encl->flags and only on that,
the ioctl would return -ENOIOCTLCMD in both cases, i.e. this
command is not available.

That would give much better aids for debugging user space code.

> 
> > + * measurement with the contents of the page. The address range of pages must
> > + * be contiguous.
> 
> Must? Who is enforcing this? I'm trying to find where...

Unfortunately I cannot recall what I meant when I wrote that. I removed
that sentence. I'm not sure what I meant exactly when I used 'contiguous'
here.

> > 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:
> > + *   0 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_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;
> 
> How many pages are allowed? Unlimited? I'm hoping some limits are
> checked somewhere...

SGX_IOC_ENCLAVE_CREATE defines the address range, and thus sets the
limit on how many pages in total can be added to the enclave.

sgx_encl_size_max_64 contains the maximum size for the address range
and is initialized as follows:

cpuid_count(SGX_CPUID, 0, &eax, &ebx, &ecx, &edx);
sgx_encl_size_max_64 = 1ULL << ((edx >> 8) & 0xFF);

[derived from sgx_drv_init()]

> > +
> > +	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 (signal_pending(current)) {
> > +			ret = -EINTR;
> > +			break;
> > +		}
> > +
> > +		if (need_resched())
> > +			cond_resched();
> > +
> > +		ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
> > +					addp.length - c, &secinfo, addp.flags);
> > +		if (ret)
> > +			break;
> > +	}
> > +
> > +	addp.count = c;

If you referred with your previous question, how to limit the number of
pages that this ioctl can process in one run, it is already supported
in the API with 'addp.count'.

It'd be possible to add this if required:

addp.length = min(addp.length, SGX_ENCLAVE_IOC_ADD_PAGES_MAX_LENGTH));

/Jarkko

  reply index

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-17 22:08 [PATCH v33 00/21] Intel SGX foundations Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 01/21] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-06-22 17:37   ` Borislav Petkov
2020-06-25  1:25     ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 02/21] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen
2020-06-24 13:04   ` Borislav Petkov
2020-06-24 14:34     ` Sean Christopherson
2020-06-25  1:28       ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 03/21] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-06-25  8:59   ` Borislav Petkov
2020-06-25 15:34     ` Sean Christopherson
2020-06-25 16:49       ` Borislav Petkov
2020-06-25 20:52     ` Jarkko Sakkinen
2020-06-25 21:11       ` Borislav Petkov
2020-06-26 13:34         ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 04/21] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 05/21] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 06/21] x86/cpu/intel: Detect SGX support Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 07/21] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 08/21] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen
2020-06-25 10:14   ` Borislav Petkov
2020-06-25 20:11     ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 09/21] x86/sgx: Add __sgx_alloc_epc_page() and sgx_free_epc_page() Jarkko Sakkinen
2020-06-25 17:06   ` Borislav Petkov
2020-06-25 20:55     ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 10/21] mm: Introduce vm_ops->may_mprotect() Jarkko Sakkinen
2020-06-25 17:14   ` Borislav Petkov
2020-06-25 17:30     ` Matthew Wilcox
2020-06-25 18:06       ` Sean Christopherson
2020-06-25 22:40         ` Jarkko Sakkinen
2020-06-25 22:26     ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 11/21] x86/sgx: Linux Enclave Driver Jarkko Sakkinen
2020-06-25 17:23   ` Borislav Petkov
2020-06-25 18:34     ` Sean Christopherson
2020-06-25 18:45       ` Borislav Petkov
2020-06-26 14:19       ` Jarkko Sakkinen
2020-06-25 20:21     ` Jarkko Sakkinen
2020-06-25 20:25       ` Borislav Petkov
2020-06-26 13:40         ` Jarkko Sakkinen
2020-06-25 18:53   ` Borislav Petkov
2020-06-26 14:17     ` Jarkko Sakkinen
2020-06-26  9:14   ` Borislav Petkov
2020-06-26 14:16     ` Sean Christopherson
2020-06-26 14:20       ` Borislav Petkov
2020-07-03 23:04         ` Jarkko Sakkinen
2020-07-03  3:09     ` Jarkko Sakkinen
2020-06-26 15:34   ` Borislav Petkov
2020-07-04  0:13     ` Jarkko Sakkinen [this message]
2020-06-27 17:43   ` Borislav Petkov
2020-06-29 15:27     ` Sean Christopherson
2020-06-29 15:37       ` Borislav Petkov
2020-07-04  1:43       ` Jarkko Sakkinen
2020-07-07  1:38         ` Sean Christopherson
2020-07-07  3:29           ` Jarkko Sakkinen
2020-07-04  1:42     ` Jarkko Sakkinen
2020-07-02  3:59   ` Sean Christopherson
2020-07-04  3:31     ` Jarkko Sakkinen
2020-09-02  3:06       ` Haitao Huang
2020-09-02 16:10         ` Sean Christopherson
2020-09-02 18:40           ` Haitao Huang
2020-09-04 12:01         ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 12/21] x86/sgx: Allow a limited use of ATTRIBUTE.PROVISIONKEY for attestation Jarkko Sakkinen
2020-06-29 16:02   ` Borislav Petkov
2020-06-29 22:04     ` Sean Christopherson
2020-06-30  8:49       ` Borislav Petkov
2020-06-30 14:20         ` Sean Christopherson
2020-06-30 17:13           ` Andy Lutomirski
2020-07-02 20:47         ` Dr. Greg
2020-07-03  2:43         ` Jarkko Sakkinen
2020-07-03  2:38       ` Jarkko Sakkinen
2020-07-03  2:32     ` Jarkko Sakkinen
2020-07-03  2:55       ` Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 13/21] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 14/21] x86/sgx: ptrace() support for the SGX driver Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 15/21] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-06-29 17:10   ` Borislav Petkov
2020-06-30  6:00     ` Sean Christopherson
2020-06-30  8:41       ` Borislav Petkov
2020-06-30 14:55         ` Sean Christopherson
2020-06-30 16:48         ` Andy Lutomirski
2020-06-30 17:23           ` Sean Christopherson
2020-07-02 12:52           ` Thomas Gleixner
2020-06-17 22:08 ` [PATCH v33 16/21] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 17/21] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 18/21] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 19/21] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 20/21] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen
2020-06-17 22:08 ` [PATCH v33 21/21] x86/sgx: Update MAINTAINERS Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200704001356.GB104749@linux.intel.com \
    --to=jarkko.sakkinen@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=asapek@google.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=chenalexchen@google.com \
    --cc=conradparker@google.com \
    --cc=cyhanish@google.com \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=jorhand@linux.microsoft.com \
    --cc=josh@joshtriplett.org \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=kmoy@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=ludloff@google.com \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sanqian.hcy@antfin.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=sethmo@google.com \
    --cc=suresh.b.siddha@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --cc=yaozhangx@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org
	public-inbox-index linux-sgx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git