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(&encl->flags); > > + > > ^ Superfluous newline. > > > + if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED)) > > + return ERR_PTR(-EFAULT); > > + > > + entry = xa_load(&encl->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(&encl_mm->encl->mm_lock); > > + list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) { > > + if (tmp == encl_mm) { > > + list_del_rcu(&encl_mm->list); > > + break; > > + } > > + } > > + spin_unlock(&encl_mm->encl->mm_lock); > > + > > + if (tmp == encl_mm) { > > + synchronize_srcu(&encl_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, &encl->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(&encl->flags); > + ^ Superfluous newline. > + if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED)) > + return ERR_PTR(-EFAULT); > + > + entry = xa_load(&encl->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(&encl_mm->encl->mm_lock); > + list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) { > + if (tmp == encl_mm) { > + list_del_rcu(&encl_mm->list); > + break; > + } > + } > + spin_unlock(&encl_mm->encl->mm_lock); > + > + if (tmp == encl_mm) { > + synchronize_srcu(&encl_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, &encl->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(&encl->flags, 0); > + kref_init(&encl->refcount); > + xa_init(&encl->page_array); > + mutex_init(&encl->lock); > + INIT_LIST_HEAD(&encl->mm_list); > + spin_lock_init(&encl->mm_lock); > + > + ret = init_srcu_struct(&encl->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(&encl->mm_lock); > + > + if (list_empty(&encl->mm_list)) { > + encl_mm = NULL; > +
[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(&encl->flags, 0); + kref_init(&encl->refcount); + xa_init(&encl->page_array); + mutex_init(&encl->lock); + INIT_LIST_HEAD(&encl->mm_list); + spin_lock_init(&encl->mm_lock); + + ret = init_srcu_struct(&encl->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(&encl->mm_lock); + + if (list_empty(&encl->mm_list)) { + encl_mm = NULL; + } else { + encl_mm = list_first_entry(&encl->mm_list, + struct sgx_encl_mm, list); + list_del_rcu(&encl_mm->list); + } + + spin_unlock(&encl->mm_lock); + + /* The list is empty, ready to go. */ +