Re: [PATCH v38 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Jarkko Sakkinen]

On Fri, Oct 02, 2020 at 07:23:55PM -0500, Haitao Huang wrote:
> On Tue, 15 Sep 2020 06:28:30 -0500, 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 
> > Tested-by: Jethro Beekman 
> > Tested-by: Haitao Huang 
> > Tested-by: Chunyang Hui 
> > Tested-by: Jordan Hand 
> > Tested-by: Nathaniel McCallum 
> > Tested-by: Seth Moore 
> > Tested-by: Darren Kenny 
> > Reviewed-by: Darren Kenny 
> > Co-developed-by: Sean Christopherson 
> > Signed-off-by: Sean Christopherson 
> > Co-developed-by: Suresh Siddha 
> > Signed-off-by: Suresh Siddha 
> > Signed-off-by: Jarkko Sakkinen 
> > ---
> >  .../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   | 220 ++
> >  6 files changed, 260 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 2a198838fca9..a89e1c46a25a 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
> >   
> > 
> >  0xA3  90-9F  linux/dtlk.h
> >  0xA4  00-1F  uapi/linux/tee.h
> > Generic TEE subsystem
> > +0xA4  00-1F  uapi/asm/sgx.h
> > 
> >  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 ..c75b375f3770
> > --- /dev/null
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR
> > BSD-3-Clause) */
> > +/*
> > + * Copyright(c) 2016-19 Intel Corporation.
> > + */
> > +#ifndef _UAPI_ASM_X86_SGX_H
> > +#define _UAPI_ASM_X86_SGX_H
> > +
> > +#include 
> > +#include 
> > +
> > +#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 f54da5f19c2b..7bdb49dfcca6 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > @@ -114,10 +114,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 
> >  #include 
> >  #include 
> 

Re: [PATCH v38 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Haitao Huang]

On Tue, 15 Sep 2020 06:28:30 -0500, 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 
Tested-by: Jethro Beekman 
Tested-by: Haitao Huang 
Tested-by: Chunyang Hui 
Tested-by: Jordan Hand 
Tested-by: Nathaniel McCallum 
Tested-by: Seth Moore 
Tested-by: Darren Kenny 
Reviewed-by: Darren Kenny 
Co-developed-by: Sean Christopherson 
Signed-off-by: Sean Christopherson 
Co-developed-by: Suresh Siddha 
Signed-off-by: Suresh Siddha 
Signed-off-by: Jarkko Sakkinen 
---
 .../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   | 220 ++
 6 files changed, 260 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 2a198838fca9..a89e1c46a25a 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

  

 0xA3  90-9F  linux/dtlk.h
 0xA4  00-1F  uapi/linux/tee.h 
Generic TEE subsystem
+0xA4  00-1F  uapi/asm/sgx.h   


 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 ..c75b375f3770
--- /dev/null
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR  
BSD-3-Clause) */

+/*
+ * Copyright(c) 2016-19 Intel Corporation.
+ */
+#ifndef _UAPI_ASM_X86_SGX_H
+#define _UAPI_ASM_X86_SGX_H
+
+#include 
+#include 
+
+#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 f54da5f19c2b..7bdb49dfcca6 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -114,10 +114,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 
 #include 
 #include 
+#include 
 #include "sgx.h"
#define SGX_EINIT_SPIN_COUNT20
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c  
b/arch/x86/kernel/cpu/sgx/ioctl.c

new file mode 100644
index ..352a3c461812
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-19 Intel Corporation.
+

Re: [PATCH v38 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Jarkko Sakkinen]

On Mon, Sep 21, 2020 at 03:51:07PM +0200, Borislav Petkov wrote:
> > "... after checking that the provided data for SECS meets the expectations
> > of ENCLS[ECREATE] for an unitialized enclave and size of the address

There is a typo (should be uninitialized).

> > Is this sufficient for you, or do you have further suggestions?
> 
> Yes, no further suggestions.
> 
> Thx.


Great, thank you.

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

/Jarkko

Re: [PATCH v38 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Borislav Petkov]

On Mon, Sep 21, 2020 at 03:28:23PM +0300, Jarkko Sakkinen wrote:
> Is this appropriate:
> 
>   /* The extra page in swap space goes to SECS. */
>   encl_size = secs->size + PAGE_SIZE;
> 
>   backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
>  VM_NORESERVE);
>   if (IS_ERR(backing)) {
>   ret = PTR_ERR(backing);
>   goto err_out_shrink;
>   }
>

Yap, thanks.

> I agree with this but I also think it would make sense to rephrase
> "verifying the correctness of the provided SECS" with something more
> informative.
> 
> I would rephrase as:
> 
> "... after checking that the provided data for SECS meets the expectations
> of ENCLS[ECREATE] for an unitialized enclave and size of the address
> space does not surpass the platform expectations. This validation is
> executed by sgx_validate_secs()."

s/executed/done/

> Is this sufficient for you, or do you have further suggestions?

Yes, no further suggestions.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v38 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Jarkko Sakkinen]

On Mon, Sep 21, 2020 at 12:03:56PM +0200, Borislav Petkov wrote:
> On Tue, Sep 15, 2020 at 02:28:30PM +0300, Jarkko Sakkinen wrote:
> > +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)))
> 
> Let that last line stick out so that you have each statement on a single line.
> 
> > +   return -EINVAL;
> > +
> > +   if (!secs->ssa_frame_size)
> > +   return -EINVAL;
> > +
> > +   if (sgx_calc_ssa_frame_size(secs->miscselect, secs->xfrm) >
> > +   secs->ssa_frame_size)
> 
> Let that stick out.
> 
> > +   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;
> 
> You're still using secs->size before validation. I know, we will return
> early if sgx_validate_secs() fails but pls move that addition after the
> validation call.

Is this appropriate:

/* The extra page in swap space goes to SECS. */
encl_size = secs->size + PAGE_SIZE;

backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
   VM_NORESERVE);
if (IS_ERR(backing)) {
ret = PTR_ERR(backing);
goto err_out_shrink;
}

> ...
> 
> > +/**
> > + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> > + * @filep: open file to /dev/sgx
> 
> Dammit, how many times do I have to type this comment here?!
> 
> "That's
> 
> @encl: enclave pointer
> 
> or so."
> 
> There's no filep - there is an encl!

I'm not actually sure what has happened. As you can easily grep, the
rename is done in five other sites. I also see a similar problem in
EINIT, which I will fix.

git grep "enclave pointer" arch/x86/kernel/cpu/sgx/ | wc -l
5

> > + * @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.
> 
> ... which is done in sgx_validate_secs()."
> 
> Yes, spell it out, otherwise one has to wonder where that validation is
> happening in the function *below* because the comment is over it - not
> over sgx_validate_secs().
> 
> And yes, you need to spell stuff like that out because this SGX crap is
> complex and it better be properly documented!

I agree with this but I also think it would make sense to rephrase
"verifying the correctness of the provided SECS" with something more
informative.

I would rephrase as:

"... after checking that the provided data for SECS meets the expectations
of ENCLS[ECREATE] for an unitialized enclave and size of the address
space does not surpass the platform expectations. This validation is
executed by sgx_validate_secs()."

Is this sufficient for you, or do you have further suggestions?

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

/Jarkko

Re: [PATCH v38 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Borislav Petkov]

On Tue, Sep 15, 2020 at 02:28:30PM +0300, Jarkko Sakkinen wrote:
> +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)))

Let that last line stick out so that you have each statement on a single line.

> + return -EINVAL;
> +
> + if (!secs->ssa_frame_size)
> + return -EINVAL;
> +
> + if (sgx_calc_ssa_frame_size(secs->miscselect, secs->xfrm) >
> + secs->ssa_frame_size)

Let that stick out.

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

You're still using secs->size before validation. I know, we will return
early if sgx_validate_secs() fails but pls move that addition after the
validation call.

...

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

Dammit, how many times do I have to type this comment here?!

"That's

@encl: enclave pointer

or so."

There's no filep - there is an encl!


> + * @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.

... which is done in sgx_validate_secs()."

Yes, spell it out, otherwise one has to wonder where that validation is
happening in the function *below* because the comment is over it - not
over sgx_validate_secs().

And yes, you need to spell stuff like that out because this SGX crap is
complex and it better be properly documented!

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette