[v36,12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

Commit Message

Jarkko Sakkinen July 16, 2020, 1:52 p.m. UTC

Add an ioctl that performs ENCLS[ECREATE], which creates SGX Enclave
Control Structure for the enclave. SECS contains attributes about the
enclave that are used by the hardware and cannot be directly accessed by
software, as SECS resides in the EPC.

One essential field in SECS is a field that stores the SHA256 of the
measured enclave pages. This field, MRENCLAVE, is initialized by the
ECREATE instruction and updated by every EADD and EEXTEND operation.
Finally, EINIT locks down the value.

Acked-by: Jethro Beekman <[e-mail]>
Tested-by: Jethro Beekman <[e-mail]>
Tested-by: Haitao Huang <[e-mail]>
Tested-by: Chunyang Hui <[e-mail]>
Tested-by: Jordan Hand <[e-mail]>
Tested-by: Nathaniel McCallum <[e-mail]>
Tested-by: Seth Moore <[e-mail]>
Co-developed-by: Sean Christopherson <[e-mail]>
Signed-off-by: Sean Christopherson <[e-mail]>
Co-developed-by: Suresh Siddha <[e-mail]>
Signed-off-by: Suresh Siddha <[e-mail]>
Signed-off-by: Jarkko Sakkinen <[e-mail]>
---
 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 arch/x86/include/uapi/asm/sgx.h               |  25 ++
 arch/x86/kernel/cpu/sgx/Makefile              |   1 +
 arch/x86/kernel/cpu/sgx/driver.c              |  12 +
 arch/x86/kernel/cpu/sgx/driver.h              |   1 +
 arch/x86/kernel/cpu/sgx/ioctl.c               | 226 ++++++++++++++++++
 6 files changed, 266 insertions(+)
 create mode 100644 arch/x86/include/uapi/asm/sgx.h
 create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c

Comments

Darren Kenny Aug. 6, 2020, 3:40 p.m. UTC | #1

On Thursday, 2020-07-16 at 16:52:51 +03, Jarkko Sakkinen wrote:
> Add an ioctl that performs ENCLS[ECREATE], which creates SGX Enclave
> Control Structure for the enclave. SECS contains attributes about the
> enclave that are used by the hardware and cannot be directly accessed by
> software, as SECS resides in the EPC.
>
> One essential field in SECS is a field that stores the SHA256 of the
> measured enclave pages. This field, MRENCLAVE, is initialized by the
> ECREATE instruction and updated by every EADD and EEXTEND operation.
> Finally, EINIT locks down the value.
>
> Acked-by: Jethro Beekman <[e-mail]>
> Tested-by: Jethro Beekman <[e-mail]>
> Tested-by: Haitao Huang <[e-mail]>
> Tested-by: Chunyang Hui <[e-mail]>
> Tested-by: Jordan Hand <[e-mail]>
> Tested-by: Nathaniel McCallum <[e-mail]>
> Tested-by: Seth Moore <[e-mail]>

Tested-by: Darren Kenny <[e-mail]>
Reviewed-by: Darren Kenny <[e-mail]>

> Co-developed-by: Sean Christopherson <[e-mail]>
> Signed-off-by: Sean Christopherson <[e-mail]>
> Co-developed-by: Suresh Siddha <[e-mail]>
> Signed-off-by: Suresh Siddha <[e-mail]>
> Signed-off-by: Jarkko Sakkinen <[e-mail]>
> ---
>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>  arch/x86/include/uapi/asm/sgx.h               |  25 ++
>  arch/x86/kernel/cpu/sgx/Makefile              |   1 +
>  arch/x86/kernel/cpu/sgx/driver.c              |  12 +
>  arch/x86/kernel/cpu/sgx/driver.h              |   1 +
>  arch/x86/kernel/cpu/sgx/ioctl.c               | 226 ++++++++++++++++++
>  6 files changed, 266 insertions(+)
>  create mode 100644 arch/x86/include/uapi/asm/sgx.h
>  create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd6a11d..35f713e3a267 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -323,6 +323,7 @@ Code  Seq#    Include File                                           Comments
>                                                                       <mailto:[e-mail]>
>  0xA3  90-9F  linux/dtlk.h
>  0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
> +0xA4  00-1F  uapi/asm/sgx.h                                          Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
>  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
>  0xAB  00-1F  linux/nbd.h
>  0xAC  00-1F  linux/raw.h
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> new file mode 100644
> index 000000000000..3787d278e84b
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
> +/*
> + * Copyright(c) 2016-19 Intel Corporation.
> + */
> +#ifndef _UAPI_ASM_X86_SGX_H
> +#define _UAPI_ASM_X86_SGX_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define SGX_MAGIC 0xA4
> +
> +#define SGX_IOC_ENCLAVE_CREATE \
> +	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
> +
> +/**
> + * struct sgx_enclave_create - parameter structure for the
> + *                             %SGX_IOC_ENCLAVE_CREATE ioctl
> + * @src:	address for the SECS page data
> + */
> +struct sgx_enclave_create  {
> +	__u64	src;
> +};
> +
> +#endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/Makefile b/arch/x86/kernel/cpu/sgx/Makefile
> index 3fc451120735..91d3dc784a29 100644
> --- a/arch/x86/kernel/cpu/sgx/Makefile
> +++ b/arch/x86/kernel/cpu/sgx/Makefile
> @@ -1,4 +1,5 @@
>  obj-y += \
>  	driver.o \
>  	encl.o \
> +	ioctl.o \
>  	main.o
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index b52520407f5b..5559bc18de41 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -118,10 +118,22 @@ static unsigned long sgx_get_unmapped_area(struct file *file,
>  	return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
>  }
>  
> +#ifdef CONFIG_COMPAT
> +static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
> +			      unsigned long arg)
> +{
> +	return sgx_ioctl(filep, cmd, arg);
> +}
> +#endif
> +
>  static const struct file_operations sgx_encl_fops = {
>  	.owner			= THIS_MODULE,
>  	.open			= sgx_open,
>  	.release		= sgx_release,
> +	.unlocked_ioctl		= sgx_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl		= sgx_compat_ioctl,
> +#endif
>  	.mmap			= sgx_mmap,
>  	.get_unmapped_area	= sgx_get_unmapped_area,
>  };
> diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
> index f7ce40dedc91..e4063923115b 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.h
> +++ b/arch/x86/kernel/cpu/sgx/driver.h
> @@ -9,6 +9,7 @@
>  #include <linux/rwsem.h>
>  #include <linux/sched.h>
>  #include <linux/workqueue.h>
> +#include <uapi/asm/sgx.h>
>  #include "sgx.h"
>  
>  #define SGX_EINIT_SPIN_COUNT	20
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> new file mode 100644
> index 000000000000..7981c411b05a
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-19 Intel Corporation.
> +
> +#include <asm/mman.h>
> +#include <linux/mman.h>
> +#include <linux/delay.h>
> +#include <linux/file.h>
> +#include <linux/hashtable.h>
> +#include <linux/highmem.h>
> +#include <linux/ratelimit.h>
> +#include <linux/sched/signal.h>
> +#include <linux/shmem_fs.h>
> +#include <linux/slab.h>
> +#include <linux/suspend.h>
> +#include "driver.h"
> +#include "encl.h"
> +#include "encls.h"
> +
> +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++) {
> +		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)
> +{
> +	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;
> +}
> +
> +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> +{
> +	unsigned long encl_size = secs->size + PAGE_SIZE;
> +	struct sgx_epc_page *secs_epc;
> +	struct sgx_pageinfo pginfo;
> +	struct sgx_secinfo secinfo;
> +	struct file *backing;
> +	long ret;
> +
> +	if (sgx_validate_secs(secs)) {
> +		pr_debug("invalid SECS\n");
> +		return -EINVAL;
> +	}
> +
> +	backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
> +				   VM_NORESERVE);
> +	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->secs_attributes = secs->attributes;
> +	encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK;
> +	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);
> +
> +	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
> + * @filep:	open file to /dev/sgx
> + * @arg:	userspace pointer to a struct sgx_enclave_create instance
> + *
> + * Allocate kernel data structures for a new enclave and execute ECREATE after
> + * verifying the correctness of the provided SECS.
> + *
> + * Note, enforcement of restricted and disallowed attributes is deferred until
> + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
> + * checked by sgx_ioc_enclave_create().
> + *
> + * 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);
> +	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;
> +
> +	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;
> +}
> -- 
> 2.25.1

Borislav Petkov Aug. 26, 2020, 2:52 p.m. UTC | #2

On Thu, Jul 16, 2020 at 04:52:51PM +0300, Jarkko Sakkinen wrote:
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd6a11d..35f713e3a267 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -323,6 +323,7 @@ Code  Seq#    Include File                                           Comments
>                                                                       <mailto:[e-mail]>
>  0xA3  90-9F  linux/dtlk.h
>  0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
> +0xA4  00-1F  uapi/asm/sgx.h                                          Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)

Again, maybe add <mailto:[e-mail]> ?

This is from a previous review - please be more careful when addressing
review comments - either do them or object to them but silently ignoring
them is not cool.

>  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
>  0xAB  00-1F  linux/nbd.h
>  0xAC  00-1F  linux/raw.h
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> new file mode 100644
> index 000000000000..3787d278e84b
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */

checkpatch is not happy about something:

WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */' is not supported in LICENSES/...
#79: FILE: arch/x86/include/uapi/asm/sgx.h:1:
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */

...

> +/**
> + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> + * @filep:	open file to /dev/sgx

Also from a previous review:

"That's

@encl: enclave pointer

or so."

> + * @arg:	userspace pointer to a struct sgx_enclave_create instance
> + *
> + * Allocate kernel data structures for a new enclave and execute ECREATE after
> + * verifying the correctness of the provided SECS.
> + *
> + * Note, enforcement of restricted and disallowed attributes is deferred until
> + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
> + * checked by sgx_ioc_enclave_create().

From that same review:

"Well, I don't see that checking. Where is it?"

Ok, I'm going to stop here. Please go over v33's review and either
address *all* feedback or incorporate it into your patches if you agree
with it but do not silently ignore it. One of the things I very strongly
detest is ignored review comments.

Jarkko Sakkinen Aug. 27, 2020, 1:24 p.m. UTC | #3

On Wed, Aug 26, 2020 at 04:52:39PM +0200, Borislav Petkov wrote:
> On Thu, Jul 16, 2020 at 04:52:51PM +0300, Jarkko Sakkinen wrote:
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd6a11d..35f713e3a267 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -323,6 +323,7 @@ Code  Seq#    Include File                                           Comments
> >                                                                       <mailto:[e-mail]>
> >  0xA3  90-9F  linux/dtlk.h
> >  0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
> > +0xA4  00-1F  uapi/asm/sgx.h                                          Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> 
> Again, maybe add <mailto:[e-mail]> ?
> 
> This is from a previous review - please be more careful when addressing
> review comments - either do them or object to them but silently ignoring
> them is not cool.
> 
> >  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
> >  0xAB  00-1F  linux/nbd.h
> >  0xAC  00-1F  linux/raw.h
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > new file mode 100644
> > index 000000000000..3787d278e84b
> > --- /dev/null
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
> 
> checkpatch is not happy about something:
> 
> WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */' is not supported in LICENSES/...
> #79: FILE: arch/x86/include/uapi/asm/sgx.h:1:
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */

I don't know what has gone in my head when I wrote that but it looks
plain wrong even without running checkpatch.pl

The line should express the dijunction of "GPL-2.0+ WITH
Linux-syscall-note" and "BSD-3-Clause".

Grepping the kernel tree, I can find 34 instances of these in uapi
files:

  /* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */

I have not checked if this passes checkpatch.pl yet, but I would
be surprised if that did not pass (obviously I'll check that).

> 
> ...
> 
> > +/**
> > + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> > + * @filep:	open file to /dev/sgx
> 
> Also from a previous review:
> 
> "That's
> 
> @encl: enclave pointer
> 
> or so."

Yes, for sure. Thanks.

> > + * @arg:	userspace pointer to a struct sgx_enclave_create instance
> > + *
> > + * Allocate kernel data structures for a new enclave and execute ECREATE after
> > + * verifying the correctness of the provided SECS.
> > + *
> > + * Note, enforcement of restricted and disallowed attributes is deferred until
> > + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
> > + * checked by sgx_ioc_enclave_create().
> 
> From that same review:
> 
> "Well, I don't see that checking. Where is it?"
> 
> Ok, I'm going to stop here. Please go over v33's review and either
> address *all* feedback or incorporate it into your patches if you agree
> with it but do not silently ignore it. One of the things I very strongly
> detest is ignored review comments.

I'm sorry about that. This was not intentional. I'll revisit them by
going through all your responses from here:

  https://patchwork.kernel.org/patch/11581715/

v34 had the splitting of the big driver patch into multiple patches.

During that process I've obviously failed to address these.

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Borislav Petkov Aug. 27, 2020, 4:15 p.m. UTC | #4

On Thu, Aug 27, 2020 at 04:24:36PM +0300, Jarkko Sakkinen wrote:
> I have not checked if this passes checkpatch.pl yet, but I would
> be surprised if that did not pass (obviously I'll check that).

Right, when you're done with the patchset, just do

checkpatch.pl -g ...

on it before sending and you'll be good to go. Just remember to read the
suggestions checkpatch gives with turned on brain and sanity-check them
instead of blindly following them.

> I'm sorry about that. This was not intentional. I'll revisit them by
> going through all your responses from here:
> 
>   https://patchwork.kernel.org/patch/11581715/

Actually this one:

https://lkml.kernel.org/r/[e-mail]

i.e., the v33 version.

Also, make sure you go through the review comments of v34 and v35 in
case you haven't done so yet.

> v34 had the splitting of the big driver patch into multiple patches.
> 
> During that process I've obviously failed to address these.

Yeah, that can happen - I mean, this is not even close to being an easy
patchset so thanks for putting in the effort.

Thx.

Jarkko Sakkinen Aug. 28, 2020, 11:39 p.m. UTC | #5

On Thu, Aug 27, 2020 at 06:15:27PM +0200, Borislav Petkov wrote:
> On Thu, Aug 27, 2020 at 04:24:36PM +0300, Jarkko Sakkinen wrote:
> > I have not checked if this passes checkpatch.pl yet, but I would
> > be surprised if that did not pass (obviously I'll check that).
> 
> Right, when you're done with the patchset, just do
> 
> checkpatch.pl -g ...
> 
> on it before sending and you'll be good to go. Just remember to read the
> suggestions checkpatch gives with turned on brain and sanity-check them
> instead of blindly following them.
> 
> > I'm sorry about that. This was not intentional. I'll revisit them by
> > going through all your responses from here:
> > 
> >   https://patchwork.kernel.org/patch/11581715/
> 
> Actually this one:
> 
> https://lkml.kernel.org/r/[e-mail]
> 
> i.e., the v33 version.

Ya, pasted wrong link, sorry :-)

> 
> Also, make sure you go through the review comments of v34 and v35 in
> case you haven't done so yet.

I'll re-check them before I send a new version.

> > v34 had the splitting of the big driver patch into multiple patches.
> > 
> > During that process I've obviously failed to address these.
> 
> Yeah, that can happen - I mean, this is not even close to being an easy
> patchset so thanks for putting in the effort.

I'd guess that this will get less painful given that the patches are now
more reasonably sizeda after chopping the driver patch.

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Jarkko Sakkinen Aug. 29, 2020, 12:21 a.m. UTC | #6

On Thu, Aug 27, 2020 at 04:24:50PM +0300, Jarkko Sakkinen wrote:
> > > + * @arg:	userspace pointer to a struct sgx_enclave_create instance
> > > + *
> > > + * Allocate kernel data structures for a new enclave and execute ECREATE after
> > > + * verifying the correctness of the provided SECS.
> > > + *
> > > + * Note, enforcement of restricted and disallowed attributes is deferred until
> > > + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
> > > + * checked by sgx_ioc_enclave_create().
> > 
> > From that same review:
> > 
> > "Well, I don't see that checking. Where is it?"
> > 
> > Ok, I'm going to stop here. Please go over v33's review and either
> > address *all* feedback or incorporate it into your patches if you agree
> > with it but do not silently ignore it. One of the things I very strongly
> > detest is ignored review comments.

OK, so sgx_validate_secs() is the validation of what the CPU requires
from the contents of the SECS. That is mean by "architectural
correctness".

I spotted the glitch that makes this confusing.

The change that the comment is related is

https://lore.kernel.org/linux-sgx/[e-mail]/

This check in sgx_encl_init() should be relocated to this commit:

	/* Check that the required attributes have been authorized. */
	if (encl->secs_attributes & ~encl->allowed_attributes)
		return -EACCES;

It is the "enforcement of restricted and disallowed attributes" part.

Does this make sense to you?

> > -- 
> > Regards/Gruss,
> >     Boris.

/Jarkko

Haitao Huang Sept. 1, 2020, 4:41 p.m. UTC | #7

On Thu, 16 Jul 2020 08:52:51 -0500, Jarkko Sakkinen  
<[e-mail]> wrote:

> Add an ioctl that performs ENCLS[ECREATE], which creates SGX Enclave
> Control Structure for the enclave. SECS contains attributes about the
> enclave that are used by the hardware and cannot be directly accessed by
> software, as SECS resides in the EPC.
>
> One essential field in SECS is a field that stores the SHA256 of the
> measured enclave pages. This field, MRENCLAVE, is initialized by the
> ECREATE instruction and updated by every EADD and EEXTEND operation.
> Finally, EINIT locks down the value.
>
> Acked-by: Jethro Beekman <[e-mail]>
> Tested-by: Jethro Beekman <[e-mail]>
> Tested-by: Haitao Huang <[e-mail]>
> Tested-by: Chunyang Hui <[e-mail]>
> Tested-by: Jordan Hand <[e-mail]>
> Tested-by: Nathaniel McCallum <[e-mail]>
> Tested-by: Seth Moore <[e-mail]>
> Co-developed-by: Sean Christopherson <[e-mail]>
> Signed-off-by: Sean Christopherson <[e-mail]>
> Co-developed-by: Suresh Siddha <[e-mail]>
> Signed-off-by: Suresh Siddha <[e-mail]>
> Signed-off-by: Jarkko Sakkinen <[e-mail]>
> ---
>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>  arch/x86/include/uapi/asm/sgx.h               |  25 ++
>  arch/x86/kernel/cpu/sgx/Makefile              |   1 +
>  arch/x86/kernel/cpu/sgx/driver.c              |  12 +
>  arch/x86/kernel/cpu/sgx/driver.h              |   1 +
>  arch/x86/kernel/cpu/sgx/ioctl.c               | 226 ++++++++++++++++++
>  6 files changed, 266 insertions(+)
>  create mode 100644 arch/x86/include/uapi/asm/sgx.h
>  create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst  
> b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd6a11d..35f713e3a267 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -323,6 +323,7 @@ Code  Seq#    Include  
> File                                           Comments
>                                                                       <mailto:[e-mail]>
>  0xA3  90-9F  linux/dtlk.h
>  0xA4  00-1F  uapi/linux/tee.h                                         
> Generic TEE subsystem
> +0xA4  00-1F  uapi/asm/sgx.h                                           
> Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
>  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
>  0xAB  00-1F  linux/nbd.h
>  0xAC  00-1F  linux/raw.h
> diff --git a/arch/x86/include/uapi/asm/sgx.h  
> b/arch/x86/include/uapi/asm/sgx.h
> new file mode 100644
> index 000000000000..3787d278e84b
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH  
> Linux-syscall-note */
> +/*
> + * Copyright(c) 2016-19 Intel Corporation.
> + */
> +#ifndef _UAPI_ASM_X86_SGX_H
> +#define _UAPI_ASM_X86_SGX_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define SGX_MAGIC 0xA4
> +
> +#define SGX_IOC_ENCLAVE_CREATE \
> +	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
> +
> +/**
> + * struct sgx_enclave_create - parameter structure for the
> + *                             %SGX_IOC_ENCLAVE_CREATE ioctl
> + * @src:	address for the SECS page data
> + */
> +struct sgx_enclave_create  {
> +	__u64	src;
> +};
> +
> +#endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/Makefile  
> b/arch/x86/kernel/cpu/sgx/Makefile
> index 3fc451120735..91d3dc784a29 100644
> --- a/arch/x86/kernel/cpu/sgx/Makefile
> +++ b/arch/x86/kernel/cpu/sgx/Makefile
> @@ -1,4 +1,5 @@
>  obj-y += \
>  	driver.o \
>  	encl.o \
> +	ioctl.o \
>  	main.o
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c  
> b/arch/x86/kernel/cpu/sgx/driver.c
> index b52520407f5b..5559bc18de41 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -118,10 +118,22 @@ static unsigned long sgx_get_unmapped_area(struct  
> file *file,
>  	return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
>  }
> +#ifdef CONFIG_COMPAT
> +static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
> +			      unsigned long arg)
> +{
> +	return sgx_ioctl(filep, cmd, arg);
> +}
> +#endif
> +
>  static const struct file_operations sgx_encl_fops = {
>  	.owner			= THIS_MODULE,
>  	.open			= sgx_open,
>  	.release		= sgx_release,
> +	.unlocked_ioctl		= sgx_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl		= sgx_compat_ioctl,
> +#endif
>  	.mmap			= sgx_mmap,
>  	.get_unmapped_area	= sgx_get_unmapped_area,
>  };
> diff --git a/arch/x86/kernel/cpu/sgx/driver.h  
> b/arch/x86/kernel/cpu/sgx/driver.h
> index f7ce40dedc91..e4063923115b 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.h
> +++ b/arch/x86/kernel/cpu/sgx/driver.h
> @@ -9,6 +9,7 @@
>  #include <linux/rwsem.h>
>  #include <linux/sched.h>
>  #include <linux/workqueue.h>
> +#include <uapi/asm/sgx.h>
>  #include "sgx.h"
> #define SGX_EINIT_SPIN_COUNT	20
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c  
> b/arch/x86/kernel/cpu/sgx/ioctl.c
> new file mode 100644
> index 000000000000..7981c411b05a
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-19 Intel Corporation.
> +
> +#include <asm/mman.h>
> +#include <linux/mman.h>
> +#include <linux/delay.h>
> +#include <linux/file.h>
> +#include <linux/hashtable.h>
> +#include <linux/highmem.h>
> +#include <linux/ratelimit.h>
> +#include <linux/sched/signal.h>
> +#include <linux/shmem_fs.h>
> +#include <linux/slab.h>
> +#include <linux/suspend.h>
> +#include "driver.h"
> +#include "encl.h"
> +#include "encls.h"
> +
> +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++) {
> +		if (!((1 << i) & xfrm))
> +			continue;
> +

Potential overflow for signed integer shift. Originally spotted and fixed  
in OOT from Colin I. K.: https://github.com/intel/linux-sgx-driver/pull/16

Thanks
Haitao

Jarkko Sakkinen Sept. 4, 2020, 11:55 a.m. UTC | #8

On Tue, Sep 01, 2020 at 11:41:14AM -0500, Haitao Huang wrote:
> On Thu, 16 Jul 2020 08:52:51 -0500, Jarkko Sakkinen
> <[e-mail]> wrote:
> 
> > Add an ioctl that performs ENCLS[ECREATE], which creates SGX Enclave
> > Control Structure for the enclave. SECS contains attributes about the
> > enclave that are used by the hardware and cannot be directly accessed by
> > software, as SECS resides in the EPC.
> > 
> > One essential field in SECS is a field that stores the SHA256 of the
> > measured enclave pages. This field, MRENCLAVE, is initialized by the
> > ECREATE instruction and updated by every EADD and EEXTEND operation.
> > Finally, EINIT locks down the value.
> > 
> > Acked-by: Jethro Beekman <[e-mail]>
> > Tested-by: Jethro Beekman <[e-mail]>
> > Tested-by: Haitao Huang <[e-mail]>
> > Tested-by: Chunyang Hui <[e-mail]>
> > Tested-by: Jordan Hand <[e-mail]>
> > Tested-by: Nathaniel McCallum <[e-mail]>
> > Tested-by: Seth Moore <[e-mail]>
> > Co-developed-by: Sean Christopherson <[e-mail]>
> > Signed-off-by: Sean Christopherson <[e-mail]>
> > Co-developed-by: Suresh Siddha <[e-mail]>
> > Signed-off-by: Suresh Siddha <[e-mail]>
> > Signed-off-by: Jarkko Sakkinen <[e-mail]>
> > ---
> >  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
> >  arch/x86/include/uapi/asm/sgx.h               |  25 ++
> >  arch/x86/kernel/cpu/sgx/Makefile              |   1 +
> >  arch/x86/kernel/cpu/sgx/driver.c              |  12 +
> >  arch/x86/kernel/cpu/sgx/driver.h              |   1 +
> >  arch/x86/kernel/cpu/sgx/ioctl.c               | 226 ++++++++++++++++++
> >  6 files changed, 266 insertions(+)
> >  create mode 100644 arch/x86/include/uapi/asm/sgx.h
> >  create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
> > 
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd6a11d..35f713e3a267 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -323,6 +323,7 @@ Code  Seq#    Include File
> > Comments
> >                                                                       <mailto:[e-mail]>
> >  0xA3  90-9F  linux/dtlk.h
> >  0xA4  00-1F  uapi/linux/tee.h
> > Generic TEE subsystem
> > +0xA4  00-1F  uapi/asm/sgx.h
> > Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> >  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
> >  0xAB  00-1F  linux/nbd.h
> >  0xAC  00-1F  linux/raw.h
> > diff --git a/arch/x86/include/uapi/asm/sgx.h
> > b/arch/x86/include/uapi/asm/sgx.h
> > new file mode 100644
> > index 000000000000..3787d278e84b
> > --- /dev/null
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH
> > Linux-syscall-note */
> > +/*
> > + * Copyright(c) 2016-19 Intel Corporation.
> > + */
> > +#ifndef _UAPI_ASM_X86_SGX_H
> > +#define _UAPI_ASM_X86_SGX_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/ioctl.h>
> > +
> > +#define SGX_MAGIC 0xA4
> > +
> > +#define SGX_IOC_ENCLAVE_CREATE \
> > +	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
> > +
> > +/**
> > + * struct sgx_enclave_create - parameter structure for the
> > + *                             %SGX_IOC_ENCLAVE_CREATE ioctl
> > + * @src:	address for the SECS page data
> > + */
> > +struct sgx_enclave_create  {
> > +	__u64	src;
> > +};
> > +
> > +#endif /* _UAPI_ASM_X86_SGX_H */
> > diff --git a/arch/x86/kernel/cpu/sgx/Makefile
> > b/arch/x86/kernel/cpu/sgx/Makefile
> > index 3fc451120735..91d3dc784a29 100644
> > --- a/arch/x86/kernel/cpu/sgx/Makefile
> > +++ b/arch/x86/kernel/cpu/sgx/Makefile
> > @@ -1,4 +1,5 @@
> >  obj-y += \
> >  	driver.o \
> >  	encl.o \
> > +	ioctl.o \
> >  	main.o
> > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > b/arch/x86/kernel/cpu/sgx/driver.c
> > index b52520407f5b..5559bc18de41 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > @@ -118,10 +118,22 @@ static unsigned long sgx_get_unmapped_area(struct
> > file *file,
> >  	return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> >  }
> > +#ifdef CONFIG_COMPAT
> > +static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
> > +			      unsigned long arg)
> > +{
> > +	return sgx_ioctl(filep, cmd, arg);
> > +}
> > +#endif
> > +
> >  static const struct file_operations sgx_encl_fops = {
> >  	.owner			= THIS_MODULE,
> >  	.open			= sgx_open,
> >  	.release		= sgx_release,
> > +	.unlocked_ioctl		= sgx_ioctl,
> > +#ifdef CONFIG_COMPAT
> > +	.compat_ioctl		= sgx_compat_ioctl,
> > +#endif
> >  	.mmap			= sgx_mmap,
> >  	.get_unmapped_area	= sgx_get_unmapped_area,
> >  };
> > diff --git a/arch/x86/kernel/cpu/sgx/driver.h
> > b/arch/x86/kernel/cpu/sgx/driver.h
> > index f7ce40dedc91..e4063923115b 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver.h
> > +++ b/arch/x86/kernel/cpu/sgx/driver.h
> > @@ -9,6 +9,7 @@
> >  #include <linux/rwsem.h>
> >  #include <linux/sched.h>
> >  #include <linux/workqueue.h>
> > +#include <uapi/asm/sgx.h>
> >  #include "sgx.h"
> > #define SGX_EINIT_SPIN_COUNT	20
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > b/arch/x86/kernel/cpu/sgx/ioctl.c
> > new file mode 100644
> > index 000000000000..7981c411b05a
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -0,0 +1,226 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> > +// Copyright(c) 2016-19 Intel Corporation.
> > +
> > +#include <asm/mman.h>
> > +#include <linux/mman.h>
> > +#include <linux/delay.h>
> > +#include <linux/file.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/highmem.h>
> > +#include <linux/ratelimit.h>
> > +#include <linux/sched/signal.h>
> > +#include <linux/shmem_fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/suspend.h>
> > +#include "driver.h"
> > +#include "encl.h"
> > +#include "encls.h"
> > +
> > +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++) {
> > +		if (!((1 << i) & xfrm))
> > +			continue;
> > +
> 
> Potential overflow for signed integer shift. Originally spotted and fixed in
> OOT from Colin I. K.: https://github.com/intel/linux-sgx-driver/pull/16
> 
> Thanks
> Haitao

Thank you. It is true that this also applies to the mainline SGX.

I fixed this and added this entry to v37 changelog:

* Fixed signed integer shift overflow.
  https://github.com/intel/linux-sgx-driver/pull/16/commits/d27ca6071b2b28e2e789d265eda796dd9fc65a64

/Jarkko

Patch

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 59472cd6a11d..35f713e3a267 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -323,6 +323,7 @@  Code  Seq#    Include File                                           Comments
                                                                      <mailto:[e-mail]>
 0xA3  90-9F  linux/dtlk.h
 0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
+0xA4  00-1F  uapi/asm/sgx.h                                          Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
 0xAA  00-3F  linux/uapi/linux/userfaultfd.h
 0xAB  00-1F  linux/nbd.h
 0xAC  00-1F  linux/raw.h
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
new file mode 100644
index 000000000000..3787d278e84b
--- /dev/null
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -0,0 +1,25 @@ 
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
+/*
+ * Copyright(c) 2016-19 Intel Corporation.
+ */
+#ifndef _UAPI_ASM_X86_SGX_H
+#define _UAPI_ASM_X86_SGX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define SGX_MAGIC 0xA4
+
+#define SGX_IOC_ENCLAVE_CREATE \
+	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
+
+/**
+ * struct sgx_enclave_create - parameter structure for the
+ *                             %SGX_IOC_ENCLAVE_CREATE ioctl
+ * @src:	address for the SECS page data
+ */
+struct sgx_enclave_create  {
+	__u64	src;
+};
+
+#endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/Makefile b/arch/x86/kernel/cpu/sgx/Makefile
index 3fc451120735..91d3dc784a29 100644
--- a/arch/x86/kernel/cpu/sgx/Makefile
+++ b/arch/x86/kernel/cpu/sgx/Makefile
@@ -1,4 +1,5 @@ 
 obj-y += \
 	driver.o \
 	encl.o \
+	ioctl.o \
 	main.o
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index b52520407f5b..5559bc18de41 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -118,10 +118,22 @@  static unsigned long sgx_get_unmapped_area(struct file *file,
 	return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
 }
 
+#ifdef CONFIG_COMPAT
+static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
+			      unsigned long arg)
+{
+	return sgx_ioctl(filep, cmd, arg);
+}
+#endif
+
 static const struct file_operations sgx_encl_fops = {
 	.owner			= THIS_MODULE,
 	.open			= sgx_open,
 	.release		= sgx_release,
+	.unlocked_ioctl		= sgx_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl		= sgx_compat_ioctl,
+#endif
 	.mmap			= sgx_mmap,
 	.get_unmapped_area	= sgx_get_unmapped_area,
 };
diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
index f7ce40dedc91..e4063923115b 100644
--- a/arch/x86/kernel/cpu/sgx/driver.h
+++ b/arch/x86/kernel/cpu/sgx/driver.h
@@ -9,6 +9,7 @@ 
 #include <linux/rwsem.h>
 #include <linux/sched.h>
 #include <linux/workqueue.h>
+#include <uapi/asm/sgx.h>
 #include "sgx.h"
 
 #define SGX_EINIT_SPIN_COUNT	20
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
new file mode 100644
index 000000000000..7981c411b05a
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -0,0 +1,226 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-19 Intel Corporation.
+
+#include <asm/mman.h>
+#include <linux/mman.h>
+#include <linux/delay.h>
+#include <linux/file.h>
+#include <linux/hashtable.h>
+#include <linux/highmem.h>
+#include <linux/ratelimit.h>
+#include <linux/sched/signal.h>
+#include <linux/shmem_fs.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+#include "driver.h"
+#include "encl.h"
+#include "encls.h"
+
+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++) {
+		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)
+{
+	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;
+}
+
+static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
+{
+	unsigned long encl_size = secs->size + PAGE_SIZE;
+	struct sgx_epc_page *secs_epc;
+	struct sgx_pageinfo pginfo;
+	struct sgx_secinfo secinfo;
+	struct file *backing;
+	long ret;
+
+	if (sgx_validate_secs(secs)) {
+		pr_debug("invalid SECS\n");
+		return -EINVAL;
+	}
+
+	backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
+				   VM_NORESERVE);
+	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->secs_attributes = secs->attributes;
+	encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK;
+	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);
+
+	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
+ * @filep:	open file to /dev/sgx
+ * @arg:	userspace pointer to a struct sgx_enclave_create instance
+ *
+ * Allocate kernel data structures for a new enclave and execute ECREATE after
+ * verifying the correctness of the provided SECS.
+ *
+ * Note, enforcement of restricted and disallowed attributes is deferred until
+ * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
+ * checked by sgx_ioc_enclave_create().
+ *
+ * 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);
+	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;
+
+	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;
+}