Re: [PATCH v36 11/24] x86/sgx: Add SGX enclave driver
On Tue, Aug 25, 2020 at 06:44:12PM +0200, Borislav Petkov wrote:
> On Thu, Jul 16, 2020 at 04:52:50PM +0300, Jarkko Sakkinen wrote:
>
> Just minor things below - I'm not even going to pretend I fully
> understand what's going on but FWICT, it looks non-threateningly ok to
> me.
>
> > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > b/arch/x86/kernel/cpu/sgx/driver.c
> > new file mode 100644
> > index ..b52520407f5b
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > @@ -0,0 +1,177 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> > +// Copyright(c) 2016-18 Intel Corporation.
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "driver.h"
> > +#include "encl.h"
> > +
> > +MODULE_DESCRIPTION("Intel SGX Enclave Driver");
> > +MODULE_AUTHOR("Jarkko Sakkinen ");
> > +MODULE_LICENSE("Dual BSD/GPL");
>
> That boilerplate stuff usually goes to the end of the file.
These all are cruft from the times when we still had a kernel module.
I.e. I'll just remove them.
>
> ...
>
> > +static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> > + unsigned long addr)
> > +{
> > + struct sgx_encl_page *entry;
> > + unsigned int flags;
> > +
> > + /* If process was forked, VMA is still there but vm_private_data is set
> > +* to NULL.
> > +*/
> > + if (!encl)
> > + return ERR_PTR(-EFAULT);
> > +
> > + flags = atomic_read(>flags);
> > +
>
> ^ Superfluous newline.
>
> > + if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED))
> > + return ERR_PTR(-EFAULT);
> > +
> > + entry = xa_load(>page_array, PFN_DOWN(addr));
> > + if (!entry)
> > + return ERR_PTR(-EFAULT);
> > +
> > + /* Page is already resident in the EPC. */
> > + if (entry->epc_page)
> > + return entry;
> > +
> > + return ERR_PTR(-EFAULT);
> > +}
> > +
> > +static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
> > +struct mm_struct *mm)
> > +{
> > + struct sgx_encl_mm *encl_mm =
> > + container_of(mn, struct sgx_encl_mm, mmu_notifier);
>
> Just let it stick out.
>
> > + struct sgx_encl_mm *tmp = NULL;
> > +
> > + /*
> > +* The enclave itself can remove encl_mm. Note, objects can't be moved
> > +* off an RCU protected list, but deletion is ok.
> > +*/
> > + spin_lock(_mm->encl->mm_lock);
> > + list_for_each_entry(tmp, _mm->encl->mm_list, list) {
> > + if (tmp == encl_mm) {
> > + list_del_rcu(_mm->list);
> > + break;
> > + }
> > + }
> > + spin_unlock(_mm->encl->mm_lock);
> > +
> > + if (tmp == encl_mm) {
> > + synchronize_srcu(_mm->encl->srcu);
> > + mmu_notifier_put(mn);
> > + }
> > +}
> > +
> > +static void sgx_mmu_notifier_free(struct mmu_notifier *mn)
> > +{
> > + struct sgx_encl_mm *encl_mm =
> > + container_of(mn, struct sgx_encl_mm, mmu_notifier);
>
> Ditto.
>
> ...
>
> > +/**
> > + * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
> > + * @encl: an enclave
> > + * @start: lower bound of the address range, inclusive
> > + * @end: upper bound of the address range, exclusive
> > + * @vm_prot_bits: requested protections of the address range
> > + *
> > + * Iterate through the enclave pages contained within [@start, @end) to
> > verify
> > + * the permissions requested by @vm_prot_bits do not exceed that of any
> > enclave
> > + * page to be mapped.
> > + *
> > + * Return:
> > + * 0 on success,
> > + * -EACCES if VMA permissions exceed enclave page permissions
> > + */
> > +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> > +unsigned long end, unsigned long vm_flags)
> > +{
> > + unsigned long vm_prot_bits = vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
> > + unsigned long idx_start = PFN_DOWN(start);
> > + unsigned long idx_end = PFN_DOWN(end - 1);
> > + struct sgx_encl_page *page;
> > + XA_STATE(xas, >page_array, idx_start);
> > +
> > + /*
> > +* Disallow RIE tasks as their VMA permissions might conflict with the
>
> "RIE", hmm what is that?
>
> /me looks at the test
>
> Aaah, READ_IMPLIES_EXEC. Is "RIE" some widely accepted acronym I'm not
> aware of?
I think it was used in some email discussions related to this piece of
code but I'm happy to write it as READ_IMPLIES_EXEC :-)
>
> > +* enclave page permissions.
> > +*/
> > + if (!!(current->personality & READ_IMPLIES_EXEC))
>
> The "!!" is not really needed - you're in boolean context.
>
> ...
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
Thanks for the remarks.
/Jarkko
Re: [PATCH v36 11/24] x86/sgx: Add SGX enclave driver
On Thu, Jul 16, 2020 at 04:52:50PM +0300, Jarkko Sakkinen wrote:
Just minor things below - I'm not even going to pretend I fully
understand what's going on but FWICT, it looks non-threateningly ok to
me.
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> new file mode 100644
> index ..b52520407f5b
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "driver.h"
> +#include "encl.h"
> +
> +MODULE_DESCRIPTION("Intel SGX Enclave Driver");
> +MODULE_AUTHOR("Jarkko Sakkinen ");
> +MODULE_LICENSE("Dual BSD/GPL");
That boilerplate stuff usually goes to the end of the file.
...
> +static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
> + unsigned long addr)
> +{
> + struct sgx_encl_page *entry;
> + unsigned int flags;
> +
> + /* If process was forked, VMA is still there but vm_private_data is set
> + * to NULL.
> + */
> + if (!encl)
> + return ERR_PTR(-EFAULT);
> +
> + flags = atomic_read(>flags);
> +
^ Superfluous newline.
> + if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED))
> + return ERR_PTR(-EFAULT);
> +
> + entry = xa_load(>page_array, PFN_DOWN(addr));
> + if (!entry)
> + return ERR_PTR(-EFAULT);
> +
> + /* Page is already resident in the EPC. */
> + if (entry->epc_page)
> + return entry;
> +
> + return ERR_PTR(-EFAULT);
> +}
> +
> +static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
> + struct mm_struct *mm)
> +{
> + struct sgx_encl_mm *encl_mm =
> + container_of(mn, struct sgx_encl_mm, mmu_notifier);
Just let it stick out.
> + struct sgx_encl_mm *tmp = NULL;
> +
> + /*
> + * The enclave itself can remove encl_mm. Note, objects can't be moved
> + * off an RCU protected list, but deletion is ok.
> + */
> + spin_lock(_mm->encl->mm_lock);
> + list_for_each_entry(tmp, _mm->encl->mm_list, list) {
> + if (tmp == encl_mm) {
> + list_del_rcu(_mm->list);
> + break;
> + }
> + }
> + spin_unlock(_mm->encl->mm_lock);
> +
> + if (tmp == encl_mm) {
> + synchronize_srcu(_mm->encl->srcu);
> + mmu_notifier_put(mn);
> + }
> +}
> +
> +static void sgx_mmu_notifier_free(struct mmu_notifier *mn)
> +{
> + struct sgx_encl_mm *encl_mm =
> + container_of(mn, struct sgx_encl_mm, mmu_notifier);
Ditto.
...
> +/**
> + * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
> + * @encl:an enclave
> + * @start: lower bound of the address range, inclusive
> + * @end: upper bound of the address range, exclusive
> + * @vm_prot_bits:requested protections of the address range
> + *
> + * Iterate through the enclave pages contained within [@start, @end) to
> verify
> + * the permissions requested by @vm_prot_bits do not exceed that of any
> enclave
> + * page to be mapped.
> + *
> + * Return:
> + * 0 on success,
> + * -EACCES if VMA permissions exceed enclave page permissions
> + */
> +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> + unsigned long end, unsigned long vm_flags)
> +{
> + unsigned long vm_prot_bits = vm_flags & (VM_READ | VM_WRITE | VM_EXEC);
> + unsigned long idx_start = PFN_DOWN(start);
> + unsigned long idx_end = PFN_DOWN(end - 1);
> + struct sgx_encl_page *page;
> + XA_STATE(xas, >page_array, idx_start);
> +
> + /*
> + * Disallow RIE tasks as their VMA permissions might conflict with the
"RIE", hmm what is that?
/me looks at the test
Aaah, READ_IMPLIES_EXEC. Is "RIE" some widely accepted acronym I'm not
aware of?
> + * enclave page permissions.
> + */
> + if (!!(current->personality & READ_IMPLIES_EXEC))
The "!!" is not really needed - you're in boolean context.
...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v36 11/24] x86/sgx: Add SGX enclave driver
On Thursday, 2020-07-16 at 16:52:50 +03, Jarkko Sakkinen wrote:
> Intel Software Guard eXtensions (SGX) is a set of CPU instructions that can
> be used by applications to set aside private regions of code and data. The
> code outside the SGX hosted software entity is prevented from accessing the
> memory inside the enclave by the CPU. We call these entities enclaves.
>
> Add a driver that provides an ioctl API to construct and run enclaves.
> Enclaves are constructed from pages residing in reserved physical memory
> areas. The contents of these pages can only be accessed when they are
> mapped as part of an enclave, by a hardware thread running inside the
> enclave.
>
> The starting state of an enclave consists of a fixed measured set of
> pages that are copied to the EPC during the construction process by
> using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> that defines the enclave properties.
>
> Enclaves are constructed by using ENCLS leaf functions ECREATE, EADD and
> EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> the EPC and EINIT checks a given signed measurement and moves the enclave
> into a state ready for execution.
>
> An initialized enclave can only be accessed through special Thread Control
> Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER. This leaf
> function converts a thread into enclave mode and continues the execution in
> the offset defined by the TCS provided to EENTER. An enclave is exited
> through syscall, exception, interrupts or by explicitly calling another
> ENCLU leaf EEXIT.
>
> The mmap() permissions are capped by the contained enclave page
> permissions. The mapped areas must also be opaque, i.e. each page address
> must contain a page. This logic is implemented in sgx_encl_may_map().
>
> Cc: linux-security-mod...@vger.kernel.org
> Cc: linux...@kvack.org
> Cc: Andrew Morton
> Cc: Matthew Wilcox
> 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
> ---
> arch/x86/kernel/cpu/sgx/Makefile | 2 +
> arch/x86/kernel/cpu/sgx/driver.c | 177
> arch/x86/kernel/cpu/sgx/driver.h | 29 +++
> arch/x86/kernel/cpu/sgx/encl.c | 333 +++
> arch/x86/kernel/cpu/sgx/encl.h | 87
> arch/x86/kernel/cpu/sgx/main.c | 11 +
> 6 files changed, 639 insertions(+)
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
>
> diff --git a/arch/x86/kernel/cpu/sgx/Makefile
> b/arch/x86/kernel/cpu/sgx/Makefile
> index 79510ce01b3b..3fc451120735 100644
> --- a/arch/x86/kernel/cpu/sgx/Makefile
> +++ b/arch/x86/kernel/cpu/sgx/Makefile
> @@ -1,2 +1,4 @@
> obj-y += \
> + driver.o \
> + encl.o \
> main.o
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> new file mode 100644
> index ..b52520407f5b
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2016-18 Intel Corporation.
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "driver.h"
> +#include "encl.h"
> +
> +MODULE_DESCRIPTION("Intel SGX Enclave Driver");
> +MODULE_AUTHOR("Jarkko Sakkinen ");
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +u64 sgx_encl_size_max_32;
> +u64 sgx_encl_size_max_64;
> +u32 sgx_misc_reserved_mask;
> +u64 sgx_attributes_reserved_mask;
> +u64 sgx_xfrm_reserved_mask = ~0x3;
> +u32 sgx_xsave_size_tbl[64];
> +
> +static int sgx_open(struct inode *inode, struct file *file)
> +{
> + struct sgx_encl *encl;
> + int ret;
> +
> + encl = kzalloc(sizeof(*encl), GFP_KERNEL);
> + if (!encl)
> + return -ENOMEM;
> +
> + atomic_set(>flags, 0);
> + kref_init(>refcount);
> + xa_init(>page_array);
> + mutex_init(>lock);
> + INIT_LIST_HEAD(>mm_list);
> + spin_lock_init(>mm_lock);
> +
> + ret = init_srcu_struct(>srcu);
> + if (ret) {
> + kfree(encl);
> + return ret;
> + }
> +
> + file->private_data = encl;
> +
> + return 0;
> +}
> +
> +static int sgx_release(struct inode *inode, struct file *file)
> +{
> + struct sgx_encl *encl = file->private_data;
> + struct sgx_encl_mm *encl_mm;
> +
> + for ( ; ; ) {
> + spin_lock(>mm_lock);
> +
> + if (list_empty(>mm_list)) {
> + encl_mm = NULL;
> + } else {
> + encl_mm =
[PATCH v36 11/24] x86/sgx: Add SGX enclave driver
Intel Software Guard eXtensions (SGX) is a set of CPU instructions that can
be used by applications to set aside private regions of code and data. The
code outside the SGX hosted software entity is prevented from accessing the
memory inside the enclave by the CPU. We call these entities enclaves.
Add a driver that provides an ioctl API to construct and run enclaves.
Enclaves are constructed from pages residing in reserved physical memory
areas. The contents of these pages can only be accessed when they are
mapped as part of an enclave, by a hardware thread running inside the
enclave.
The starting state of an enclave consists of a fixed measured set of
pages that are copied to the EPC during the construction process by
using ENCLS leaf functions and Software Enclave Control Structure (SECS)
that defines the enclave properties.
Enclaves are constructed by using ENCLS leaf functions ECREATE, EADD and
EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
the EPC and EINIT checks a given signed measurement and moves the enclave
into a state ready for execution.
An initialized enclave can only be accessed through special Thread Control
Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER. This leaf
function converts a thread into enclave mode and continues the execution in
the offset defined by the TCS provided to EENTER. An enclave is exited
through syscall, exception, interrupts or by explicitly calling another
ENCLU leaf EEXIT.
The mmap() permissions are capped by the contained enclave page
permissions. The mapped areas must also be opaque, i.e. each page address
must contain a page. This logic is implemented in sgx_encl_may_map().
Cc: linux-security-mod...@vger.kernel.org
Cc: linux...@kvack.org
Cc: Andrew Morton
Cc: Matthew Wilcox
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
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
---
arch/x86/kernel/cpu/sgx/Makefile | 2 +
arch/x86/kernel/cpu/sgx/driver.c | 177
arch/x86/kernel/cpu/sgx/driver.h | 29 +++
arch/x86/kernel/cpu/sgx/encl.c | 333 +++
arch/x86/kernel/cpu/sgx/encl.h | 87
arch/x86/kernel/cpu/sgx/main.c | 11 +
6 files changed, 639 insertions(+)
create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
diff --git a/arch/x86/kernel/cpu/sgx/Makefile b/arch/x86/kernel/cpu/sgx/Makefile
index 79510ce01b3b..3fc451120735 100644
--- a/arch/x86/kernel/cpu/sgx/Makefile
+++ b/arch/x86/kernel/cpu/sgx/Makefile
@@ -1,2 +1,4 @@
obj-y += \
+ driver.o \
+ encl.o \
main.o
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
new file mode 100644
index ..b52520407f5b
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include "driver.h"
+#include "encl.h"
+
+MODULE_DESCRIPTION("Intel SGX Enclave Driver");
+MODULE_AUTHOR("Jarkko Sakkinen ");
+MODULE_LICENSE("Dual BSD/GPL");
+
+u64 sgx_encl_size_max_32;
+u64 sgx_encl_size_max_64;
+u32 sgx_misc_reserved_mask;
+u64 sgx_attributes_reserved_mask;
+u64 sgx_xfrm_reserved_mask = ~0x3;
+u32 sgx_xsave_size_tbl[64];
+
+static int sgx_open(struct inode *inode, struct file *file)
+{
+ struct sgx_encl *encl;
+ int ret;
+
+ encl = kzalloc(sizeof(*encl), GFP_KERNEL);
+ if (!encl)
+ return -ENOMEM;
+
+ atomic_set(>flags, 0);
+ kref_init(>refcount);
+ xa_init(>page_array);
+ mutex_init(>lock);
+ INIT_LIST_HEAD(>mm_list);
+ spin_lock_init(>mm_lock);
+
+ ret = init_srcu_struct(>srcu);
+ if (ret) {
+ kfree(encl);
+ return ret;
+ }
+
+ file->private_data = encl;
+
+ return 0;
+}
+
+static int sgx_release(struct inode *inode, struct file *file)
+{
+ struct sgx_encl *encl = file->private_data;
+ struct sgx_encl_mm *encl_mm;
+
+ for ( ; ; ) {
+ spin_lock(>mm_lock);
+
+ if (list_empty(>mm_list)) {
+ encl_mm = NULL;
+ } else {
+ encl_mm = list_first_entry(>mm_list,
+ struct sgx_encl_mm, list);
+ list_del_rcu(_mm->list);
+ }
+
+ spin_unlock(>mm_lock);
+
+ /* The list is empty, ready to go. */
+ if (!encl_mm)
+ break;
+
+
