Message ID | 20201104145430.300542-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
---|---|
Headers | show |
Series | Intel SGX foundations | expand |
On 2020-11-04 15:54, Jarkko Sakkinen wrote: *snip* > Jarkko Sakkinen (14): > x86/sgx: Add SGX architectural data structures > x86/sgx: Add wrappers for ENCLS functions > x86/cpu/intel: Add nosgx kernel parameter > x86/sgx: Add SGX page allocator functions > x86/sgx: Add SGX misc driver interface > x86/sgx: Add SGX_IOC_ENCLAVE_CREATE > x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES > x86/sgx: Add SGX_IOC_ENCLAVE_INIT > x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION > selftests/x86: Add a selftest for SGX > x86/sgx: Add a page reclaimer > x86/sgx: Add ptrace() support for the SGX driver > docs: x86/sgx: Document SGX kernel architecture > x86/sgx: Update MAINTAINERS > > Sean Christopherson (10): > x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections > x86/cpufeatures: x86/msr: Add Intel SGX hardware bits > x86/cpufeatures: x86/msr: Add Intel SGX Launch Control hardware bits > x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX > x86/cpu/intel: Detect SGX support > mm: Add 'mprotect' hook to struct vm_operations_struct > x86/vdso: Add support for exception fixup in vDSO functions > x86/fault: Add helper function to sanitize error code > x86/traps: Attempt to fixup exceptions in vDSO before signaling > x86/vdso: Implement a vDSO for Intel SGX enclave call I tested Jarkko's public git master branch at the time of writing (patch number, commit): 01 3dbc955 Acked-By: Jethro Beekman <jethro@fortanix.com> 02 0fb18ca Acked-By: Jethro Beekman <jethro@fortanix.com> 03 8f7ab60 Acked-By: Jethro Beekman <jethro@fortanix.com> 04 358d170 Acked-By: Jethro Beekman <jethro@fortanix.com> 05 0c64b4c Acked-By: Jethro Beekman <jethro@fortanix.com> 06 b0bacb5 Acked-By: Jethro Beekman <jethro@fortanix.com> 07 e131efe Acked-By: Jethro Beekman <jethro@fortanix.com> 08 5984a2c Acked-By: Jethro Beekman <jethro@fortanix.com> 09 93b27a8 Acked-By: Jethro Beekman <jethro@fortanix.com> 10 8ec6c36 Acked-By: Jethro Beekman <jethro@fortanix.com> 11 1e67355 Tested-By: Jethro Beekman <jethro@fortanix.com> 12 9f48d02 Tested-By: Jethro Beekman <jethro@fortanix.com> 13 53f7984 Tested-By: Jethro Beekman <jethro@fortanix.com> 14 5ab939b Tested-By: Jethro Beekman <jethro@fortanix.com> 15 6caa47ae Acked-By: Jethro Beekman <jethro@fortanix.com> 16 3106551 Acked-By: Jethro Beekman <jethro@fortanix.com> 17 7193709 Acked-By: Jethro Beekman <jethro@fortanix.com> 18 9c7d634 Acked-By: Jethro Beekman <jethro@fortanix.com> 19 cad6a3d Tested-By: Jethro Beekman <jethro@fortanix.com> 20 0dadc6b Acked-By: Jethro Beekman <jethro@fortanix.com> 21 e396b6f Acked-By: Jethro Beekman <jethro@fortanix.com> 22 bfcbc47 Tested-By: Jethro Beekman <jethro@fortanix.com> 23 7a0da40 Acked-By: Jethro Beekman <jethro@fortanix.com> 24 a644dc1 Acked-By: Jethro Beekman <jethro@fortanix.com> -- Jethro Beekman | Fortanix
On Sun, Nov 08, 2020 at 11:56:30AM +0800, Hillf Danton wrote: > On Wed, 4 Nov 2020 16:54:27 Jarkko Sakkinen wrote: > [...] > > +/** > > + * sgx_alloc_epc_page() - Allocate an EPC page > > + * @owner: the owner of the EPC page > > + * @reclaim: reclaim pages if necessary > > + * > > + * Iterate through EPC sections and borrow a free EPC page to the caller. When a > > + * page is no longer needed it must be released with sgx_free_epc_page(). If > > + * @reclaim is set to true, directly reclaim pages when we are out of pages. No > > + * mm's can be locked when @reclaim is set to true. > > + * > > + * Finally, wake up ksgxswapd when the number of pages goes below the watermark > > + * before returning back to the caller. > > + * > > + * Return: > > + * an EPC page, > > + * -errno on error > > + */ > > +struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > > +{ > > + struct sgx_epc_page *entry; > > Nit: s/entry/epc_page/ > > + > > + for ( ; ; ) { > > + entry = __sgx_alloc_epc_page(); > > + if (!IS_ERR(entry)) { > > + entry->owner = owner; > > + break; > > + } > > + > > + if (list_empty(&sgx_active_page_list)) > > + return ERR_PTR(-ENOMEM); > > + > > + if (!reclaim) { > > + entry = ERR_PTR(-EBUSY); > > + break; > > + } > > + > > + if (signal_pending(current)) { > > + entry = ERR_PTR(-ERESTARTSYS); > > + break; > > + } > > + > > + sgx_reclaim_pages(); > i > This is the direct reclaim mode with ksgxswapd that works in > the background ignored in the entire for loop. But we can go > with it in parallel, see below, if it tries as hard as it can > to maitain the watermark in which allocators may have no > interest. I think this policy should be left at is and once the code in mainline further refined. Consider it as a baseline/initial version for reclaiming code. > > + schedule(); > > To cut allocator's latency use cond_resched(); Thanks, I'll change this. > > + } > > + > > + if (sgx_should_reclaim(SGX_NR_LOW_PAGES)) > > + wake_up(&ksgxswapd_waitq); > > Nit: s/ksgxswapd/sgxd/ as it seems to have nothing to do with swap, > given sgx itself is clear and good enough. Yeah, it also handling kexec() situation, i.e. has multitude of functions. > > + > > + return entry; > > +} > > struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim) > { > struct sgx_epc_page *epc_page; > > for (;;) { > epc_page = __sgx_alloc_epc_page(); > > if (!IS_ERR(epc_page)) { > epc_page->owner = owner; > return epc_page; > } > > if (signal_pending(current)) > return ERR_PTR(-ERESTARTSYS); > > if (list_empty(&sgx_active_page_list) || !reclaim) > return ERR_PTR(-ENOMEM); > > wake_up(&ksgxswapd_waitq); > cond_resched(); > } > return ERR_PTR(-ENOMEM); > } /Jarkko
On Wed, Nov 04, 2020 at 04:54:06PM +0200, Jarkko Sakkinen wrote: Good morning, I hope the weekend is going well for everyone. > Important Kernel Touch Points > ============================= > > This implementation is picky and will decline to work on hardware which > is locked to Intel's root of trust. Given that this driver is no longer locked to the Intel trust root, by virtue of being restricted to run only on platforms which support Flexible Launch Control, there is no longer any legitimate technical reason to not expose all of the functionality of the hardware. The patch that I am including below implements signature based policy controls for enclave initialization. It does so in a manner that is completely transparent to the default behavior of the driver, which is to initialize any enclave passed to it with the exception of enclaves that set the PROVISION_KEY attribute bit. Secondary to the discussions that have been ongoing regarding the restriction of mmap/mprotect, this patch has been extended to implement signature based controls on dynamic enclaves. The default behavior of the driver under this patch is to reject mmap/mprotect on initialized enclaves, unless the platform owner has elected to allow the enclave signer the option to implement 'dynamic' enclaves, ie. enclaves that are allowed to modify their page permissions after initialization. There have been a number of 'GOOGLE-ites' copied on all of these discussions. The notion of signature based policy controls should be uncontroversial since it allows platform owners to implement a 'Zero-Trust' security model, a notion that has been widely advocated by GOOGLE and others in the industry. The strongest platform security guarantees for trusted execution environments, that offer strong confidentiality guarantees on both code and data, are ultimately only reputational. This patch enables SGX platform owners to avail themselves of that option while not restricting, in any way, what platform owners can do with their hardware. This patch is also available from the following location, given the vagaries of e-mail based patch transmission: ftp://ftp.enjellic.com/pub/sgx/kernel/SFLC-v41.patch Have a good remainder of the weekend. Dr. Greg --------------------------------------------------------------------------- Implement signature based policy controls. This patch implements the ability for the platform owner to implement signature based enclave control policies. It does so in a manner that is completely transparent to the normal behavior of the driver, which is to initialize any enclave that is presented to it with the exception of enclaves that have the PROVISION_KEY attribute set. If a launch enclave control policy is not implemented, any attempt to pass a launch token into the driver will cause enclave initialization to fail. Signature based policy control is based on the identity modulus signature of the enclave signer which is the SHA256 hash of the modulus of the enclave signing key. The following policy functionality is implemented. 1.) Control over which keys are allowed to sign enclaves. 2.) Control over which keys are allowed to implement launch enclaves. 3.) Control over which keys are allowed to sign enclaves that have access to the PROVISION_KEY attribute. 4.) Control over which enclaves are allowed to have their page permissions modified after enclave initialization. For each policy type a plurality of key signatures are allowed. Cryptographic initialization policy is accessed through the following four pseudo-files that are implemented by this patch: /sys/kernel/security/sgx/signing_keys /sys/kernel/security/sgx/launch_keys /sys/kernel/security/sgx/provisioning_keys /sys/kernel/security/sgx/dynamic_keys Policy keys are registered with the driver by writing the identity modulus signature to these files in simple hexadecimal format, ie: 0000000000000000000000000000000000000000000000000000000000000000 The current list of policy keys can be displayed by reading the contents of the pseudo-files. In addition to a key signature, the following keywords are accepted as valid entries for a policy file: clear lock The 'clear' keyword causes all existing entries in a policy list to be deleted. The 'lock' keyword causes any further modifications or access to a policy list to be denied. All of the policy code is implemented in a single file, policy.c, with minimal impact to the driver at large. Since the calculation of the identity modulus signature needed to program a launch control register is effectively a policy decision, the code to compute the signature was moved from the ioctl.c file to the policy.c file. Tested-by: Dr. Greg <greg@enjellic.com> Signed-off-by: Dr. Greg <greg@enjellic.com> --- arch/x86/Kconfig | 1 + arch/x86/include/uapi/asm/sgx.h | 1 + arch/x86/kernel/cpu/sgx/Makefile | 3 +- arch/x86/kernel/cpu/sgx/driver.c | 8 + arch/x86/kernel/cpu/sgx/driver.h | 2 + arch/x86/kernel/cpu/sgx/encl.c | 47 +-- arch/x86/kernel/cpu/sgx/encl.h | 3 + arch/x86/kernel/cpu/sgx/ioctl.c | 129 +++---- arch/x86/kernel/cpu/sgx/policy.c | 569 +++++++++++++++++++++++++++++++ 9 files changed, 664 insertions(+), 99 deletions(-) create mode 100644 arch/x86/kernel/cpu/sgx/policy.c diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 618d1aabccb8..575936aa4d91 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1937,6 +1937,7 @@ config X86_SGX depends on CRYPTO_SHA256=y select SRCU select MMU_NOTIFIER + select SECURITYFS help Intel(R) Software Guard eXtensions (SGX) is a set of CPU instructions that can be used by applications to set aside private regions of code diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 791e45334a4a..48316ffe00b3 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -63,6 +63,7 @@ struct sgx_enclave_add_pages { */ struct sgx_enclave_init { __u64 sigstruct; + __u64 token; }; /** diff --git a/arch/x86/kernel/cpu/sgx/Makefile b/arch/x86/kernel/cpu/sgx/Makefile index 91d3dc784a29..4576a491eb3f 100644 --- a/arch/x86/kernel/cpu/sgx/Makefile +++ b/arch/x86/kernel/cpu/sgx/Makefile @@ -2,4 +2,5 @@ obj-y += \ driver.o \ encl.o \ ioctl.o \ - main.o + main.o \ + policy.o diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c index f2eac41bb4ff..35ff0ca362dd 100644 --- a/arch/x86/kernel/cpu/sgx/driver.c +++ b/arch/x86/kernel/cpu/sgx/driver.c @@ -190,5 +190,13 @@ int __init sgx_drv_init(void) return ret; } + ret = sgx_policy_fs_init(); + if (ret) { + pr_err("SGX policy fs creation failed with %d.\n", ret); + misc_deregister(&sgx_dev_provision); + misc_deregister(&sgx_dev_enclave); + return ret; + } + return 0; } diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h index 4eddb4d571ef..6c0fac29e6ff 100644 --- a/arch/x86/kernel/cpu/sgx/driver.h +++ b/arch/x86/kernel/cpu/sgx/driver.h @@ -26,4 +26,6 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg); int sgx_drv_init(void); +int sgx_policy_fs_init(void); +u64 *sgx_policy_get_signer(u64 *signature); #endif /* __ARCH_X86_SGX_DRIVER_H__ */ diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 5551c7d36483..2731edc296b8 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -212,27 +212,22 @@ static void sgx_vma_open(struct vm_area_struct *vma) * @end: upper bound of the address range, exclusive * @vm_flags: VMA flags * - * Iterate through the enclave pages contained within [@start, @end) to verify - * that the permissions requested by a subset of {VM_READ, VM_WRITE, VM_EXEC} - * does not contain any permissions that are not contained in the build time - * permissions of any of the enclave pages within the given address range. + * This function provides a method for determining whether or not mmap + * or mprotect can be invoked on the virtual memory address range of + * an enclave. Page permission manipulations are only allowed on + * enclaves that have their dynamic flag set. * - * An enclave creator must declare the strongest permissions that will be - * needed for each enclave page This ensures that mappings have the identical - * or weaker permissions that the earlier declared permissions. + * The function signature is left intact since future versions of the + * driver may implement verifications that the requested permission + * changes are consistent with the desire of the enclave author. * * Return: 0 on success, -EACCES otherwise */ 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); - struct sgx_encl_page *page; - unsigned long count = 0; int ret = 0; - XA_STATE(xas, &encl->page_array, PFN_DOWN(start)); - /* * Disallow READ_IMPLIES_EXEC tasks as their VMA permissions might * conflict with the enclave page permissions. @@ -240,31 +235,9 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, if (current->personality & READ_IMPLIES_EXEC) return -EACCES; - mutex_lock(&encl->lock); - xas_lock(&xas); - xas_for_each(&xas, page, PFN_DOWN(end - 1)) { - if (!page) - break; - - if (~page->vm_max_prot_bits & vm_prot_bits) { - ret = -EACCES; - break; - } - - /* Reschedule on every XA_CHECK_SCHED iteration. */ - if (!(++count % XA_CHECK_SCHED)) { - xas_pause(&xas); - xas_unlock(&xas); - mutex_unlock(&encl->lock); - - cond_resched(); - - mutex_lock(&encl->lock); - xas_lock(&xas); - } - } - xas_unlock(&xas); - mutex_unlock(&encl->lock); + /* Disallow mapping on initialized enclave. */ + if (test_bit(SGX_ENCL_INITIALIZED, &encl->flags) && !encl->dynamic) + ret = -EACCES; return ret; } diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index 244e1d93fce2..cc4392929652 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -51,6 +51,7 @@ struct sgx_encl { unsigned long base; unsigned long size; unsigned long flags; + unsigned int dynamic; unsigned int page_cnt; unsigned int secs_child_cnt; struct mutex lock; @@ -104,4 +105,6 @@ unsigned int sgx_alloc_va_slot(struct sgx_va_page *va_page); void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset); bool sgx_va_page_full(struct sgx_va_page *va_page); +int sgx_policy_get_params(struct sgx_encl *encl, void *modulus, u64 *signer, + int *signcnt); #endif /* _X86_ENCL_H */ diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 6d37117ac8a0..4c92f4e1d507 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -466,70 +466,14 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) return ret; } -static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus, - void *hash) +static int sgx_encl_try_init(struct sgx_encl *encl, + struct sgx_sigstruct *sigstruct, void *token, + u64 *signer) { - SHASH_DESC_ON_STACK(shash, tfm); - - shash->tfm = tfm; - - return crypto_shash_digest(shash, modulus, SGX_MODULUS_SIZE, hash); -} - -static int sgx_get_key_hash(const void *modulus, void *hash) -{ - struct crypto_shash *tfm; - int ret; - - tfm = crypto_alloc_shash("sha256", 0, CRYPTO_ALG_ASYNC); - if (IS_ERR(tfm)) - return PTR_ERR(tfm); - - ret = __sgx_get_key_hash(tfm, modulus, hash); - - crypto_free_shash(tfm); - return ret; -} - -static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, - void *token) -{ - u64 mrsigner[4]; int i, j, k; void *addr; int ret; - /* - * Deny initializing enclaves with attributes (namely provisioning) - * that have not been explicitly allowed. - */ - if (encl->attributes & ~encl->attributes_mask) - return -EACCES; - - /* - * Attributes should not be enforced *only* against what's available on - * platform (done in sgx_encl_create) but checked and enforced against - * the mask for enforcement in sigstruct. For example an enclave could - * opt to sign with AVX bit in xfrm, but still be loadable on a platform - * without it if the sigstruct->body.attributes_mask does not turn that - * bit on. - */ - if (sigstruct->body.attributes & sigstruct->body.attributes_mask & - sgx_attributes_reserved_mask) - return -EINVAL; - - if (sigstruct->body.miscselect & sigstruct->body.misc_mask & - sgx_misc_reserved_mask) - return -EINVAL; - - if (sigstruct->body.xfrm & sigstruct->body.xfrm_mask & - sgx_xfrm_reserved_mask) - return -EINVAL; - - ret = sgx_get_key_hash(sigstruct->modulus, mrsigner); - if (ret) - return ret; - mutex_lock(&encl->lock); /* @@ -545,7 +489,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, preempt_disable(); for (k = 0; k < 4; k++) - wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + k, mrsigner[k]); + wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + k, signer[k]); ret = __einit(sigstruct, token, addr); @@ -585,6 +529,60 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, return ret; } +static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, + void *token) +{ + u64 mrsigner[4]; + u64 *signer; + int ret; + int signcnt = 1; + + /* Configure the launch policy. */ + ret = sgx_policy_get_params(encl, sigstruct->modulus, mrsigner, + &signcnt); + if (ret) + return ret; + + /* + * Deny initializing enclaves with attributes (namely provisioning) + * that have not been explicitly allowed. + */ + if (encl->attributes & ~encl->attributes_mask) + return -EACCES; + + /* + * Attributes should not be enforced *only* against what's available on + * platform (done in sgx_encl_create) but checked and enforced against + * the mask for enforcement in sigstruct. For example an enclave could + * opt to sign with AVX bit in xfrm, but still be loadable on a platform + * without it if the sigstruct->body.attributes_mask does not turn that + * bit on. + */ + if (sigstruct->body.attributes & sigstruct->body.attributes_mask & + sgx_attributes_reserved_mask) + return -EINVAL; + + if (sigstruct->body.miscselect & sigstruct->body.misc_mask & + sgx_misc_reserved_mask) + return -EINVAL; + + if (sigstruct->body.xfrm & sigstruct->body.xfrm_mask & + sgx_xfrm_reserved_mask) + return -EINVAL; + + /* Try the available policies. */ + signer = mrsigner; + while (signcnt--) { + ret = sgx_encl_try_init(encl, sigstruct, token, signer); + if (!ret) + return ret; + if (signcnt) + signer = sgx_policy_get_signer(signer); + } + + return ret; +} + /** * sgx_ioc_enclave_init() - handler for %SGX_IOC_ENCLAVE_INIT * @encl: an enclave pointer @@ -621,7 +619,16 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) sigstruct = kmap(initp_page); token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2); - memset(token, 0, SGX_LAUNCH_TOKEN_SIZE); + if (!init_arg.token) + memset(token, 0, SGX_LAUNCH_TOKEN_SIZE); + else { + if (copy_from_user((uint8_t *) token, + (void __user *) init_arg.token, + SGX_LAUNCH_TOKEN_SIZE)) { + ret = -EFAULT; + goto out; + } + } if (copy_from_user(sigstruct, (void __user *)init_arg.sigstruct, sizeof(*sigstruct))) { diff --git a/arch/x86/kernel/cpu/sgx/policy.c b/arch/x86/kernel/cpu/sgx/policy.c new file mode 100644 index 000000000000..e51440759192 --- /dev/null +++ b/arch/x86/kernel/cpu/sgx/policy.c @@ -0,0 +1,569 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) Enjellic Systems Development, LLC + +#define KEY_SIZE 32 + +#include <linux/types.h> +#include <linux/seq_file.h> +#include <linux/atomic.h> +#include <linux/security.h> +#include "driver.h" +#include "encl.h" + +static struct dentry *sgx_fs; + +struct list_key { + struct list_head list; + u64 key[KEY_SIZE / 8]; +}; + +struct list_key_iterator { + char *type; + atomic_t *opencount; + unsigned int *count; + struct mutex *lock; + struct list_head *list; + bool *lockfile; +}; + +static struct dentry *launch_keys; +static atomic_t launch_keys_opencount = ATOMIC_INIT(1); +static unsigned int launch_keys_count; +static bool launch_keys_locked; +static DEFINE_MUTEX(launch_key_list_mutex); +static LIST_HEAD(launch_key_list); + +static struct dentry *provision_keys; +static atomic_t provision_keys_opencount = ATOMIC_INIT(1); +static unsigned int provision_keys_count; +static bool provision_keys_locked; +static DEFINE_MUTEX(provision_key_list_mutex); +static LIST_HEAD(provision_key_list); + +static struct dentry *signing_keys; +static atomic_t signing_keys_opencount = ATOMIC_INIT(1); +static unsigned int signing_keys_count; +static bool signing_keys_locked; +static DEFINE_MUTEX(signing_key_list_mutex); +static LIST_HEAD(signing_key_list); + +static struct dentry *dynamic_keys; +static atomic_t dynamic_keys_opencount = ATOMIC_INIT(1); +static unsigned int dynamic_keys_count; +static bool dynamic_keys_locked; +static DEFINE_MUTEX(dynamic_key_list_mutex); +static LIST_HEAD(dynamic_key_list); + +/** + * have_signer - Verify the presence of a key signer. + * + * @signature: Pointer to signature of signer. + * + * Return: + * 0 Signer signature was not found. + * 1 Signer signature was found. + */ +static bool have_signer(struct list_head *keylist, struct mutex *lock, + uint8_t *signature) +{ + bool retn = false; + struct list_key *kp; + + mutex_lock(lock); + list_for_each_entry(kp, keylist, list) { + pr_debug("%s: Checking signer=%*phN, ks=%*phN\n", __func__, + KEY_SIZE, signature, KEY_SIZE, kp->key); + if (memcmp(kp->key, signature, KEY_SIZE) == 0) { + retn = true; + goto done; + } + } + + done: + mutex_unlock(lock); + return retn; +} + +static int process_write_key(const char __user *buf, size_t datalen, + unsigned int *keycnt, struct mutex *lock, + struct list_head *keylist) +{ + ssize_t retn; + + char *p, keybufr[KEY_SIZE*2 + 1], key[KEY_SIZE]; + + struct list_key *kp; + + if (datalen != sizeof(keybufr)) { + retn = -EINVAL; + goto done; + } + + memset(keybufr, '\0', sizeof(keybufr)); + if (copy_from_user(keybufr, buf, datalen)) { + retn = -EFAULT; + goto done; + } + + p = strchr(keybufr, '\n'); + if (!p) { + retn = -EINVAL; + goto done; + } + *p = '\0'; + if (hex2bin(key, keybufr, sizeof(key))) { + retn = -EINVAL; + goto done; + } + + kp = kzalloc(sizeof(*kp), GFP_KERNEL); + if (!kp) { + retn = -ENOMEM; + goto done; + } + memcpy(kp->key, key, sizeof(kp->key)); + + mutex_lock(lock); + list_add_tail(&kp->list, keylist); + ++*keycnt; + mutex_unlock(lock); + + retn = datalen; + pr_debug("%s: Added key: %*phN\n", __func__, KEY_SIZE, key); + + done: + return retn; +} + +static int process_lock(const char __user *buf, size_t datalen, bool *lockfile) +{ + char bufr[5]; + + if (datalen != strlen("lock") + 1) + return 0; + + memset(bufr, '\0', sizeof(bufr)); + if (copy_from_user(bufr, buf, datalen-1)) + return -EFAULT; + if (strcmp(bufr, "lock") != 0) + return 0; + + *lockfile = true; + return datalen; +} + +static int process_clear(const char __user *buf, size_t datalen, char *type, + unsigned int *keycnt, struct mutex *lock, + struct list_head *keylist) +{ + char bufr[6]; + struct list_key *kp, *kp_tmp; + + if (datalen != strlen("clear") + 1) + return 0; + + memset(bufr, '\0', sizeof(bufr)); + if (copy_from_user(bufr, buf, datalen-1)) + return -EFAULT; + if (strcmp(bufr, "clear") != 0) + return 0; + + mutex_lock(lock); + list_for_each_entry_safe(kp, kp_tmp, keylist, list) { + pr_debug("[%s]: Freeing signature: %*phN\n", __FILE__, + KEY_SIZE, kp->key); + list_del(&kp->list); + kfree(kp); + } + *keycnt = 0; + mutex_unlock(lock); + + pr_info("Cleared %s signatures.\n", type); + return datalen; +} + +static void *key_start(struct seq_file *c, loff_t *pos) +{ + struct list_key_iterator *ki = (struct list_key_iterator *) c->private; + + if (*pos >= *ki->count) + return NULL; + + mutex_lock(ki->lock); + return seq_list_start(ki->list, *pos); +} + +static void *key_next(struct seq_file *c, void *p, loff_t *pos) +{ + struct list_key_iterator *ki = (struct list_key_iterator *) c->private; + + return seq_list_next(p, ki->list, pos); +} + +static void key_stop(struct seq_file *c, void *p) +{ + struct list_key_iterator *ki = (struct list_key_iterator *) c->private; + + mutex_unlock(ki->lock); +} + +static int key_show(struct seq_file *c, void *key) +{ + struct list_key *kp; + + kp = list_entry(key, struct list_key, list); + seq_printf(c, "%*phN\n", KEY_SIZE, kp->key); + return 0; +} + +static const struct seq_operations keys_seqops = { + .start = key_start, + .next = key_next, + .stop = key_stop, + .show = key_show +}; + +static ssize_t write_keys(struct file *file, const char __user *buf, + size_t datalen, loff_t *ppos) +{ + struct seq_file *s = file->private_data; + struct list_key_iterator *ki = (struct list_key_iterator *) s->private; + ssize_t retn; + + if (*ppos != 0) + return -EINVAL; + + retn = process_lock(buf, datalen, ki->lockfile); + if (retn != 0) + return retn; + + retn = process_clear(buf, datalen, ki->type, ki->count, ki->lock, + ki->list); + if (retn != 0) + return retn; + + retn = process_write_key(buf, datalen, ki->count, ki->lock, ki->list); + return retn; +} + +static int release_keys(struct inode *inode, struct file *file) +{ + struct seq_file *s = file->private_data; + struct list_key_iterator *ki = (struct list_key_iterator *) s->private; + + atomic_set(ki->opencount, 1); + seq_release_private(inode, file); + return 0; +} + +static int open_launch_keys(struct inode *inode, struct file *filp) +{ + struct list_key_iterator *ki; + + if (launch_keys_locked) + return -EACCES; + + if (!atomic_dec_and_test(&launch_keys_opencount)) + return -EBUSY; + + ki = __seq_open_private(filp, &keys_seqops, sizeof(*ki)); + if (!ki) + return -ENOMEM; + + ki->type = "launch control"; + ki->opencount = &launch_keys_opencount; + ki->count = &launch_keys_count; + ki->lock = &launch_key_list_mutex; + ki->list = &launch_key_list; + ki->lockfile = &launch_keys_locked; + + return 0; +} + +static const struct file_operations launch_keys_ops = { + .open = open_launch_keys, + .write = write_keys, + .release = release_keys, + .read = seq_read, + .llseek = seq_lseek, +}; + +/* Provisioning control. */ + +static int open_provision_keys(struct inode *inode, struct file *filp) +{ + struct list_key_iterator *ki; + + if (provision_keys_locked) + return -EACCES; + + if (!atomic_dec_and_test(&provision_keys_opencount)) + return -EBUSY; + + ki = __seq_open_private(filp, &keys_seqops, sizeof(*ki)); + if (!ki) + return -ENOMEM; + + ki->type = "provisioning control"; + ki->opencount = &provision_keys_opencount; + ki->count = &provision_keys_count; + ki->lock = &provision_key_list_mutex; + ki->list = &provision_key_list; + + return 0; +} + +static const struct file_operations provision_keys_ops = { + .open = open_provision_keys, + .write = write_keys, + .release = release_keys, + .read = seq_read, + .llseek = seq_lseek, +}; + +/* Signing control. */ + +static int open_signing_keys(struct inode *inode, struct file *filp) +{ + struct list_key_iterator *ki; + + if (signing_keys_locked) + return -EACCES; + + if (!atomic_dec_and_test(&signing_keys_opencount)) + return -EBUSY; + + ki = __seq_open_private(filp, &keys_seqops, sizeof(*ki)); + if (!ki) + return -ENOMEM; + + ki->type = "signing control"; + ki->opencount = &signing_keys_opencount; + ki->count = &signing_keys_count; + ki->lock = &signing_key_list_mutex; + ki->list = &signing_key_list; + + return 0; +} + +static const struct file_operations signing_keys_ops = { + .open = open_signing_keys, + .write = write_keys, + .release = release_keys, + .read = seq_read, + .llseek = seq_lseek, +}; + +/* Dynamic memory control. */ + +static int open_dynamic_keys(struct inode *inode, struct file *filp) +{ + struct list_key_iterator *ki; + + if (dynamic_keys_locked) + return -EACCES; + + if (!atomic_dec_and_test(&dynamic_keys_opencount)) + return -EBUSY; + + ki = __seq_open_private(filp, &keys_seqops, sizeof(*ki)); + if (!ki) + return -ENOMEM; + + ki->type = "dynamic control"; + ki->opencount = &dynamic_keys_opencount; + ki->count = &dynamic_keys_count; + ki->lock = &dynamic_key_list_mutex; + ki->list = &dynamic_key_list; + ki->lockfile = &dynamic_keys_locked; + + return 0; +} + +static const struct file_operations dynamic_keys_ops = { + .open = open_dynamic_keys, + .write = write_keys, + .release = release_keys, + .read = seq_read, + .llseek = seq_lseek, +}; + +static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus, + void *hash) +{ + SHASH_DESC_ON_STACK(shash, tfm); + + shash->tfm = tfm; + + return crypto_shash_digest(shash, modulus, SGX_MODULUS_SIZE, hash); +} + +static int sgx_get_key_hash(const void *modulus, void *hash) +{ + struct crypto_shash *tfm; + int ret; + + tfm = crypto_alloc_shash("sha256", 0, CRYPTO_ALG_ASYNC); + if (IS_ERR(tfm)) + return PTR_ERR(tfm); + + ret = __sgx_get_key_hash(tfm, modulus, hash); + + crypto_free_shash(tfm); + return ret; +} + +/** + * sgx_policy_get_params + * + * This function sets the cryptographically configured initialization + * policy parameters. These include the identity modulus signature to + * be used as well as the configuration of the allowed enclave + * attributes. + * + * Return: + * 0 on success. + * -errno otherwise + */ + +int sgx_policy_get_params(struct sgx_encl *encl, void *modulus, u64 *signer, + int *signcnt) +{ + int retn = -EINVAL; + uint8_t mrsigner[KEY_SIZE]; + struct list_key *kp; + + retn = sgx_get_key_hash(modulus, mrsigner); + if (retn) + goto no_signer; + + if (provision_keys_count > 0 && + have_signer(&provision_key_list, &provision_key_list_mutex, + mrsigner)) + encl->attributes_mask |= SGX_ATTR_PROVISIONKEY; + + if (dynamic_keys_count > 0 && + have_signer(&dynamic_key_list, &dynamic_key_list_mutex, mrsigner)) + encl->dynamic = 1; + + if (signing_keys_count == 0 && launch_keys_count == 0) + goto have_signer; + + if (signing_keys_count > 0 && + have_signer(&signing_key_list, &signing_key_list_mutex, + mrsigner)) + goto have_signer; + + if (launch_keys_count == 0) + goto no_signer; + + if (encl->attributes & SGX_ATTR_EINITTOKENKEY) { + if (have_signer(&launch_key_list, &launch_key_list_mutex, + mrsigner)) { + encl->attributes_mask |= SGX_ATTR_EINITTOKENKEY; + goto have_signer; + } else + goto no_signer; + } + + *signcnt = launch_keys_count; + kp = list_first_entry(&launch_key_list, struct list_key, list); + memcpy(mrsigner, kp->key, KEY_SIZE); + + have_signer: + memcpy(signer, mrsigner, KEY_SIZE); + pr_debug("%s: Using signer: %*phN\n", __func__, KEY_SIZE, signer); + return 0; + no_signer: + memset(signer, '\0', KEY_SIZE); + return retn; +} + +/** + * sgx_policy_get_launch_signer - Iterate through list of enclave signers. + * + * @signer: The last returned enclave signer. + * + * This function iterates through the list of enclave signers from the + * last signature. Calling the function with a NULL value + * resets the iteration to the beginning of the list. + * + * Return: + * NULL indicates end of list + * non-NULL the next enclave signature on the list. + */ + +u64 *sgx_policy_get_signer(u64 *signer) +{ + bool seeking = false; + u64 *retn = NULL; + struct list_key *kp; + + if (!signer) { + kp = list_first_entry(&launch_key_list, struct list_key, list); + return kp->key; + } + kp = list_last_entry(&launch_key_list, struct list_key, list); + if (memcmp(kp->key, signer, sizeof(kp->key)) == 0) + return NULL; + + mutex_lock(&launch_key_list_mutex); + list_for_each_entry(kp, &launch_key_list, list) { + if (seeking) { + retn = kp->key; + goto done; + } + pr_debug("%s: Skipping: %*phN\n", __func__, KEY_SIZE, kp->key); + if (memcmp(kp->key, signer, KEY_SIZE) == 0) + seeking = true; + } + + done: + mutex_unlock(&launch_key_list_mutex); + return retn; +} + +int __init sgx_policy_fs_init(void) +{ + int retn = -1; + + sgx_fs = securityfs_create_dir("sgx", NULL); + if (IS_ERR(sgx_fs)) { + retn = PTR_ERR(sgx_fs); + goto err; + } + + launch_keys = securityfs_create_file("launch_keys", 0600, sgx_fs, + NULL, &launch_keys_ops); + if (IS_ERR(launch_keys)) { + retn = PTR_ERR(launch_keys); + goto err; + } + + provision_keys = securityfs_create_file("provisioning_keys", 0600, + sgx_fs, NULL, + &provision_keys_ops); + if (IS_ERR(provision_keys)) { + retn = PTR_ERR(provision_keys); + goto err; + } + + signing_keys = securityfs_create_file("signing_keys", 0600, sgx_fs, + NULL, &signing_keys_ops); + if (IS_ERR(signing_keys)) { + retn = PTR_ERR(signing_keys); + goto err; + } + + dynamic_keys = securityfs_create_file("dynamic_keys", 0600, sgx_fs, + NULL, &dynamic_keys_ops); + if (IS_ERR(dynamic_keys)) { + retn = PTR_ERR(dynamic_keys); + goto err; + } + + return 0; + + err: + return retn; +}
On 11/21/20 7:12 AM, Dr. Greg wrote: >> Important Kernel Touch Points >> ============================= >> >> This implementation is picky and will decline to work on hardware which >> is locked to Intel's root of trust. > Given that this driver is no longer locked to the Intel trust root, by > virtue of being restricted to run only on platforms which support > Flexible Launch Control, there is no longer any legitimate technical > reason to not expose all of the functionality of the hardware. I honestly can't understand what the point of this is, and I mean that on multiple levels. First of all, there's not a coherent description of the problem you're solving with ~700 lines of code and the treatise you wrote here instead of a changelog. Second, is the point here to distract folks from testing the branch in the tip tree? Or, is it to show appreciation to maintainers by giving them more of the thing they love: code to review?
Dr. Greg, I know you like sending these emails, but they're not really helpful for Linux kernel development. Please see below. On Sat, Nov 21, 2020 at 7:13 AM Dr. Greg <greg@enjellic.com> wrote: > > On Wed, Nov 04, 2020 at 04:54:06PM +0200, Jarkko Sakkinen wrote: > > Good morning, I hope the weekend is going well for everyone. > > > Important Kernel Touch Points > > ============================= > > > > This implementation is picky and will decline to work on hardware which > > is locked to Intel's root of trust. > > Given that this driver is no longer locked to the Intel trust root, by > virtue of being restricted to run only on platforms which support > Flexible Launch Control, there is no longer any legitimate technical > reason to not expose all of the functionality of the hardware. I read this three times, and I can't tell what functionality you're referring to. > > The patch that I am including below implements signature based policy > controls for enclave initialization. It does so in a manner that is > completely transparent to the default behavior of the driver, which is > to initialize any enclave passed to it with the exception of enclaves > that set the PROVISION_KEY attribute bit. It's completely unreviewable. It's an ABI patch, and it does not document what it does, nor does it document why it does it. It's just a bunch of code. NAK. To be crystal clear, I will not review, and I will NAK outright, any patches of this sort, until ALL of the following conditions are met: a) Either a functioning SGX driver lands in a -rc kernel or there is an excellent justification for why a change of this sort is needed prior to a release. Dr. Greg, you seem to be interested in SGX actually landing upstream, but these patches are just causing delays. Please stop. b) The patch needs to explain what problem it solves and why it is a good solution to that problem. c) The patch needs to explain, either in a changelog or in a text file in the patch, *exactly* what it does. Imagine MSDN-like documentation in the good old days. The documentation needs to be sufficiently clear that a userspace programmer could use your mechanism without reference to your implementation and that a kernel programmer could, in principle, re-implement your code from the description. Unless all three of these are met, your patch is going nowhere, and I think no one should waste their time trying to read it. > > Secondary to the discussions that have been ongoing regarding the > restriction of mmap/mprotect, this patch has been extended to > implement signature based controls on dynamic enclaves. The default > behavior of the driver under this patch is to reject mmap/mprotect on > initialized enclaves, unless the platform owner has elected to allow > the enclave signer the option to implement 'dynamic' enclaves, > ie. enclaves that are allowed to modify their page permissions after > initialization. You have sent this change repeatedly, and now for some reason you're sending it mixed in with unrelated changes. Please stop. At no point have you explained why this approach is better than anything else. It has the obnoxious side effect that you can't usefully SCM_RIGHTS an enclave to a different process with your patch applied, which is at least a minor disadvantage. You have not explained any advantage to your patch at all. Dr Greg, before you hit send on further emails about SGX, could you kindly try to imagine you're a kernel maintainer, read your own email, and consider whether how to make it add something useful to the discussion? Thanks, Andy
On Sat, Nov 21, 2020 at 08:25:23AM -0800, Dave Hansen wrote: Good morning, I hope the week has started well for everyone. > On 11/21/20 7:12 AM, Dr. Greg wrote: > >> Important Kernel Touch Points > >> ============================= > >> > >> This implementation is picky and will decline to work on hardware which > >> is locked to Intel's root of trust. > > Given that this driver is no longer locked to the Intel trust root, by > > virtue of being restricted to run only on platforms which support > > Flexible Launch Control, there is no longer any legitimate technical > > reason to not expose all of the functionality of the hardware. > I honestly can't understand what the point of this is, and I mean > that on multiple levels. I'm sorry the issue is elusive to you but that doesn't mean it isn't technically relevant or grounded. It also doesn't mean this issue isn't relevant to the kernel community at large. I have been active in Linux since late 1991 and my perception was that technical honesty was always the imperative, hence my last e-mail on this subject. > First of all, there's not a coherent description of the problem > you're solving with ~700 lines of code and the treatise you wrote > here instead of a changelog. A number of points. While I'm flattered, I cannot ethically accept the fact that the e-mail I wrote amounted to a treatise. To do so would do injustice to the likes of Euclid[0], Descartes[1] and Newton[2] among notable others. From a literary metric perspective it wouldn't rise to the level of a monograph let alone an essay on the subject. With that behind us. There was a full changelog with the patch, the e-mail essentially wrapped the changelog and patch with a cover letter that was directed at being responsive to the issue of including the SGX driver in the kernel. If you would have clicked on the link that I provided, which I will replicate below: ftp://ftp.enjellic.com/pub/sgx/kernel/SFLC-v41.patch You will get a fully 'git am' compliant patch, including a changelog. The changelog was written in a parlance consistent with someone who would have a basic understanding of the technology under review. If this entire review and vetting process is being done absent that kind of understanding, then the case can be made that the kernel development process has larger issues on its hands. Lets be honest though, that is not the case here, we have been talking about this issue for over a year, everyone involved with this technology knows what the problem is. Since LKML is copied, the basic issue is as follows: 1.) SGX as a technology is designed to execute code and operate on data in a manner that is confidential to inspection and impervious to modification and control by the kernel. 2.) The mindset of the driver developers is that the kernel should be the ultimate authority on what SGX is allowed to do. The two world views are inherently and technically incompatible and lead to a potential security dilemma for the kernel. We simply advocate for an additional level of cryptographic security that supplements, not replaces, kernel controls to address this issue. Issue #1 isn't theoretical. The Linux Foundation feels there is commercial value to this concept, as do the primary signatories (Intel, GOOGLE, Microsoft, IBM/RedHat, Alibaba, ARM, Huawei) to the Confidential Computing Consortium, all of which have a desire to economically exploit the notion of a generic Trusted Execution Environment such as SGX. So this is either a legitimate technical issue that needs to be addressed or these companies and their customers are on a fools errand. > Second, is the point here to distract folks from testing the branch > in the tip tree? Or, is it to show appreciation to maintainers by > giving them more of the thing they love: code to review? Overall, given the extremely small number of people that understand this technology end to end, let alone who can effectively test it, our involvement was driven by: Quis custodiet ipsos custodes? With respect to the maintainers, I'm sure it is a thankless job. However, if you are getting paid to review and maintain kernel code then one needs to review kernel code or find a different job if it is too thankless. I have plenty of thankless things to do in my job but I do them since it is my job. Our patch has two external functions of around 30 lines (~1 screen) each that impact the driver. The bulk of the 700 lines, all in one file, is boilerplate code, largely replicated for each instance, needed to read/write sysfs files and maintain four, nearly identical, linked lists. If this is an insurmountable review burden then the kernel development process has larger problems on its hands. Have a good day. Dr. Greg [0]: The Elements [1]: Discourse on the Method [2]: Opticks As always, Greg Wettstein, Ph.D, Worker Autonomously self-defensive Enjellic Systems Development, LLC IOT platforms and edge devices. 4206 N. 19th Ave. Fargo, ND 58102 PH: 701-281-1686 EMAIL: greg@enjellic.com ------------------------------------------------------------------------------ "I can only provide the information, I can't make you hear it." -- Shelley Bainter
On Tue, Nov 24, 2020 at 2:56 AM Dr. Greg <greg@enjellic.com> wrote: > > On Sat, Nov 21, 2020 at 08:25:23AM -0800, Dave Hansen wrote: > > You will get a fully 'git am' compliant patch, including a changelog. > > The changelog was written in a parlance consistent with someone who > would have a basic understanding of the technology under review. If > this entire review and vetting process is being done absent that kind > of understanding, then the case can be made that the kernel > development process has larger issues on its hands. No, it wasn't. I have a fairly good understanding of SGX, and I told you quite explicitly what was wrong with your changelog. Understanding the sentences you wrote and having the background is not at all the same thing as extracting meaning from your writing. Your patch conveyed no information. This email you just sent also conveys no information. > > Lets be honest though, that is not the case here, we have been talking > about this issue for over a year, everyone involved with this > technology knows what the problem is. > > Since LKML is copied, the basic issue is as follows: > > 1.) SGX as a technology is designed to execute code and operate on > data in a manner that is confidential to inspection and impervious to > modification and control by the kernel. > > 2.) The mindset of the driver developers is that the kernel should be > the ultimate authority on what SGX is allowed to do. > > The two world views are inherently and technically incompatible and > lead to a potential security dilemma for the kernel. We simply > advocate for an additional level of cryptographic security that > supplements, not replaces, kernel controls to address this issue. No, they are not. The kernel can and will enforce policy on what SGX may do. Your own NAKked patch, in fact, does exactly this. At the same time, SGX provides security to the contents of enclaves. These are not mutually exclusive. > Our patch has two external functions of around 30 lines (~1 screen) > each that impact the driver. The bulk of the 700 lines, all in one > file, is boilerplate code, largely replicated for each instance, > needed to read/write sysfs files and maintain four, nearly identical, > linked lists. If this is an insurmountable review burden then the > kernel development process has larger problems on its hands. Frankly, the largest problem in the kernel development process with regards to SGX is the distraction created by your emails. Please just stop. If you have something useful to say, distill it down to the smallest amount of text that actually says what you're trying to say. And don't forget the part about "something useful to say". If you do not have something useful to say, please don't say it.
On Sat, Nov 21, 2020 at 10:36:58AM -0800, Andy Lutomirski wrote: Good morning to everyone. > Dr. Greg, I know you like sending these emails, but they're not > really helpful for Linux kernel development. Please see below. I don't necessarily enjoy sending these e-mails and they take time away from a major initiative that I'm taking on, which is why I wanted to close all of this out. Since we are establishing the last correspondence on these issues, I just wanted to establish some final clarifications for everyone reading along at home. Andy, I've always publically recognized you as a gifted kernel developer, but for the record, have you a-priori architected or written either an SGX application stack or a trusted/untrusted runtime stack that exceeded 65 KLOC in size? I've done both, in the case of native SGX application stacks, multiple times. Including the first enclave<->enclave remote attestation and communications framework that bypassed the need for an Attestation Service Provider. Intel SGX developer licensing requires that you provide application recipients with a full and complete runtime along with the signed application. Our developer license allowed us to substitute our runtime for Intel's. I also spent 4+ years of back and forth with Washington, DC working to establish the importance of this technology for the DOD, DHS and various other groups that were interested in our national cybersecurity posture. Including demonstrations of the technology in Faraday shielded rooms. That doesn't make me an expert in kernel programming but it did make me feel obligated to ask what I believe are legitimate technical questions regarding the design and evolution of this driver. > On Sat, Nov 21, 2020 at 7:13 AM Dr. Greg <greg@enjellic.com> wrote: > > > > On Wed, Nov 04, 2020 at 04:54:06PM +0200, Jarkko Sakkinen wrote: > > > > Good morning, I hope the weekend is going well for everyone. > > > > > Important Kernel Touch Points > > > ============================= > > > > > > This implementation is picky and will decline to work on hardware which > > > is locked to Intel's root of trust. > > > > Given that this driver is no longer locked to the Intel trust root, by > > virtue of being restricted to run only on platforms which support > > Flexible Launch Control, there is no longer any legitimate technical > > reason to not expose all of the functionality of the hardware. > I read this three times, and I can't tell what functionality you're > referring to. Yes you do, as I mentioned to Dave in my last e-mail, we have been disagreeing about this for a year. You were at some kind of seminar where SGX was discussed. Based on that you developed a 'manifesto' regarding how Linux should implement the technology. That manifesto indicated there would be no place for cryptographic policy control on enclaves. A well taken and considered point on a locked launch control platform but completely irrelevant for this driver, that only operates on Flexible Launch Control platforms. By demanding compliance with only that vision you deny platform owners a final measure of defense against anonymous code execution. > > The patch that I am including below implements signature based policy > > controls for enclave initialization. It does so in a manner that is > > completely transparent to the default behavior of the driver, which is > > to initialize any enclave passed to it with the exception of enclaves > > that set the PROVISION_KEY attribute bit. > It's completely unreviewable. It's an ABI patch, and it does not > document what it does, nor does it document why it does it. It's > just a bunch of code. NAK. You can certainly NAK-away Andy[0]. I've had sufficent private feedback from reasoned and informed individuals that I've made my point that this isn't about technical considerations. Here is the link, again, to the patch in 'git am' compliant format. ftp://ftp.enjellic.com/pub/sgx/kernel/SFLC-v41.patch I've been watching Linux patches go by for close to 30 years. If this is completely unreviewable garbage, legitimate concerns can be raised about what is getting pushed into the kernel. > To be crystal clear, I will not review, and I will NAK outright, any > patches of this sort, until ALL of the following conditions are met: > > a) Either a functioning SGX driver lands in a -rc kernel or there is > an excellent justification for why a change of this sort is needed > prior to a release. Dr. Greg, you seem to be interested in SGX > actually landing upstream, but these patches are just causing delays. > Please stop. For the record I am stopping, I've said everything that I can say and the debate is not intellectually honest from a technical perspective, continuing forward would be a waste of my time and that of others. Everything is now publically documented for the decision makers. As to delaying the driver. I was told a year ago that any consideration of these security issues would hopelessly delay the driver. I was told six months ago, when I refreshed the patch against the new driver design, that the release of the driver was imminent and thus the issue couldn't be addressed. Three months ago I was admonished for my apparent attempt to delay the driver. It is now a year later and we are patching grammar and noun possessiveness issues in the driver, so I don't think it is intellectually honest to suggest I've delayed the driver, more on that below. > b) The patch needs to explain what problem it solves and why it is a > good solution to that problem. For the official record let me see if I can frame why I think we have not been intellectually honest with respect to this driver nor my suggested delay of it. 1.) SGX/TEE's are designed to be secure to an adversarial operating system or application space, ie. an IAGO threat model. Major players financing Linux development believe there is a commercial opportunity available in this capability. 2.) One year ago you indicated that the then current driver implementation was deficient, since it allowed executable code from anonymous memory, ie. executable code that was not surveilled by the LSM. 3.) At that time I pointed out that this makes little difference since the technology was perfectly capable of dynamically loading any executable code or data that it wanted to, using integrity and confidentiality protections that would prevent any inspection or control by the operating system, see point 1. 4.) This only leaves reputational or identity based security as a final and ultimate check as to who can execute code on a platform. Our patch provides that without requiring it. You and Dave have both indicated, in response to my concerns, that there is minimal threat to enclave based code. If that is the case we have needlessly delayed the driver for a year engineering a solution to what is a non-existent threat. > c) The patch needs to explain, either in a changelog or in a text > file in the patch, *exactly* what it does. Imagine MSDN-like > documentation in the good old days. The documentation needs to be > sufficiently clear that a userspace programmer could use your > mechanism without reference to your implementation and that a kernel > programmer could, in principle, re-implement your code from the > description. I will let the patch speak for itself, if I honestly believed that writing a monograph on the design would make any difference I would, but I think we can all agree that is not the case. For the record, this is infrastructure for a system administrator, a userspace programmer would have no involvement in it whatsoever. As I indicated in my mail to Dave, the changelog was designed for someone who has a working knowledge of SGX and basic kernel fundamentals such as linked lists and readable/writable /sysfs files. > Unless all three of these are met, your patch is going nowhere, and > I think no one should waste their time trying to read it. It is fine if it doesn't go anywhere, I've made my point, my intention in all of this was to provide what I believed to be an informed viewpoint on relevant technical issues. > > Secondary to the discussions that have been ongoing regarding the > > restriction of mmap/mprotect, this patch has been extended to > > implement signature based controls on dynamic enclaves. The default > > behavior of the driver under this patch is to reject mmap/mprotect on > > initialized enclaves, unless the platform owner has elected to allow > > the enclave signer the option to implement 'dynamic' enclaves, > > ie. enclaves that are allowed to modify their page permissions after > > initialization. > You have sent this change repeatedly, and now for some reason you're > sending it mixed in with unrelated changes. Please stop. At no > point have you explained why this approach is better than anything > else. In the discussion surrounding the mmap/mprotect based introspection of page permissions implemented by the SGX driver, you indicated that we should 'pretend' that some potential security model could be built on top of the EPCM page permission walking infrastructure. I believe we do significantly better than that. There are currently no controls over dynamic enclave behavior. Given that our patch implements a generic policy control framework we could implement this naturally and with little cost or complexity. For the LKML record, absent our patch the driver has an open security issue with respect to anonymous code execution that should be addressed, if that issue is indeed of any concern. In the vm_ops->mprotect thread, Haitao Huang elegantly described how this can be achieved, even without EDMM instructions, in a manner that would never allow the LSM to see WX permissions on a page. In fact the current page permission introspection infrastructure enables that behavior. > It has the obnoxious side effect that you can't usefully > SCM_RIGHTS an enclave to a different process with your patch > applied, which is at least a minor disadvantage. You have not > explained any advantage to your patch at all. I guess I don't understand the SCM_RIGHTS issue. The policy controls are implemented after the ioctl resolves the enclave control structure from the file descriptor and just prior to enclave initialization. It isn't clear how this would affect or inhibit a process that has possession of the file descriptor. The only thing that I can think of is that you disagree with the optional capability of blocking the enclave from implementing anonymously executable memory. Absent that ability there is the security issue that has now been extensively discussed. The current EPCM page permission walking code only enforces the wishes of the enclave signer not the platform owner. As I've noted multiple times, the SGX hardware itself is perfectly capable of enforcing those wishes. The security threat comes from the enclave conspiring with the untrusted component of the application, the current code only endorses what the enclave wants to do, not necessarily the wishes of the platform owner. Beyond that it is unclear where the issue of SCM_RIGHTS comes from. I know for certain our runtime doesn't use the concept nor does Intel's. The only reference that GOOGLE has is of you saying it would be nice to have if the system administrator wanted to enforce restrictions on what can be executed inside of an enclave and then give the enclave to another process. As has been discussed, this is largely irrelevant as the new process owner of the enclave handle (fd) can choose to load whatever code they are interested in without any inspection or controls. In fact, this is an often stated desire of the SGX user community in order to protect their algorithms in a cloud environment. > Dr Greg, before you hit send on further emails about SGX, could you > kindly try to imagine you're a kernel maintainer, read your own > email, and consider whether how to make it add something useful to > the discussion? You can breath a sign of relief as I won't be hitting send anymore. > Thanks, > Andy Have a good day. Dr. Greg [0]: If you have the facts on your side pound on the facts, if you have the law on your side pound on the law, if you have neither pound on the table. As always, Dr. Greg Wettstein, Ph.D, Worker Autonomously self-defensive Enjellic Systems Development, LLC IOT platforms and edge devices. 4206 N. 19th Ave. Fargo, ND 58102 PH: 701-281-1686 EMAIL: greg@enjellic.com ------------------------------------------------------------------------------ "There is no safe haven from irresponsible leadership." -- Lou Dobbs
On Tue, Nov 24, 2020 at 10:40 AM Dr. Greg <greg@enjellic.com> wrote: > > On Sat, Nov 21, 2020 at 10:36:58AM -0800, Andy Lutomirski wrote: > > Intel SGX developer licensing requires that you provide application > recipients with a full and complete runtime along with the signed > application. Our developer license allowed us to substitute our > runtime for Intel's. Upstream Linux wants nothing to do with Intel's developer licensing. > > > Given that this driver is no longer locked to the Intel trust root, by > > > virtue of being restricted to run only on platforms which support > > > Flexible Launch Control, there is no longer any legitimate technical > > > reason to not expose all of the functionality of the hardware. > > > I read this three times, and I can't tell what functionality you're > > referring to. > > Yes you do, as I mentioned to Dave in my last e-mail, we have been > disagreeing about this for a year. No, I don't. It's entirely possible that I'm aware of the functionality you're referring to, but that doesn't mean that your description quoted above is sufficient for me to have the slightest idea which functionality you mean right here. > > You were at some kind of seminar where SGX was discussed. Based on > that you developed a 'manifesto' regarding how Linux should implement > the technology. That manifesto indicated there would be no place for > cryptographic policy control on enclaves. That's not what I said. Paraphrasing myself, I said that there is no place for a driver in upstream Linux that allows only Intel-approved code to run. That is not at all the same thing as saying we won't support cryptographic policy in a way that allows the platform owner an appropriate degree of control. Actually supporting launch control (the EINIT-enforced kind) in upstream Linux will be tricky but is surely possible. It would probably help if Intel or firmware vendors had some clear specification of exactly how they intend for platform owners to select an SGXLEPUBKEYHASH value. (A nice spec in which an authenticated UEFI variable contained the desired SGXLEPUBKEYHASH and lock state might be an excellent start.) Supporting this in upstream Linux will also require decoupling the user code that creates an enclave from the user code that invokes the LE. Jarkko already wrote some code for this, and it could be revived. If this ends up being inconsistent with Intel's licensing requirements, then Intel can change their licensing requirements or people can just ignore Intel and use a different signing key. Frankly, using Intel's signing key in SGXLEPUBKEYHASH offers a dubious degree of protection in the first place -- it assumes that Intel will never approve a malware enclave, and it also assumes that the lack of functioning EINITTOKEN revocation won't break the whole scheme. > > A well taken and considered point on a locked launch control platform > but completely irrelevant for this driver, that only operates on > Flexible Launch Control platforms. By demanding compliance with only > that vision you deny platform owners a final measure of defense > against anonymous code execution. I am denying Intel the chance to impose their licensing requirements. The fact that Intel chose to poison the well with their licensing system and that, as a result, Linux won't support the Intel model of launch control to protect platform owners is an unfortunate side effect. > > > > The patch that I am including below implements signature based policy > > > controls for enclave initialization. It does so in a manner that is > > > completely transparent to the default behavior of the driver, which is > > > to initialize any enclave passed to it with the exception of enclaves > > > that set the PROVISION_KEY attribute bit. > > > It's completely unreviewable. It's an ABI patch, and it does not > > document what it does, nor does it document why it does it. It's > > just a bunch of code. NAK. > > You can certainly NAK-away Andy[0]. I've had sufficent private > feedback from reasoned and informed individuals that I've made my > point that this isn't about technical considerations. Depends on what you mean by "technical". In the submitting-patches guide (https://www.kernel.org/doc/html/v5.9/process/submitting-patches.html), you will find instructions to "describe your changes" and "separate your changes". If you can produce a reasonably, reviewable submission of your code, I will review it. But I am not going to dig through your diff to try to find the hidden technical merit. > > Here is the link, again, to the patch in 'git am' compliant format. > > ftp://ftp.enjellic.com/pub/sgx/kernel/SFLC-v41.patch > > I've been watching Linux patches go by for close to 30 years. If this > is completely unreviewable garbage, legitimate concerns can be raised > about what is getting pushed into the kernel. The kernel review process is by no means perfect. That does not mean that you get to apply your unreviewable garbage just because other people have pulled it off in the past. > For the LKML record, absent our patch the driver has an open security > issue with respect to anonymous code execution that should be > addressed, if that issue is indeed of any concern. What do you mean? > The only thing that I can think of is that you disagree with the > optional capability of blocking the enclave from implementing > anonymously executable memory. Absent that ability there is the > security issue that has now been extensively discussed. You keep sending a patch that blocks mmap and mprotect on an initialized enclave. As far as I can tell, you haven't explained how it's any better than the code it replaces. The code it replaces enforces per-page maximum permissions, and all the infrastructure is in place for the platform owner to be able to enforce their rules without breaking ABI. Your proposal appears to accomplish something a little bit like what the code in -tip does, except without as much room for future improvements. Since you haven't tried to explain why you think it's better, I can't really evaluate it.