Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Hansen <dave.hansen@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	x86@kernel.org, linux-sgx@vger.kernel.org
Cc: linux-kernel@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>,
	Darren Kenny <darren.kenny@oracle.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, bp@alien8.de, cedric.xing@intel.com,
	chenalexchen@google.com, conradparker@google.com,
	cyhanish@google.com, haitao.huang@intel.com, 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,
	mikko.ylinen@intel.com
Subject: Re: [PATCH v39 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE
Date: Fri, 16 Oct 2020 10:07:47 -0700
Message-ID: <5f194bf0-ced1-66e1-b6a2-503741a3e7f1@intel.com> (raw)
In-Reply-To: <20201003045059.665934-13-jarkko.sakkinen@linux.intel.com>

> +static u32 sgx_calc_ssa_frame_size(u32 miscselect, u64 xfrm)
> +{
> +	u32 size_max = PAGE_SIZE;
> +	u32 size;
> +	int i;
> +
> +	for (i = 2; i < 64; i++) {

Should this be:

	for (i = XFEATURE_YMM; i < XFEATURE_MAX; i++) {

Basically, does this need to be 64, or should it be limited to the
kernel-known XFEATURES?  Or, should this be looping through all the bits
set in xfeatures_mask_user().

> +		if (!((1 << i) & xfrm))
> +			continue;
> +
> +		size = SGX_SSA_GPRS_SIZE + sgx_xsave_size_tbl[i];
> +
> +		if (miscselect & SGX_MISC_EXINFO)
> +			size += SGX_SSA_MISC_EXINFO_SIZE;
> +
> +		if (size > size_max)
> +			size_max = size;
> +	}
> +
> +	return PFN_UP(size_max);
> +}
> +
> +static int sgx_validate_secs(const struct sgx_secs *secs)
> +{

What's the overall point of this function?  Does it avoid a #GP from an
instruction later?

Does all of the 'secs' content come from userspace?

> +	u64 max_size = (secs->attributes & SGX_ATTR_MODE64BIT) ?
> +		       sgx_encl_size_max_64 : sgx_encl_size_max_32;
> +
> +	if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
> +		return -EINVAL;
> +
> +	if (secs->base & (secs->size - 1))
> +		return -EINVAL;
> +
> +	if (secs->miscselect & sgx_misc_reserved_mask ||
> +	    secs->attributes & sgx_attributes_reserved_mask ||
> +	    secs->xfrm & sgx_xfrm_reserved_mask)
> +		return -EINVAL;
> +
> +	if (secs->size > max_size)
> +		return -EINVAL;
> +
> +	if (!(secs->xfrm & XFEATURE_MASK_FP) ||
> +	    !(secs->xfrm & XFEATURE_MASK_SSE) ||
> +	    (((secs->xfrm >> XFEATURE_BNDREGS) & 1) != ((secs->xfrm >> XFEATURE_BNDCSR) & 1)))
> +		return -EINVAL;
> +
> +	if (!secs->ssa_frame_size)
> +		return -EINVAL;
> +
> +	if (sgx_calc_ssa_frame_size(secs->miscselect, secs->xfrm) > secs->ssa_frame_size)
> +		return -EINVAL;
> +
> +	if (memchr_inv(secs->reserved1, 0, sizeof(secs->reserved1)) ||
> +	    memchr_inv(secs->reserved2, 0, sizeof(secs->reserved2)) ||
> +	    memchr_inv(secs->reserved3, 0, sizeof(secs->reserved3)) ||
> +	    memchr_inv(secs->reserved4, 0, sizeof(secs->reserved4)))
> +		return -EINVAL;
> +
> +	return 0;
> +}

I think it would be nice to at least have one comment per condition to
explain what's going on there.

> +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> +{
> +	struct sgx_epc_page *secs_epc;
> +	struct sgx_pageinfo pginfo;
> +	struct sgx_secinfo secinfo;
> +	unsigned long encl_size;
> +	struct file *backing;
> +	long ret;
> +
> +	if (sgx_validate_secs(secs)) {
> +		pr_debug("invalid SECS\n");
> +		return -EINVAL;
> +	}
> +
> +	/* The extra page goes to SECS. */
> +	encl_size = secs->size + PAGE_SIZE;
> +
> +	backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
> +				   VM_NORESERVE);

What's the >>5 adjustment for?

> +	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->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);

I'm wondering what the impact of setting this flag is.  That's hard to
figure out because the flag isn't documented.

It's also unusual to have atomic_or() used like this.  The normal
set_bit()/clear_bit() that you can use on an unsigned long are actually
implemented as atomics.

I'm curious both why this needs to be atomics, *and* why the atomic_or()
interface is being used.

> +	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
> + * @encl:	an enclave pointer
> + * @arg:	userspace pointer to a struct sgx_enclave_create instance
> + *
> + * Allocate kernel data structures for a new enclave and execute ECREATE after
> + * checking that the provided data for SECS meets the expectations of ECREATE
> + * for an uninitialized enclave and size of the address space does not surpass the
> + * platform expectations. This validation is done by sgx_validate_secs().
> + *
> + * 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 (atomic_read(&encl->flags) & SGX_ENCL_CREATED)
> +		return -EINVAL;
> +
> +	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);

GFP_KERNEL pages are in low memory and don't need to be kmap()'d.

This can just be:

	secs = __get_free_page(GFP_KERNEL);
	if (copy_from_user(secs, (void __user *)ecreate.src,...

and forget about the kmapping.  You also need to change __free_pages()
to free_pages().

The other alternative would be to just kmalloc() it.  kmalloc()
guarantees alignment in a stronger way than it used to.

> +	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;
> +}
> +
> +long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> +{
> +	struct sgx_encl *encl = filep->private_data;
> +	int ret, encl_flags;
> +
> +	encl_flags = atomic_fetch_or(SGX_ENCL_IOCTL, &encl->flags);
> +	if (encl_flags & SGX_ENCL_IOCTL)
> +		return -EBUSY;

Is the SGX_ENCL_IOCTL bit essentially just a lock to single-thread
ioctl()s?  Should we name it as such?

> +	if (encl_flags & SGX_ENCL_DEAD) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	switch (cmd) {
> +	case SGX_IOC_ENCLAVE_CREATE:
> +		ret = sgx_ioc_enclave_create(encl, (void __user *)arg);
> +		break;
> +	default:
> +		ret = -ENOIOCTLCMD;
> +		break;
> +	}
> +
> +out:
> +	atomic_andnot(SGX_ENCL_IOCTL, &encl->flags);
> +	return ret;
> +}
> 


  reply index

Thread overview: 117+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-03  4:50 [PATCH v39 00/24] Intel SGX foundations Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 01/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-10-19 14:10   ` Dave Hansen
2020-10-19 17:49     ` Sean Christopherson
2020-10-03  4:50 ` [PATCH v39 02/24] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 03/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 04/24] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 05/24] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2020-10-19 14:30   ` Dave Hansen
2020-10-19 17:38     ` Sean Christopherson
2020-10-19 17:48       ` Dave Hansen
2020-10-19 17:53         ` Sean Christopherson
2020-10-19 17:58           ` Dave Hansen
2020-10-03  4:50 ` [PATCH v39 06/24] x86/cpu/intel: Detect SGX support Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 07/24] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 08/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen
2020-10-19  8:45   ` Jarkko Sakkinen
2020-10-19 12:39     ` Borislav Petkov
2020-10-23  9:01       ` Jarkko Sakkinen
2020-10-19 13:40     ` Dave Hansen
2020-10-23  9:03       ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 09/24] x86/sgx: Add __sgx_alloc_epc_page() and sgx_free_epc_page() Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 11/24] x86/sgx: Add SGX enclave driver Jarkko Sakkinen
2020-10-03 14:39   ` Greg KH
2020-10-04 14:32     ` Jarkko Sakkinen
2020-10-04 15:01       ` Jarkko Sakkinen
2020-10-05  9:42       ` Greg KH
2020-10-05 12:42         ` Jarkko Sakkinen
2020-10-07 18:09           ` Haitao Huang
2020-10-07 19:26             ` Greg KH
2020-10-09  6:44               ` Jarkko Sakkinen
2020-10-14 20:16                 ` Dave Hansen
2020-10-05  8:45     ` Christoph Hellwig
2020-10-05 11:42       ` Jarkko Sakkinen
2020-10-05 11:50         ` Greg KH
2020-10-05 14:23           ` Jarkko Sakkinen
2020-10-05 15:02             ` Greg KH
2020-10-05 16:40               ` Dave Hansen
2020-10-05 20:02                 ` Jarkko Sakkinen
2020-10-09  7:10     ` Pavel Machek
2020-10-09  7:21       ` Greg KH
2020-10-09  8:21         ` Pavel Machek
2020-10-03 19:54   ` Matthew Wilcox
2020-10-04 21:50     ` Jarkko Sakkinen
2020-10-04 22:02       ` Jarkko Sakkinen
2020-10-04 22:27       ` Matthew Wilcox
2020-10-04 23:41         ` Jarkko Sakkinen
2020-10-05  1:30           ` Matthew Wilcox
2020-10-05  3:06             ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Jarkko Sakkinen
2020-10-16 17:07   ` Dave Hansen [this message]
2020-10-18  4:26     ` Jarkko Sakkinen
2020-10-19 20:21       ` Dave Hansen
2020-10-19 20:48         ` Sean Christopherson
2020-10-03  4:50 ` [PATCH v39 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-10-16 21:25   ` Dave Hansen
2020-10-18  5:03     ` Jarkko Sakkinen
2020-10-19  7:03       ` Jarkko Sakkinen
2020-10-19 20:48       ` Dave Hansen
2020-10-19 21:15         ` Sean Christopherson
2020-10-19 21:44           ` Dave Hansen
2020-10-23 10:11             ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION Jarkko Sakkinen
2020-10-20 15:48   ` Dave Hansen
2020-10-23 10:14     ` Jarkko Sakkinen
2020-10-20 21:19   ` Dave Hansen
2020-10-23 10:17     ` Jarkko Sakkinen
2020-10-23 14:19       ` Dave Hansen
2020-10-24 11:34         ` Jarkko Sakkinen
2020-10-24 15:47           ` Andy Lutomirski
2020-10-24 20:23             ` Jarkko Sakkinen
2020-10-27 10:38               ` Dr. Greg
2020-10-23 14:23       ` Jethro Beekman
2020-10-24 11:40         ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 16/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-10-03  5:22   ` Haitao Huang
2020-10-03 13:32     ` Jarkko Sakkinen
2020-10-03 18:23       ` Haitao Huang
2020-10-04 22:39         ` Jarkko Sakkinen
2020-10-07 17:25           ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 17/24] x86/sgx: Add ptrace() support for the SGX driver Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 18/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 19/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 20/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-10-06  2:57   ` Sean Christopherson
2020-10-06  8:30     ` Jethro Beekman
2020-10-06 15:15       ` Sean Christopherson
2020-10-06 17:28         ` Jarkko Sakkinen
2020-10-06 23:21           ` Sean Christopherson
2020-10-07  0:22             ` Jarkko Sakkinen
2020-10-07  1:17               ` Sean Christopherson
2020-10-07  3:14                 ` Jarkko Sakkinen
2020-10-07  4:34                   ` Sean Christopherson
2020-10-07  7:39                     ` Jarkko Sakkinen
2020-10-07  8:04                       ` Jarkko Sakkinen
2020-10-07 15:25                       ` Sean Christopherson
2020-10-07 17:08                         ` Jarkko Sakkinen
2020-10-07 17:13                           ` Jarkko Sakkinen
2020-10-06 15:49       ` Jarkko Sakkinen
2020-10-06 15:36     ` Jarkko Sakkinen
2020-10-06 21:39     ` Jarkko Sakkinen
2020-10-07  0:23       ` Jarkko Sakkinen
2020-10-17  1:48   ` Andy Lutomirski
2020-10-17 21:02     ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 22/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-10-12 16:50   ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 23/24] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 24/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-10-16 21:04   ` Dave Hansen
2020-10-18  4:27     ` Jarkko Sakkinen
2020-10-03 14:32 ` [PATCH v39 00/24] Intel SGX foundations Greg KH
2020-10-03 14:53   ` Jarkko Sakkinen
2020-10-15 19:06 ` Dave Hansen
2020-10-17 20:43   ` 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=5f194bf0-ced1-66e1-b6a2-503741a3e7f1@intel.com \
    --to=dave.hansen@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=darren.kenny@oracle.com \
    --cc=haitao.huang@intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=jorhand@linux.microsoft.com \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=kmoy@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=ludloff@google.com \
    --cc=luto@kernel.org \
    --cc=mikko.ylinen@intel.com \
    --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