* [PATCH v25 10/21] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-02-04 6:05 UTC (permalink / raw)
To: linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp, josh, luto,
kai.huang, rientjes, cedric.xing, puiterwijk, Jarkko Sakkinen,
linux-security-module, Suresh Siddha, Jarkko Sakkinen
In-Reply-To: <20200204060545.31729-1-jarkko.sakkinen@linux.intel.com>
From: Jarkko Sakkinen <jarkko.sakkinen@intel.com>
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 disallowed to
access the memory inside the enclave enforced by the CPU. We call these
entities as enclaves.
This commit implements 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.
Enclave 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 check 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 permissions, which enclave page is added will set the limit for maximum
permissions that can be set for mmap() and mprotect(). This will
effectively allow to build different security schemes between producers and
consumers of enclaves. Later on we can increase granularity with LSM hooks
for page addition (i.e. for producers) and mapping of the enclave (i.e. for
consumers)
Cc: linux-security-module@vger.kernel.org
Cc: Nathaniel McCallum <npmccallum@redhat.com>
Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Co-developed-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
arch/x86/include/uapi/asm/sgx.h | 66 ++
arch/x86/kernel/cpu/sgx/Makefile | 3 +
arch/x86/kernel/cpu/sgx/driver.c | 194 +++++
arch/x86/kernel/cpu/sgx/driver.h | 30 +
arch/x86/kernel/cpu/sgx/encl.c | 329 +++++++++
arch/x86/kernel/cpu/sgx/encl.h | 87 +++
arch/x86/kernel/cpu/sgx/ioctl.c | 697 ++++++++++++++++++
arch/x86/kernel/cpu/sgx/main.c | 12 +-
arch/x86/kernel/cpu/sgx/reclaim.c | 1 +
10 files changed, 1419 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/include/uapi/asm/sgx.h
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
create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 2e91370dc159..1c54dd2704db 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -321,6 +321,7 @@ Code Seq# Include File Comments
<mailto:tlewis@mindspring.com>
0xA3 90-9F linux/dtlk.h
0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
+0xA4 00-1F uapi/asm/sgx.h Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
0xAA 00-3F linux/uapi/linux/userfaultfd.h
0xAB 00-1F linux/nbd.h
0xAC 00-1F linux/raw.h
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
new file mode 100644
index 000000000000..5edb08ab8fd0
--- /dev/null
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
+/*
+ * Copyright(c) 2016-19 Intel Corporation.
+ */
+#ifndef _UAPI_ASM_X86_SGX_H
+#define _UAPI_ASM_X86_SGX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/**
+ * enum sgx_epage_flags - page control flags
+ * %SGX_PAGE_MEASURE: Measure the page contents with a sequence of
+ * ENCLS[EEXTEND] operations.
+ */
+enum sgx_page_flags {
+ SGX_PAGE_MEASURE = 0x01,
+};
+
+#define SGX_MAGIC 0xA4
+
+#define SGX_IOC_ENCLAVE_CREATE \
+ _IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
+#define SGX_IOC_ENCLAVE_ADD_PAGES \
+ _IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
+#define SGX_IOC_ENCLAVE_INIT \
+ _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
+
+/**
+ * struct sgx_enclave_create - parameter structure for the
+ * %SGX_IOC_ENCLAVE_CREATE ioctl
+ * @src: address for the SECS page data
+ */
+struct sgx_enclave_create {
+ __u64 src;
+};
+
+/**
+ * struct sgx_enclave_add_pages - parameter structure for the
+ * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
+ * @src: start address for the page data
+ * @offset: starting page offset
+ * @length: length of the data (multiple of the page size)
+ * @secinfo: address for the SECINFO data
+ * @flags: page control flags
+ * @count: number of bytes added (multiple of the page size)
+ */
+struct sgx_enclave_add_pages {
+ __u64 src;
+ __u64 offset;
+ __u64 length;
+ __u64 secinfo;
+ __u64 flags;
+ __u64 count;
+};
+
+/**
+ * struct sgx_enclave_init - parameter structure for the
+ * %SGX_IOC_ENCLAVE_INIT ioctl
+ * @sigstruct: address for the SIGSTRUCT data
+ */
+struct sgx_enclave_init {
+ __u64 sigstruct;
+};
+
+#endif /* _UAPI_ASM_X86_SGX_H */
diff --git a/arch/x86/kernel/cpu/sgx/Makefile b/arch/x86/kernel/cpu/sgx/Makefile
index 2dec75916a5e..f8d32da3a67a 100644
--- a/arch/x86/kernel/cpu/sgx/Makefile
+++ b/arch/x86/kernel/cpu/sgx/Makefile
@@ -1,3 +1,6 @@
obj-y += \
+ driver.o \
+ encl.o \
+ ioctl.o \
main.o \
reclaim.o
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
new file mode 100644
index 000000000000..b4aa7b9f8376
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#include <linux/acpi.h>
+#include <linux/miscdevice.h>
+#include <linux/mman.h>
+#include <linux/security.h>
+#include <linux/suspend.h>
+#include <asm/traps.h>
+#include "driver.h"
+#include "encl.h"
+
+MODULE_DESCRIPTION("Intel SGX Enclave Driver");
+MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>");
+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);
+ INIT_RADIX_TREE(&encl->page_tree, GFP_KERNEL);
+ 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. */
+ if (!encl_mm)
+ break;
+
+ synchronize_srcu(&encl->srcu);
+ mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
+ kfree(encl_mm);
+ };
+
+ mutex_lock(&encl->lock);
+ atomic_or(SGX_ENCL_DEAD, &encl->flags);
+ mutex_unlock(&encl->lock);
+
+ kref_put(&encl->refcount, sgx_encl_release);
+ return 0;
+}
+
+#ifdef CONFIG_COMPAT
+static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ return sgx_ioctl(filep, cmd, arg);
+}
+#endif
+
+static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
+{
+ struct sgx_encl *encl = file->private_data;
+ int ret;
+
+ ret = sgx_encl_may_map(encl, vma->vm_start, vma->vm_end,
+ vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC));
+ if (ret)
+ return ret;
+
+ ret = sgx_encl_mm_add(encl, vma->vm_mm);
+ if (ret)
+ return ret;
+
+ vma->vm_ops = &sgx_vm_ops;
+ vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
+ vma->vm_private_data = encl;
+
+ return 0;
+}
+
+static unsigned long sgx_get_unmapped_area(struct file *file,
+ unsigned long addr,
+ unsigned long len,
+ unsigned long pgoff,
+ unsigned long flags)
+{
+ if (flags & MAP_PRIVATE)
+ return -EINVAL;
+
+ if (flags & MAP_FIXED)
+ return addr;
+
+ return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
+}
+
+static const struct file_operations sgx_encl_fops = {
+ .owner = THIS_MODULE,
+ .open = sgx_open,
+ .release = sgx_release,
+ .unlocked_ioctl = sgx_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = sgx_compat_ioctl,
+#endif
+ .mmap = sgx_mmap,
+ .get_unmapped_area = sgx_get_unmapped_area,
+};
+
+const struct file_operations sgx_provision_fops = {
+ .owner = THIS_MODULE,
+};
+
+static struct miscdevice sgx_dev_enclave = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "enclave",
+ .nodename = "sgx/enclave",
+ .fops = &sgx_encl_fops,
+};
+
+int __init sgx_drv_init(void)
+{
+ unsigned int eax, ebx, ecx, edx;
+ u64 attr_mask, xfrm_mask;
+ int ret;
+ int i;
+
+ if (!boot_cpu_has(X86_FEATURE_SGX_LC)) {
+ pr_info("The public key MSRs are not writable.\n");
+ return -ENODEV;
+ }
+
+ cpuid_count(SGX_CPUID, 0, &eax, &ebx, &ecx, &edx);
+ sgx_misc_reserved_mask = ~ebx | SGX_MISC_RESERVED_MASK;
+ sgx_encl_size_max_64 = 1ULL << ((edx >> 8) & 0xFF);
+ sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
+
+ cpuid_count(SGX_CPUID, 1, &eax, &ebx, &ecx, &edx);
+
+ attr_mask = (((u64)ebx) << 32) + (u64)eax;
+ sgx_attributes_reserved_mask = ~attr_mask | SGX_ATTR_RESERVED_MASK;
+
+ if (boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+ xfrm_mask = (((u64)edx) << 32) + (u64)ecx;
+
+ for (i = 2; i < 64; i++) {
+ cpuid_count(0x0D, i, &eax, &ebx, &ecx, &edx);
+ if ((1 << i) & xfrm_mask)
+ sgx_xsave_size_tbl[i] = eax + ebx;
+ }
+
+ sgx_xfrm_reserved_mask = ~xfrm_mask;
+ }
+
+ ret = misc_register(&sgx_dev_enclave);
+ if (ret) {
+ pr_err("Creating /dev/sgx/enclave failed with %d.\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
new file mode 100644
index 000000000000..e4063923115b
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/driver.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+#ifndef __ARCH_SGX_DRIVER_H__
+#define __ARCH_SGX_DRIVER_H__
+
+#include <crypto/hash.h>
+#include <linux/kref.h>
+#include <linux/mmu_notifier.h>
+#include <linux/radix-tree.h>
+#include <linux/rwsem.h>
+#include <linux/sched.h>
+#include <linux/workqueue.h>
+#include <uapi/asm/sgx.h>
+#include "sgx.h"
+
+#define SGX_EINIT_SPIN_COUNT 20
+#define SGX_EINIT_SLEEP_COUNT 50
+#define SGX_EINIT_SLEEP_TIME 20
+
+extern u64 sgx_encl_size_max_32;
+extern u64 sgx_encl_size_max_64;
+extern u32 sgx_misc_reserved_mask;
+extern u64 sgx_attributes_reserved_mask;
+extern u64 sgx_xfrm_reserved_mask;
+extern u32 sgx_xsave_size_tbl[64];
+
+long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
+
+int sgx_drv_init(void);
+
+#endif /* __ARCH_X86_SGX_DRIVER_H__ */
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
new file mode 100644
index 000000000000..cd2b8dbb0eca
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -0,0 +1,329 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-18 Intel Corporation.
+
+#include <linux/lockdep.h>
+#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/shmem_fs.h>
+#include <linux/suspend.h>
+#include <linux/sched/mm.h>
+#include "arch.h"
+#include "encl.h"
+#include "sgx.h"
+
+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);
+
+ if ((flags & SGX_ENCL_DEAD) || !(flags & SGX_ENCL_INITIALIZED))
+ return ERR_PTR(-EFAULT);
+
+ entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
+ 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);
+ 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);
+
+ kfree(encl_mm);
+}
+
+static const struct mmu_notifier_ops sgx_mmu_notifier_ops = {
+ .release = sgx_mmu_notifier_release,
+ .free_notifier = sgx_mmu_notifier_free,
+};
+
+static struct sgx_encl_mm *sgx_encl_find_mm(struct sgx_encl *encl,
+ struct mm_struct *mm)
+{
+ struct sgx_encl_mm *encl_mm = NULL;
+ struct sgx_encl_mm *tmp;
+ int idx;
+
+ idx = srcu_read_lock(&encl->srcu);
+
+ list_for_each_entry_rcu(tmp, &encl->mm_list, list) {
+ if (tmp->mm == mm) {
+ encl_mm = tmp;
+ break;
+ }
+ }
+
+ srcu_read_unlock(&encl->srcu, idx);
+
+ return encl_mm;
+}
+
+int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
+{
+ struct sgx_encl_mm *encl_mm;
+ int ret;
+
+ if (atomic_read(&encl->flags) & SGX_ENCL_DEAD)
+ return -EINVAL;
+
+ /*
+ * mm_structs are kept on mm_list until the mm or the enclave dies,
+ * i.e. once an mm is off the list, it's gone for good, therefore it's
+ * impossible to get a false positive on @mm due to a stale mm_list.
+ */
+ if (sgx_encl_find_mm(encl, mm))
+ return 0;
+
+ encl_mm = kzalloc(sizeof(*encl_mm), GFP_KERNEL);
+ if (!encl_mm)
+ return -ENOMEM;
+
+ encl_mm->encl = encl;
+ encl_mm->mm = mm;
+ encl_mm->mmu_notifier.ops = &sgx_mmu_notifier_ops;
+
+ ret = __mmu_notifier_register(&encl_mm->mmu_notifier, mm);
+ if (ret) {
+ kfree(encl_mm);
+ return ret;
+ }
+
+ spin_lock(&encl->mm_lock);
+ list_add_rcu(&encl_mm->list, &encl->mm_list);
+ spin_unlock(&encl->mm_lock);
+
+ synchronize_srcu(&encl->srcu);
+
+ return 0;
+}
+
+static void sgx_vma_open(struct vm_area_struct *vma)
+{
+ struct sgx_encl *encl = vma->vm_private_data;
+
+ if (!encl)
+ return;
+
+ if (sgx_encl_mm_add(encl, vma->vm_mm))
+ vma->vm_private_data = NULL;
+}
+
+static unsigned int sgx_vma_fault(struct vm_fault *vmf)
+{
+ unsigned long addr = (unsigned long)vmf->address;
+ struct vm_area_struct *vma = vmf->vma;
+ struct sgx_encl *encl = vma->vm_private_data;
+ struct sgx_encl_page *entry;
+ int ret = VM_FAULT_NOPAGE;
+ unsigned long pfn;
+
+ if (!encl)
+ return VM_FAULT_SIGBUS;
+
+ mutex_lock(&encl->lock);
+
+ entry = sgx_encl_load_page(encl, addr);
+ if (IS_ERR(entry)) {
+ if (unlikely(PTR_ERR(entry) != -EBUSY))
+ ret = VM_FAULT_SIGBUS;
+
+ goto out;
+ }
+
+ if (!follow_pfn(vma, addr, &pfn))
+ goto out;
+
+ ret = vmf_insert_pfn(vma, addr, PFN_DOWN(entry->epc_page->desc));
+ if (ret != VM_FAULT_NOPAGE) {
+ ret = VM_FAULT_SIGBUS;
+ goto out;
+ }
+
+out:
+ mutex_unlock(&encl->lock);
+ return ret;
+}
+
+/**
+ * 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. Page addresses that do not have an associated enclave
+ * page are interpreted to zero permissions.
+ *
+ * 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_prot_bits)
+{
+ unsigned long idx, idx_start, idx_end;
+ struct sgx_encl_page *page;
+
+ /* PROT_NONE always succeeds. */
+ if (!vm_prot_bits)
+ return 0;
+
+ idx_start = PFN_DOWN(start);
+ idx_end = PFN_DOWN(end - 1);
+
+ for (idx = idx_start; idx <= idx_end; ++idx) {
+ mutex_lock(&encl->lock);
+ page = radix_tree_lookup(&encl->page_tree, idx);
+ mutex_unlock(&encl->lock);
+
+ if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
+ return -EACCES;
+ }
+
+ return 0;
+}
+
+static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
+ unsigned long end, unsigned long prot)
+{
+ return sgx_encl_may_map(vma->vm_private_data, start, end,
+ calc_vm_prot_bits(prot, 0));
+}
+
+const struct vm_operations_struct sgx_vm_ops = {
+ .open = sgx_vma_open,
+ .fault = sgx_vma_fault,
+ .may_mprotect = sgx_vma_mprotect,
+};
+
+/**
+ * sgx_encl_find - find an enclave
+ * @mm: mm struct of the current process
+ * @addr: address in the ELRANGE
+ * @vma: the resulting VMA
+ *
+ * Find an enclave identified by the given address. Give back a VMA that is
+ * part of the enclave and located in that address. The VMA is given back if it
+ * is a proper enclave VMA even if an &sgx_encl instance does not exist yet
+ * (enclave creation has not been performed).
+ *
+ * Return:
+ * 0 on success,
+ * -EINVAL if an enclave was not found,
+ * -ENOENT if the enclave has not been created yet
+ */
+int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
+ struct vm_area_struct **vma)
+{
+ struct vm_area_struct *result;
+ struct sgx_encl *encl;
+
+ result = find_vma(mm, addr);
+ if (!result || result->vm_ops != &sgx_vm_ops || addr < result->vm_start)
+ return -EINVAL;
+
+ encl = result->vm_private_data;
+ *vma = result;
+
+ return encl ? 0 : -ENOENT;
+}
+
+/**
+ * sgx_encl_destroy() - destroy enclave resources
+ * @encl: an &sgx_encl instance
+ */
+void sgx_encl_destroy(struct sgx_encl *encl)
+{
+ struct sgx_encl_page *entry;
+ struct radix_tree_iter iter;
+ void **slot;
+
+ atomic_or(SGX_ENCL_DEAD, &encl->flags);
+
+ radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
+ entry = *slot;
+
+ if (entry->epc_page) {
+ sgx_free_page(entry->epc_page);
+ encl->secs_child_cnt--;
+ entry->epc_page = NULL;
+ }
+
+ radix_tree_delete(&entry->encl->page_tree,
+ PFN_DOWN(entry->desc));
+ kfree(entry);
+ }
+
+ if (!encl->secs_child_cnt && encl->secs.epc_page) {
+ sgx_free_page(encl->secs.epc_page);
+ encl->secs.epc_page = NULL;
+ }
+}
+
+/**
+ * sgx_encl_release - Destroy an enclave instance
+ * @kref: address of a kref inside &sgx_encl
+ *
+ * Used together with kref_put(). Frees all the resources associated with the
+ * enclave and the instance itself.
+ */
+void sgx_encl_release(struct kref *ref)
+{
+ struct sgx_encl *encl = container_of(ref, struct sgx_encl, refcount);
+
+ sgx_encl_destroy(encl);
+
+ if (encl->backing)
+ fput(encl->backing);
+
+ WARN_ON_ONCE(!list_empty(&encl->mm_list));
+
+ /* Detect EPC page leak's. */
+ WARN_ON_ONCE(encl->secs_child_cnt);
+ WARN_ON_ONCE(encl->secs.epc_page);
+
+ kfree(encl);
+}
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
new file mode 100644
index 000000000000..1d1bc5d590ee
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/**
+ * Copyright(c) 2016-19 Intel Corporation.
+ */
+#ifndef _X86_ENCL_H
+#define _X86_ENCL_H
+
+#include <linux/cpumask.h>
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/mm_types.h>
+#include <linux/mmu_notifier.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/radix-tree.h>
+#include <linux/srcu.h>
+#include <linux/workqueue.h>
+#include "sgx.h"
+
+/**
+ * enum sgx_encl_page_desc - defines bits for an enclave page's descriptor
+ * %SGX_ENCL_PAGE_ADDR_MASK: Holds the virtual address of the page.
+ *
+ * The page address for SECS is zero and is used by the subsystem to recognize
+ * the SECS page.
+ */
+enum sgx_encl_page_desc {
+ /* Bits 11:3 are available when the page is not swapped. */
+ SGX_ENCL_PAGE_ADDR_MASK = PAGE_MASK,
+};
+
+#define SGX_ENCL_PAGE_ADDR(page) \
+ ((page)->desc & SGX_ENCL_PAGE_ADDR_MASK)
+
+struct sgx_encl_page {
+ unsigned long desc;
+ unsigned long vm_max_prot_bits;
+ struct sgx_epc_page *epc_page;
+ struct sgx_encl *encl;
+};
+
+enum sgx_encl_flags {
+ SGX_ENCL_CREATED = BIT(0),
+ SGX_ENCL_INITIALIZED = BIT(1),
+ SGX_ENCL_DEBUG = BIT(2),
+ SGX_ENCL_DEAD = BIT(3),
+ SGX_ENCL_IOCTL = BIT(4),
+};
+
+struct sgx_encl_mm {
+ struct sgx_encl *encl;
+ struct mm_struct *mm;
+ struct list_head list;
+ struct mmu_notifier mmu_notifier;
+};
+
+struct sgx_encl {
+ atomic_t flags;
+ u64 secs_attributes;
+ u64 allowed_attributes;
+ unsigned int page_cnt;
+ unsigned int secs_child_cnt;
+ struct mutex lock;
+ struct list_head mm_list;
+ spinlock_t mm_lock;
+ struct file *backing;
+ struct kref refcount;
+ struct srcu_struct srcu;
+ unsigned long base;
+ unsigned long size;
+ unsigned long ssaframesize;
+ struct radix_tree_root page_tree;
+ struct sgx_encl_page secs;
+ cpumask_t cpumask;
+};
+
+extern const struct vm_operations_struct sgx_vm_ops;
+
+int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
+ struct vm_area_struct **vma);
+void sgx_encl_destroy(struct sgx_encl *encl);
+void sgx_encl_release(struct kref *ref);
+int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
+int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
+ unsigned long end, unsigned long vm_prot_bits);
+
+#endif /* _X86_ENCL_H */
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
new file mode 100644
index 000000000000..83513cdfd1c0
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -0,0 +1,697 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2016-19 Intel Corporation.
+
+#include <asm/mman.h>
+#include <linux/mman.h>
+#include <linux/delay.h>
+#include <linux/file.h>
+#include <linux/hashtable.h>
+#include <linux/highmem.h>
+#include <linux/ratelimit.h>
+#include <linux/sched/signal.h>
+#include <linux/shmem_fs.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+#include "driver.h"
+#include "encl.h"
+#include "encls.h"
+
+/* A per-cpu cache for the last known values of IA32_SGXLEPUBKEYHASHx MSRs. */
+static DEFINE_PER_CPU(u64 [4], sgx_lepubkeyhash_cache);
+
+static u32 sgx_calc_ssaframesize(u32 miscselect, u64 xfrm)
+{
+ u32 size_max = PAGE_SIZE;
+ u32 size;
+ int i;
+
+ for (i = 2; i < 64; i++) {
+ if (!((1 << i) & xfrm))
+ continue;
+
+ size = SGX_SSA_GPRS_SIZE + sgx_xsave_size_tbl[i];
+ if (miscselect & SGX_MISC_EXINFO)
+ size += SGX_SSA_MISC_EXINFO_SIZE;
+
+ if (size > size_max)
+ size_max = size;
+ }
+
+ return PFN_UP(size_max);
+}
+
+static int sgx_validate_secs(const struct sgx_secs *secs,
+ unsigned long ssaframesize)
+{
+ if (secs->size < (2 * PAGE_SIZE) || !is_power_of_2(secs->size))
+ return -EINVAL;
+
+ if (secs->base & (secs->size - 1))
+ return -EINVAL;
+
+ if (secs->miscselect & sgx_misc_reserved_mask ||
+ secs->attributes & sgx_attributes_reserved_mask ||
+ secs->xfrm & sgx_xfrm_reserved_mask)
+ return -EINVAL;
+
+ if (secs->attributes & SGX_ATTR_MODE64BIT) {
+ if (secs->size > sgx_encl_size_max_64)
+ return -EINVAL;
+ } else if (secs->size > sgx_encl_size_max_32)
+ return -EINVAL;
+
+ if (!(secs->xfrm & XFEATURE_MASK_FP) ||
+ !(secs->xfrm & XFEATURE_MASK_SSE) ||
+ (((secs->xfrm >> XFEATURE_BNDREGS) & 1) !=
+ ((secs->xfrm >> XFEATURE_BNDCSR) & 1)))
+ return -EINVAL;
+
+ if (!secs->ssa_frame_size || ssaframesize > secs->ssa_frame_size)
+ return -EINVAL;
+
+ if (memchr_inv(secs->reserved1, 0, sizeof(secs->reserved1)) ||
+ memchr_inv(secs->reserved2, 0, sizeof(secs->reserved2)) ||
+ memchr_inv(secs->reserved3, 0, sizeof(secs->reserved3)) ||
+ memchr_inv(secs->reserved4, 0, sizeof(secs->reserved4)))
+ return -EINVAL;
+
+ return 0;
+}
+
+static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
+ unsigned long offset,
+ u64 secinfo_flags)
+{
+ struct sgx_encl_page *encl_page;
+ unsigned long prot;
+
+ encl_page = kzalloc(sizeof(*encl_page), GFP_KERNEL);
+ if (!encl_page)
+ return ERR_PTR(-ENOMEM);
+
+ encl_page->desc = encl->base + offset;
+ encl_page->encl = encl;
+
+ prot = _calc_vm_trans(secinfo_flags, SGX_SECINFO_R, PROT_READ) |
+ _calc_vm_trans(secinfo_flags, SGX_SECINFO_W, PROT_WRITE) |
+ _calc_vm_trans(secinfo_flags, SGX_SECINFO_X, PROT_EXEC);
+
+ /*
+ * TCS pages must always RW set for CPU access while the SECINFO
+ * permissions are *always* zero - the CPU ignores the user provided
+ * values and silently overwrites them with zero permissions.
+ */
+ if ((secinfo_flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
+ prot |= PROT_READ | PROT_WRITE;
+
+ /* Calculate maximum of the VM flags for the page. */
+ encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
+
+ return encl_page;
+}
+
+static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
+{
+ unsigned long encl_size = secs->size + PAGE_SIZE;
+ struct sgx_epc_page *secs_epc;
+ unsigned long ssaframesize;
+ struct sgx_pageinfo pginfo;
+ struct sgx_secinfo secinfo;
+ struct file *backing;
+ long ret;
+
+ if (atomic_read(&encl->flags) & SGX_ENCL_CREATED)
+ return -EINVAL;
+
+ ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
+ if (sgx_validate_secs(secs, ssaframesize)) {
+ pr_debug("invalid SECS\n");
+ return -EINVAL;
+ }
+
+ backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
+ VM_NORESERVE);
+ if (IS_ERR(backing))
+ return PTR_ERR(backing);
+
+ encl->backing = backing;
+
+ secs_epc = sgx_try_alloc_page();
+ if (IS_ERR(secs_epc)) {
+ ret = PTR_ERR(secs_epc);
+ goto err_out_backing;
+ }
+
+ encl->secs.epc_page = secs_epc;
+
+ pginfo.addr = 0;
+ pginfo.contents = (unsigned long)secs;
+ pginfo.metadata = (unsigned long)&secinfo;
+ pginfo.secs = 0;
+ memset(&secinfo, 0, sizeof(secinfo));
+
+ ret = __ecreate((void *)&pginfo, sgx_epc_addr(secs_epc));
+ if (ret) {
+ pr_debug("ECREATE returned %ld\n", ret);
+ goto err_out;
+ }
+
+ if (secs->attributes & SGX_ATTR_DEBUG)
+ atomic_or(SGX_ENCL_DEBUG, &encl->flags);
+
+ encl->secs.encl = encl;
+ encl->secs_attributes = secs->attributes;
+ encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK;
+ encl->base = secs->base;
+ encl->size = secs->size;
+ encl->ssaframesize = secs->ssa_frame_size;
+
+ /*
+ * Set SGX_ENCL_CREATED only after the enclave is fully prepped. This
+ * allows setting and checking enclave creation without having to take
+ * encl->lock.
+ */
+ atomic_or(SGX_ENCL_CREATED, &encl->flags);
+
+ return 0;
+
+err_out:
+ sgx_free_page(encl->secs.epc_page);
+ encl->secs.epc_page = NULL;
+
+err_out_backing:
+ fput(encl->backing);
+ encl->backing = NULL;
+
+ return ret;
+}
+
+/**
+ * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
+ * @filep: open file to /dev/sgx
+ * @arg: userspace pointer to a struct sgx_enclave_create instance
+ *
+ * Allocate kernel data structures for a new enclave and execute ECREATE after
+ * verifying the correctness of the provided SECS.
+ *
+ * Note, enforcement of restricted and disallowed attributes is deferred until
+ * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
+ * checked by sgx_ioc_enclave_create().
+ *
+ * Return:
+ * 0 on success,
+ * -errno otherwise
+ */
+static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
+{
+ struct sgx_enclave_create ecreate;
+ struct page *secs_page;
+ struct sgx_secs *secs;
+ int ret;
+
+ if (copy_from_user(&ecreate, arg, sizeof(ecreate)))
+ return -EFAULT;
+
+ secs_page = alloc_page(GFP_KERNEL);
+ if (!secs_page)
+ return -ENOMEM;
+
+ secs = kmap(secs_page);
+ if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = sgx_encl_create(encl, secs);
+
+out:
+ kunmap(secs_page);
+ __free_page(secs_page);
+ return ret;
+}
+
+static int sgx_validate_secinfo(struct sgx_secinfo *secinfo)
+{
+ u64 perm = secinfo->flags & SGX_SECINFO_PERMISSION_MASK;
+ u64 pt = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
+
+ if (pt != SGX_SECINFO_REG && pt != SGX_SECINFO_TCS)
+ return -EINVAL;
+
+ if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R))
+ return -EINVAL;
+
+ /*
+ * CPU will silently overwrite the permissions as zero, which means
+ * that we need to validate it ourselves.
+ */
+ if (pt == SGX_SECINFO_TCS && perm)
+ return -EINVAL;
+
+ if (secinfo->flags & SGX_SECINFO_RESERVED_MASK)
+ return -EINVAL;
+
+ if (memchr_inv(secinfo->reserved, 0, sizeof(secinfo->reserved)))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int __sgx_encl_add_page(struct sgx_encl *encl,
+ struct sgx_encl_page *encl_page,
+ struct sgx_epc_page *epc_page,
+ struct sgx_secinfo *secinfo, unsigned long src)
+{
+ struct sgx_pageinfo pginfo;
+ struct vm_area_struct *vma;
+ struct page *src_page;
+ int ret;
+
+ /* Query vma's VM_MAYEXEC as an indirect path_noexec() check. */
+ if (encl_page->vm_max_prot_bits & VM_EXEC) {
+ vma = find_vma(current->mm, src);
+ if (!vma)
+ return -EFAULT;
+
+ if (!(vma->vm_flags & VM_MAYEXEC))
+ return -EACCES;
+ }
+
+ ret = get_user_pages(src, 1, 0, &src_page, NULL);
+ if (ret < 1)
+ return ret;
+
+ pginfo.secs = (unsigned long)sgx_epc_addr(encl->secs.epc_page);
+ pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
+ pginfo.metadata = (unsigned long)secinfo;
+ pginfo.contents = (unsigned long)kmap_atomic(src_page);
+
+ ret = __eadd(&pginfo, sgx_epc_addr(epc_page));
+
+ kunmap_atomic((void *)pginfo.contents);
+ put_page(src_page);
+
+ return ret ? -EIO : 0;
+}
+
+static int __sgx_encl_extend(struct sgx_encl *encl,
+ struct sgx_epc_page *epc_page)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < 16; i++) {
+ ret = __eextend(sgx_epc_addr(encl->secs.epc_page),
+ sgx_epc_addr(epc_page) + (i * 0x100));
+ if (ret) {
+ if (encls_failed(ret))
+ ENCLS_WARN(ret, "EEXTEND");
+ return -EIO;
+ }
+ }
+
+ return 0;
+}
+
+static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
+ unsigned long offset, unsigned long length,
+ struct sgx_secinfo *secinfo, unsigned long flags)
+{
+ struct sgx_encl_page *encl_page;
+ struct sgx_epc_page *epc_page;
+ int ret;
+
+ encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
+ if (IS_ERR(encl_page))
+ return PTR_ERR(encl_page);
+
+ epc_page = sgx_try_alloc_page();
+ if (IS_ERR(epc_page)) {
+ kfree(encl_page);
+ return PTR_ERR(epc_page);
+ }
+
+ if (atomic_read(&encl->flags) &
+ (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
+ ret = -EFAULT;
+ goto err_out_free;
+ }
+
+ down_read(¤t->mm->mmap_sem);
+ mutex_lock(&encl->lock);
+
+ /*
+ * Insert prior to EADD in case of OOM. EADD modifies MRENCLAVE, i.e.
+ * can't be gracefully unwound, while failure on EADD/EXTEND is limited
+ * to userspace errors (or kernel/hardware bugs).
+ */
+ ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
+ encl_page);
+ if (ret)
+ goto err_out_unlock;
+
+ ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
+ src);
+ if (ret)
+ goto err_out;
+
+ /*
+ * Complete the "add" before doing the "extend" so that the "add"
+ * isn't in a half-baked state in the extremely unlikely scenario the
+ * the enclave will be destroyed in response to EEXTEND failure.
+ */
+ encl_page->encl = encl;
+ encl_page->epc_page = epc_page;
+ encl->secs_child_cnt++;
+
+ if (flags & SGX_PAGE_MEASURE) {
+ ret = __sgx_encl_extend(encl, epc_page);
+ if (ret)
+ goto err_out;
+ }
+
+ mutex_unlock(&encl->lock);
+ up_read(¤t->mm->mmap_sem);
+ return ret;
+
+err_out:
+ radix_tree_delete(&encl_page->encl->page_tree,
+ PFN_DOWN(encl_page->desc));
+
+err_out_unlock:
+ mutex_unlock(&encl->lock);
+ up_read(¤t->mm->mmap_sem);
+
+err_out_free:
+ sgx_free_page(epc_page);
+ kfree(encl_page);
+
+ /*
+ * Destroy enclave on ENCLS failure as this means that EPC has been
+ * invalidated.
+ */
+ if (ret == -EIO)
+ sgx_encl_destroy(encl);
+
+ return ret;
+}
+
+/**
+ * sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES
+ * @encl: pointer to an enclave instance (via ioctl() file pointer)
+ * @arg: a user pointer to a struct sgx_enclave_add_pages instance
+ *
+ * Add one or more pages to an uninitialized enclave, and optionally extend the
+ * measurement with the contents of the page. The address range of pages must
+ * be contiguous. The SECINFO and measurement mask are applied to all pages.
+ *
+ * A SECINFO for a TCS is required to always contain zero permissions because
+ * CPU silently zeros them. Allowing anything else would cause a mismatch in
+ * the measurement.
+ *
+ * mmap()'s protection bits are capped by the page permissions. For each page
+ * address, the maximum protection bits are computed with the following
+ * heuristics:
+ *
+ * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO permissions.
+ * 2. A TCS page: PROT_R | PROT_W.
+ * 3. No page: PROT_NONE.
+ *
+ * mmap() is not allowed to surpass the minimum of the maximum protection bits
+ * within the given address range.
+ *
+ * As stated above, a non-existent page is interpreted as a page with no
+ * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek
+ * an address range for the enclave that can be then populated into SECS.
+ *
+ * If ENCLS opcode fails, that effectively means that EPC has been invalidated.
+ * When this happens the enclave is destroyed and -EIO is returned to the
+ * caller.
+ *
+ * Return:
+ * 0 on success,
+ * -EACCES if an executable source page is located in a noexec partition,
+ * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
+ * -errno otherwise
+ */
+static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
+{
+ struct sgx_enclave_add_pages addp;
+ struct sgx_secinfo secinfo;
+ unsigned long c;
+ int ret;
+
+ if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
+ return -EINVAL;
+
+ if (copy_from_user(&addp, arg, sizeof(addp)))
+ return -EFAULT;
+
+ if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
+ !IS_ALIGNED(addp.src, PAGE_SIZE))
+ return -EINVAL;
+
+ if (!(access_ok(addp.src, PAGE_SIZE)))
+ return -EFAULT;
+
+ if (addp.length & (PAGE_SIZE - 1))
+ return -EINVAL;
+
+ if (addp.offset + addp.length - PAGE_SIZE >= encl->size)
+ return -EINVAL;
+
+ if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
+ sizeof(secinfo)))
+ return -EFAULT;
+
+ if (sgx_validate_secinfo(&secinfo))
+ return -EINVAL;
+
+ for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
+ if (signal_pending(current)) {
+ ret = -EINTR;
+ break;
+ }
+
+ if (need_resched())
+ cond_resched();
+
+ ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
+ addp.length - c, &secinfo, addp.flags);
+ if (ret)
+ break;
+ }
+
+ addp.count = c;
+
+ if (copy_to_user(arg, &addp, sizeof(addp)))
+ return -EFAULT;
+
+ return ret;
+}
+
+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;
+}
+
+static void sgx_update_lepubkeyhash_msrs(u64 *lepubkeyhash, bool enforce)
+{
+ u64 *cache;
+ int i;
+
+ cache = per_cpu(sgx_lepubkeyhash_cache, smp_processor_id());
+ for (i = 0; i < 4; i++) {
+ if (enforce || (lepubkeyhash[i] != cache[i])) {
+ wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0 + i, lepubkeyhash[i]);
+ cache[i] = lepubkeyhash[i];
+ }
+ }
+}
+
+static int sgx_einit(struct sgx_sigstruct *sigstruct,
+ struct sgx_einittoken *token,
+ struct sgx_epc_page *secs, u64 *lepubkeyhash)
+{
+ int ret;
+
+ if (!boot_cpu_has(X86_FEATURE_SGX_LC))
+ return __einit(sigstruct, token, sgx_epc_addr(secs));
+
+ preempt_disable();
+ sgx_update_lepubkeyhash_msrs(lepubkeyhash, false);
+ ret = __einit(sigstruct, token, sgx_epc_addr(secs));
+ if (ret == SGX_INVALID_EINITTOKEN) {
+ sgx_update_lepubkeyhash_msrs(lepubkeyhash, true);
+ ret = __einit(sigstruct, token, sgx_epc_addr(secs));
+ }
+ preempt_enable();
+ return ret;
+}
+
+static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct,
+ struct sgx_einittoken *token)
+{
+ u64 mrsigner[4];
+ int ret;
+ int i;
+ int j;
+
+ /* Check that the required attributes have been authorized. */
+ if (encl->secs_attributes & ~encl->allowed_attributes)
+ return -EACCES;
+
+ ret = sgx_get_key_hash(sigstruct->modulus, mrsigner);
+ if (ret)
+ return ret;
+
+ mutex_lock(&encl->lock);
+
+ if (atomic_read(&encl->flags) & SGX_ENCL_INITIALIZED) {
+ ret = -EFAULT;
+ goto err_out;
+ }
+
+ for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
+ for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
+ ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
+ mrsigner);
+ if (ret == SGX_UNMASKED_EVENT)
+ continue;
+ else
+ break;
+ }
+
+ if (ret != SGX_UNMASKED_EVENT)
+ break;
+
+ msleep_interruptible(SGX_EINIT_SLEEP_TIME);
+
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ goto err_out;
+ }
+ }
+
+ if (ret & ENCLS_FAULT_FLAG) {
+ if (encls_failed(ret))
+ ENCLS_WARN(ret, "EINIT");
+
+ sgx_encl_destroy(encl);
+ ret = -EFAULT;
+ } else if (ret) {
+ pr_debug("EINIT returned %d\n", ret);
+ ret = -EPERM;
+ } else {
+ atomic_or(SGX_ENCL_INITIALIZED, &encl->flags);
+ }
+
+err_out:
+ mutex_unlock(&encl->lock);
+ return ret;
+}
+
+/**
+ * sgx_ioc_enclave_init - handler for %SGX_IOC_ENCLAVE_INIT
+ *
+ * @filep: open file to /dev/sgx
+ * @arg: userspace pointer to a struct sgx_enclave_init instance
+ *
+ * Flush any outstanding enqueued EADD operations and perform EINIT. The
+ * Launch Enclave Public Key Hash MSRs are rewritten as necessary to match
+ * the enclave's MRSIGNER, which is caculated from the provided sigstruct.
+ *
+ * Return:
+ * 0 on success,
+ * SGX error code on EINIT failure,
+ * -errno otherwise
+ */
+static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
+{
+ struct sgx_einittoken *einittoken;
+ struct sgx_sigstruct *sigstruct;
+ struct sgx_enclave_init einit;
+ struct page *initp_page;
+ int ret;
+
+ if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
+ return -EINVAL;
+
+ if (copy_from_user(&einit, arg, sizeof(einit)))
+ return -EFAULT;
+
+ initp_page = alloc_page(GFP_KERNEL);
+ if (!initp_page)
+ return -ENOMEM;
+
+ sigstruct = kmap(initp_page);
+ einittoken = (struct sgx_einittoken *)
+ ((unsigned long)sigstruct + PAGE_SIZE / 2);
+ memset(einittoken, 0, sizeof(*einittoken));
+
+ if (copy_from_user(sigstruct, (void __user *)einit.sigstruct,
+ sizeof(*sigstruct))) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = sgx_encl_init(encl, sigstruct, einittoken);
+
+out:
+ kunmap(initp_page);
+ __free_page(initp_page);
+ return ret;
+}
+
+
+long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+ struct sgx_encl *encl = filep->private_data;
+ int ret, encl_flags;
+
+ encl_flags = atomic_fetch_or(SGX_ENCL_IOCTL, &encl->flags);
+ if (encl_flags & SGX_ENCL_IOCTL)
+ return -EBUSY;
+
+ if (encl_flags & SGX_ENCL_DEAD)
+ return -EFAULT;
+
+ switch (cmd) {
+ case SGX_IOC_ENCLAVE_CREATE:
+ ret = sgx_ioc_enclave_create(encl, (void __user *)arg);
+ break;
+ case SGX_IOC_ENCLAVE_ADD_PAGES:
+ ret = sgx_ioc_enclave_add_pages(encl, (void __user *)arg);
+ break;
+ case SGX_IOC_ENCLAVE_INIT:
+ ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
+ break;
+ default:
+ ret = -ENOIOCTLCMD;
+ break;
+ }
+
+ atomic_andnot(SGX_ENCL_IOCTL, &encl->flags);
+
+ return ret;
+}
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 60d82e7537c8..842f9abba1c0 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -8,6 +8,7 @@
#include <linux/ratelimit.h>
#include <linux/sched/signal.h>
#include <linux/slab.h>
+#include "driver.h"
#include "encls.h"
struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
@@ -193,6 +194,8 @@ static bool __init sgx_page_cache_init(void)
static void __init sgx_init(void)
{
+ int ret;
+
if (!boot_cpu_has(X86_FEATURE_SGX))
return;
@@ -202,10 +205,17 @@ static void __init sgx_init(void)
if (!sgx_page_reclaimer_init())
goto err_page_cache;
+ ret = sgx_drv_init();
+ if (ret)
+ goto err_kthread;
+
return;
+err_kthread:
+ kthread_stop(ksgxswapd_tsk);
+
err_page_cache:
sgx_page_cache_teardown();
}
-arch_initcall(sgx_init);
+device_initcall(sgx_init);
diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
index f071158d34f6..bdb42f4326aa 100644
--- a/arch/x86/kernel/cpu/sgx/reclaim.c
+++ b/arch/x86/kernel/cpu/sgx/reclaim.c
@@ -10,6 +10,7 @@
#include <linux/sched/mm.h>
#include <linux/sched/signal.h>
#include "encls.h"
+#include "driver.h"
struct task_struct *ksgxswapd_tsk;
--
2.20.1
^ permalink raw reply related
* kernel BUG at lib/assoc_array.c:LINE!
From: syzbot @ 2020-02-04 4:44 UTC (permalink / raw)
To: dhowells, jarkko.sakkinen, jmorris, keyrings, linux-kernel,
linux-security-module, serge, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 46d6b7be Merge git://git.kernel.org/pub/scm/linux/kernel/g..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11383a79e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=6dda7ccc1e75a63f
dashboard link: https://syzkaller.appspot.com/bug?extid=23e14950fa7550d86091
compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+23e14950fa7550d86091@syzkaller.appspotmail.com
------------[ cut here ]------------
kernel BUG at lib/assoc_array.c:652!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 2778 Comm: kworker/0:37 Not tainted 5.5.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: afs afs_manage_cell
RIP: 0010:assoc_array_insert_into_terminal_node lib/assoc_array.c:652 [inline]
RIP: 0010:assoc_array_insert+0x2baa/0x2bd0 lib/assoc_array.c:1001
Code: 0f 0b e8 a9 64 d4 fd 0f 0b e8 a2 64 d4 fd 0f 0b e8 9b 64 d4 fd 0f 0b e8 94 64 d4 fd 0f 0b e8 8d 64 d4 fd 0f 0b e8 86 64 d4 fd <0f> 0b e8 7f 64 d4 fd 0f 0b e8 78 64 d4 fd 0f 0b e8 71 64 d4 fd 0f
RSP: 0018:ffffc900087ff810 EFLAGS: 00010293
RAX: ffffffff83a25a7a RBX: 1ffff11012d568af RCX: ffff88809f34a580
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc900087ff920 R08: ffffffff83a249fd R09: ffffffff83538f4f
R10: ffff88809f34a580 R11: 0000000000000004 R12: ffff888096ab4588
R13: ffff888096ab4500 R14: ffff888096ab4578 R15: dffffc0000000000
FS: 0000000000000000(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000738000 CR3: 0000000054a3b000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
__key_link_begin+0xfe/0x230 security/keys/keyring.c:1316
construct_alloc_key security/keys/request_key.c:404 [inline]
construct_key_and_link security/keys/request_key.c:499 [inline]
request_key_and_link+0x9b6/0x1680 security/keys/request_key.c:637
request_key_tag+0x53/0x190 security/keys/request_key.c:701
dns_query+0x266/0x6c0 net/dns_resolver/dns_query.c:128
afs_dns_query+0xdd/0x320 fs/afs/addr_list.c:249
afs_update_cell fs/afs/cell.c:391 [inline]
afs_manage_cell+0xda2/0x1500 fs/afs/cell.c:693
process_one_work+0x7f5/0x10f0 kernel/workqueue.c:2264
worker_thread+0xbbc/0x1630 kernel/workqueue.c:2410
kthread+0x332/0x350 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Modules linked in:
---[ end trace 9dabb2deade74362 ]---
RIP: 0010:assoc_array_insert_into_terminal_node lib/assoc_array.c:652 [inline]
RIP: 0010:assoc_array_insert+0x2baa/0x2bd0 lib/assoc_array.c:1001
Code: 0f 0b e8 a9 64 d4 fd 0f 0b e8 a2 64 d4 fd 0f 0b e8 9b 64 d4 fd 0f 0b e8 94 64 d4 fd 0f 0b e8 8d 64 d4 fd 0f 0b e8 86 64 d4 fd <0f> 0b e8 7f 64 d4 fd 0f 0b e8 78 64 d4 fd 0f 0b e8 71 64 d4 fd 0f
RSP: 0018:ffffc900087ff810 EFLAGS: 00010293
RAX: ffffffff83a25a7a RBX: 1ffff11012d568af RCX: ffff88809f34a580
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc900087ff920 R08: ffffffff83a249fd R09: ffffffff83538f4f
R10: ffff88809f34a580 R11: 0000000000000004 R12: ffff888096ab4588
R13: ffff888096ab4500 R14: ffff888096ab4578 R15: dffffc0000000000
FS: 0000000000000000(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000013e4978 CR3: 000000008adb2000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* [PATCH SECOND RESEND] MAINTAINERS: fix style in SAFESETID SECURITY MODULE
From: Lukas Bulwahn @ 2020-02-04 4:04 UTC (permalink / raw)
To: Micah Morton
Cc: Joe Perches, Andy Shevchenko, linux-security-module,
kernel-janitors, linux-kernel, Lukas Bulwahn
Commit fc5b34a35458 ("Add entry in MAINTAINERS file for SafeSetID LSM")
slips in some formatting with spaces instead of tabs, which
./scripts/checkpatch.pl -f MAINTAINERS complains about:
WARNING: MAINTAINERS entries use one tab after TYPE:
#14394: FILE: MAINTAINERS:14394:
+M: Micah Morton <mortonm@chromium.org>
WARNING: MAINTAINERS entries use one tab after TYPE:
#14395: FILE: MAINTAINERS:14395:
+S: Supported
WARNING: MAINTAINERS entries use one tab after TYPE:
#14396: FILE: MAINTAINERS:14396:
+F: security/safesetid/
WARNING: MAINTAINERS entries use one tab after TYPE:
#14397: FILE: MAINTAINERS:14397:
+F: Documentation/admin-guide/LSM/SafeSetID.rst
Fixes: fc5b34a35458 ("Add entry in MAINTAINERS file for SafeSetID LSM")
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on current master and next-20200203
Micah, please pick this patch.
MAINTAINERS | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 1f77fb8cdde3..83c7879aa4b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14578,10 +14578,10 @@ F: drivers/media/pci/saa7146/
F: include/media/drv-intf/saa7146*
SAFESETID SECURITY MODULE
-M: Micah Morton <mortonm@chromium.org>
-S: Supported
-F: security/safesetid/
-F: Documentation/admin-guide/LSM/SafeSetID.rst
+M: Micah Morton <mortonm@chromium.org>
+S: Supported
+F: security/safesetid/
+F: Documentation/admin-guide/LSM/SafeSetID.rst
SAMSUNG AUDIO (ASoC) DRIVERS
M: Krzysztof Kozlowski <krzk@kernel.org>
--
2.17.1
^ permalink raw reply related
* [ANNOUNCE][CFP] Linux Security Summit North America 2020
From: James Morris @ 2020-02-03 23:59 UTC (permalink / raw)
To: linux-security-module
Cc: linux-kernel, lwn, fedora-selinux-list, linux-crypto,
kernel-hardening, linux-integrity, selinux, Audit-ML,
gentoo-hardened, keyrings, tpmdd-devel,
Linux Security Summit Program Committee
[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]
==============================================================================
ANNOUNCEMENT AND CALL FOR PARTICIPATION
LINUX SECURITY SUMMIT NORTH AMERICA 2020
24-26 JUNE
AUSTIN, TEXAS, USA
==============================================================================
DESCRIPTION
Linux Security Summit North America (LSS-NA) is a technical forum for
collaboration between Linux developers, researchers, and end-users. Its
primary aim is to foster community efforts in analyzing and solving Linux
security challenges.
The program committee currently seeks proposals for:
* Refereed Presentations:
45 minutes in length.
* Panel Discussion Topics:
45 minutes in length.
* Short Topics:
30 minutes in total, including at least 10 minutes discussion.
* Tutorials
90 minutes in length.
Tutorial sessions should be focused on advanced Linux security defense
topics within areas such as the kernel, compiler, and security-related
libraries. Priority will be given to tutorials created for this conference,
and those where the presenter a leading subject matter expert on the topic.
Topic areas include, but are not limited to:
* Kernel self-protection
* Access control
* Cryptography and key management
* Integrity policy and enforcement
* Hardware Security
* IoT and embedded security
* Virtualization and containers
* System-specific system hardening
* Case studies
* Security tools
* Security UX
* Emerging technologies, threats & techniques
Proposals should be submitted via:
https://events.linuxfoundation.org/linux-security-summit-north-america/program/cfp/
DATES
* CFP close: March 31
* CFP notifications: April 13
* Schedule announced: April 16
* Event: June 24-26
WHO SHOULD ATTEND
We're seeking a diverse range of attendees and welcome participation by
people involved in Linux security development, operations, and research.
LSS-NA is a unique global event that provides the opportunity to present and
discuss your work or research with key Linux security community members and
maintainers. It’s also useful for those who wish to keep up with the latest
in Linux security development and to provide input to the development
process.
WEB SITE
https://events.linuxfoundation.org/linux-security-summit-north-america/
TWITTER
For event updates and announcements, follow:
https://twitter.com/LinuxSecSummit
#linuxsecuritysummit
PROGRAM COMMITTEE
The program committee for LSS 2020 is:
* James Morris, Microsoft
* Serge Hallyn, Cisco
* Paul Moore, Cisco
* Stephen Smalley, NSA
* Elena Reshetova, Intel
* John Johansen, Canonical
* Kees Cook, Google
* Casey Schaufler, Intel
* Mimi Zohar, IBM
* David A. Wheeler, Institute for Defense Analyses
The program committee may be contacted as a group via email:
lss-pc () lists.linuxfoundation.org
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-03 22:49 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, casey.schaufler, jmorris,
linux-security-module, selinux
Cc: keescook, penguin-kernel, paul
In-Reply-To: <ad8c9dce-7078-a53f-ac8a-56f069d7290b@schaufler-ca.com>
On 2/3/20 1:43 PM, Casey Schaufler wrote:
> On 2/3/2020 1:02 PM, John Johansen wrote:
>> On 1/24/20 12:16 PM, Stephen Smalley wrote:
>>> ...
>>>
>>> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces.
>>>
>>
>> It is not needed for the new interface. And if I could go back and remove it from the old interface I would.
>
> So, what would the "context" for this case be? "/usr/sbin/cupsd" or "enforce"?
>
"/usr/sbin/cupsd"
"enforce" is the profile mode which can be looked up separately using "/usr/sbin/cupsd" if it is really needed.
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-02-03 21:43 UTC (permalink / raw)
To: John Johansen, Stephen Smalley, casey.schaufler, jmorris,
linux-security-module, selinux
Cc: keescook, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <cfb7a1c1-0e74-d012-7e88-c3879ddbf696@canonical.com>
On 2/3/2020 1:02 PM, John Johansen wrote:
> On 1/24/20 12:16 PM, Stephen Smalley wrote:
>> ...
>>
>> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces.
>>
>
> It is not needed for the new interface. And if I could go back and remove it from the old interface I would.
So, what would the "context" for this case be? "/usr/sbin/cupsd" or "enforce"?
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-02-03 21:39 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <cdb0ba7f-2863-d721-7ec2-1e01464e2b41@tycho.nsa.gov>
On 2/3/2020 11:37 AM, Stephen Smalley wrote:
> On 2/3/20 1:54 PM, Stephen Smalley wrote:
>> I'd suggest something like the following instead:
>> * @getprocattr
>> * Get the value of process attribute @name for task @p into a buffer
>> * allocated by the security module and returned via @value. The
>> * caller will free the returned buffer via kfree. The set of
>> * attribute names is fixed by proc but the format of @value is up
>> * to the security module authors except for the "context" attribute,
>> * whose value is required to be a NUL-terminated printable ASCII
>> * string without trailing whitespace.
>> * @p the task whose attribute is being fetched
>> * @name the name of the process attribute being fetched
>> * @value set to point to the buffer containing the attribute value
>> * Return the length of @value including the NUL on success, or -errno on error.
I'm fine with either description, so I'll adopt yours.
>>
>> The printable ASCII bit is based on what the dbus maintainer requested in previous discussions. The question of whether the terminating NUL should be included in the returned length was otherwise left ambiguous and inconsistent in your patch among the different security modules; if you prefer not including it in the length returned by the security modules, you'll need to adjust SELinux at least to not do so for "context".
append_ctx() is supposed to take the possibility that the module specific context string
may or may not include a trailing '\0' into account. Alas, I see a problem in the current
version for the "no trailing '\0'" case. On the other hand, with your proposed description
the trailing '\0' is required, so it's a matter of verifying that all modules providing
a "context" provide one that includes a trailing '\0' and return strlen()+1.
>
> BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break.
The behavior of interfaces (e.g. "current", "exec") that are module defined
is only of concern with respect to to "display" behavior. If a security module
wants to provide a variable size binary blob in "current" I would object on
principle, but policy as I understand it has long been that if the authors want
to do that, it's their call. The "context" has a defined format, and it would
be up to the authors to come up with a printable ASCII representation of the
binary blob. If they care. They're not required to provide a "context".
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-03 21:02 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler, casey.schaufler, jmorris,
linux-security-module, selinux
Cc: keescook, penguin-kernel, paul
In-Reply-To: <6bd3e393-e1df-7117-d15a-81cb1946807b@tycho.nsa.gov>
On 1/24/20 12:16 PM, Stephen Smalley wrote:
> On 1/24/20 2:28 PM, Casey Schaufler wrote:
>> On 1/24/2020 8:20 AM, Stephen Smalley wrote:
>>> On 1/24/20 9:42 AM, Stephen Smalley wrote:
>>>> On 1/23/20 7:23 PM, Casey Schaufler wrote:
>>>>> Add an entry /proc/.../attr/context which displays the full
>>>>> process security "context" in compound format:'
>>>>> lsm1\0value\0lsm2\0value\0...
>>>>> This entry is not writable.
>>>>>
>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>>> Cc: linux-api@vger.kernel.org
>>>>
>>>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever.
>>>
>>> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC.
>>
>> I don't believe that a new hook is necessary, and that introducing one
>> just to deal with a '\n' would be pedantic. We really have two rational
>> options. AppArmor could drop the '\n' from their "context". Or, we can
>> simply document that the /proc/pid/attr/context interface will trim any
>> trailing whitespace. I understand that this would be a break from the
>> notion that the LSM infrastructure does not constrain what a module uses
>> for its own data. On the other hand, we have been saying that "context"s
>> are strings, and ignoring trailing whitespace is usual behavior for
>> strings.
>
> Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them. But you need to do one of those things before this interface gets merged upstream.
>
> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces.
>
It is not needed for the new interface. And if I could go back and remove it from the old interface I would.
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-03 20:59 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, casey.schaufler, jmorris,
linux-security-module, selinux
Cc: keescook, penguin-kernel, paul
In-Reply-To: <9afb8d9d-a590-0e13-bf46-53a347ea15dd@schaufler-ca.com>
On 1/24/20 11:28 AM, Casey Schaufler wrote:
> On 1/24/2020 8:20 AM, Stephen Smalley wrote:
>> On 1/24/20 9:42 AM, Stephen Smalley wrote:
>>> On 1/23/20 7:23 PM, Casey Schaufler wrote:
>>>> Add an entry /proc/.../attr/context which displays the full
>>>> process security "context" in compound format:'
>>>> lsm1\0value\0lsm2\0value\0...
>>>> This entry is not writable.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> Cc: linux-api@vger.kernel.org
>>>
>>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever.
>>
>> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC.
>
> I don't believe that a new hook is necessary, and that introducing one
> just to deal with a '\n' would be pedantic. We really have two rational
> options. AppArmor could drop the '\n' from their "context". Or, we can
> simply document that the /proc/pid/attr/context interface will trim any
> trailing whitespace. I understand that this would be a break from the
> notion that the LSM infrastructure does not constrain what a module uses
> for its own data. On the other hand, we have been saying that "context"s
> are strings, and ignoring trailing whitespace is usual behavior for
> strings.
>
>
AppArmor can drop the trailing '\n' it is not required. I would say it
could be dropped from /proc/pid/attr/current except there may be some
third party code that requires it.
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-03 20:54 UTC (permalink / raw)
To: Simon McVittie, Stephen Smalley
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, penguin-kernel, paul
In-Reply-To: <20200127200558.GA1657845@horizon>
On 1/27/20 12:05 PM, Simon McVittie wrote:
> On Fri, 24 Jan 2020 at 15:16:36 -0500, Stephen Smalley wrote:
>> Aside from the trailing newline and \0 issues, AppArmor also has a
>> whitespace-separated (mode) field that may or may not be present in the
>> contexts it presently returns, ala "/usr/sbin/cupsd (enforce)".
>
> My understanding from last time I worked with AppArmor is that this
> is genuinely part of the context, and whether it is present or absent
> does not vary according to the kernel API used to access contexts.
> AppArmor-specific higher-level APIs parse it into a label and an optional
> mode, but LSM-agnostic user-space APIs (like the one in dbus) pass the
> whole string through as-is.
>
> (In practice it seems to be present if and only if the context is
> something other than "unconfined", although I don't know offhand whether
> that's an API guarantee.)
>
Correct, currently it is always included unless the context is unconfined.
There is no guarantee that I am aware of beyond that is what the code
did in the past and so as to not break things we continue to do exactly
that.
The mode certainly does not need to be included in a newer interface.
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Stephen Smalley @ 2020-02-03 19:37 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <09d96236-715a-344a-38bc-c05208698125@tycho.nsa.gov>
On 2/3/20 1:54 PM, Stephen Smalley wrote:
> I'd suggest something like the following instead:
> * @getprocattr
> * Get the value of process attribute @name for task @p into a buffer
> * allocated by the security module and returned via @value. The
> * caller will free the returned buffer via kfree. The set of
> * attribute names is fixed by proc but the format of @value is up
> * to the security module authors except for the "context" attribute,
> * whose value is required to be a NUL-terminated printable ASCII
> * string without trailing whitespace.
> * @p the task whose attribute is being fetched
> * @name the name of the process attribute being fetched
> * @value set to point to the buffer containing the attribute value
> * Return the length of @value including the NUL on success, or -errno
> on error.
>
> The printable ASCII bit is based on what the dbus maintainer requested
> in previous discussions. The question of whether the terminating NUL
> should be included in the returned length was otherwise left ambiguous
> and inconsistent in your patch among the different security modules; if
> you prefer not including it in the length returned by the security
> modules, you'll need to adjust SELinux at least to not do so for "context".
BTW, I think the above guarantees with the exception of no trailing
whitespace and whether the NUL byte is included or excluded from length
are in reality also required for "current" (and SO_PEERSEC) or existing
userspace will break.
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Joe Perches @ 2020-02-03 19:29 UTC (permalink / raw)
To: Casey Schaufler, Shuah Khan, Mimi Zohar, jmorris, serge, mpe,
erichte, nayna, yuehaibing
Cc: linux-security-module, linux-kernel
In-Reply-To: <e175a880-637c-20d4-e0dc-bff31287e0dc@schaufler-ca.com>
On Mon, 2020-02-03 at 11:23 -0800, Casey Schaufler wrote:
> On 2/3/2020 11:02 AM, Joe Perches wrote:
> > On Mon, 2020-02-03 at 11:55 -0700, Shuah Khan wrote:
> > > On 2/3/20 6:21 AM, Mimi Zohar wrote:
> > > > On Wed, 2020-01-29 at 19:08 -0800, Joe Perches wrote:
> > > > > On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
> > > > > > Change messages to messages to make it easier to debug. The following
> > > > > > error message isn't informative enough to figure out what failed.
> > > > > > Change messages to include function information.
> > > > > >
> > > > > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > > > > > ---
> > > > > > .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
> > > > > > security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
> > > > > > 2 files changed, 18 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> > > > > perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > > so all the pr_<level> logging is more specific.
> > > > >
> > > > > This would prefix all pr_<level> output with "integrity: "
> > > Joe! Sorry for the delay in getting back to you.
> > >
> > > > Agreed. Joe, could you post a patch with a proper patch description
> > > > for this?
> > > >
> > > I have been looking into this a bit more and there is an opportunity
> > > here to add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt to integrity.h
> > > and get rid of duplicate defines.
> > That might work but:
> >
> > $ git grep --name-only 'integrity\.h' security | xargs grep pr_fmt
> > security/integrity/digsig.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > security/integrity/digsig_asymmetric.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > security/integrity/evm/evm_main.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > security/security.c:#define pr_fmt(fmt) "LSM: " fmt
> >
> > Here security.c already uses "LSM: "
> >
> > Does anyone care about the LSM: prefix?
>
> Yes.
No worries. My mistake it wouldn't change.
It was a bad grep as the integrity.h in security.c is #included
from the linux/include path and not the integrity subdirectory.
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Casey Schaufler @ 2020-02-03 19:23 UTC (permalink / raw)
To: Joe Perches, Shuah Khan, Mimi Zohar, jmorris, serge, mpe, erichte,
nayna, yuehaibing
Cc: linux-security-module, linux-kernel, Casey Schaufler
In-Reply-To: <94cf0aee8fd8b78718e252488458cfea105c0469.camel@perches.com>
On 2/3/2020 11:02 AM, Joe Perches wrote:
> On Mon, 2020-02-03 at 11:55 -0700, Shuah Khan wrote:
>> On 2/3/20 6:21 AM, Mimi Zohar wrote:
>>> On Wed, 2020-01-29 at 19:08 -0800, Joe Perches wrote:
>>>> On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
>>>>> Change messages to messages to make it easier to debug. The following
>>>>> error message isn't informative enough to figure out what failed.
>>>>> Change messages to include function information.
>>>>>
>>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>> ---
>>>>> .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
>>>>> security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
>>>>> 2 files changed, 18 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
>>>> perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> so all the pr_<level> logging is more specific.
>>>>
>>>> This would prefix all pr_<level> output with "integrity: "
>> Joe! Sorry for the delay in getting back to you.
>>
>>> Agreed. Joe, could you post a patch with a proper patch description
>>> for this?
>>>
>> I have been looking into this a bit more and there is an opportunity
>> here to add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt to integrity.h
>> and get rid of duplicate defines.
> That might work but:
>
> $ git grep --name-only 'integrity\.h' security | xargs grep pr_fmt
> security/integrity/digsig.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> security/integrity/digsig_asymmetric.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> security/integrity/evm/evm_main.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> security/security.c:#define pr_fmt(fmt) "LSM: " fmt
>
> Here security.c already uses "LSM: "
>
> Does anyone care about the LSM: prefix?
Yes.
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Shuah Khan @ 2020-02-03 19:20 UTC (permalink / raw)
To: Joe Perches, Mimi Zohar, jmorris, serge, mpe, erichte, nayna,
yuehaibing
Cc: linux-security-module, linux-kernel, Shuah Khan
In-Reply-To: <94cf0aee8fd8b78718e252488458cfea105c0469.camel@perches.com>
On 2/3/20 12:02 PM, Joe Perches wrote:
> On Mon, 2020-02-03 at 11:55 -0700, Shuah Khan wrote:
>> On 2/3/20 6:21 AM, Mimi Zohar wrote:
>>> On Wed, 2020-01-29 at 19:08 -0800, Joe Perches wrote:
>>>> On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
>>>>> Change messages to messages to make it easier to debug. The following
>>>>> error message isn't informative enough to figure out what failed.
>>>>> Change messages to include function information.
>>>>>
>>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>> ---
>>>>> .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
>>>>> security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
>>>>> 2 files changed, 18 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
>>>>
>>>> perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> so all the pr_<level> logging is more specific.
>>>>
>>>> This would prefix all pr_<level> output with "integrity: "
>>
>> Joe! Sorry for the delay in getting back to you.
>>
>>> Agreed. Joe, could you post a patch with a proper patch description
>>> for this?
>>>
>>
>> I have been looking into this a bit more and there is an opportunity
>> here to add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt to integrity.h
>> and get rid of duplicate defines.
>
> That might work but:
>
> $ git grep --name-only 'integrity\.h' security | xargs grep pr_fmt
> security/integrity/digsig.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> security/integrity/digsig_asymmetric.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> security/integrity/evm/evm_main.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> security/security.c:#define pr_fmt(fmt) "LSM: " fmt
>
> Here security.c already uses "LSM: "
>
> Does anyone care about the LSM: prefix?
>
>
What I have in mind is replace the ones under security/integrity/
adding the define to integrity.h is under security/integrity.
I would leave the security/security.c:#define pr_fmt(fmt) "LSM: " fmt
alone and just replace the ones under security/integrity/ in which case
KBUILD_MODNAME will show integrity as the module.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Joe Perches @ 2020-02-03 19:18 UTC (permalink / raw)
To: Nayna, Shuah Khan, jmorris, serge, mpe, zohar, erichte, nayna,
yuehaibing
Cc: linux-security-module, linux-kernel
In-Reply-To: <79d7b969-9b7d-7853-356b-876e444d3390@linux.vnet.ibm.com>
On Mon, 2020-02-03 at 10:15 -0500, Nayna wrote:
> On 1/29/20 10:08 PM, Joe Perches wrote:
> > On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
> > > Change messages to messages to make it easier to debug. The following
> > > error message isn't informative enough to figure out what failed.
> > > Change messages to include function information.
> > >
> > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > > ---
> > > .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
> > > security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
> > > 2 files changed, 18 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> > perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > so all the pr_<level> logging is more specific.
> >
> > This would prefix all pr_<level> output with "integrity: "
> >
> > 3integrity: Couldn't get size: 0x%lx
> > 3integrity: Error reading db var: 0x%lx
> > 3integrity: MODSIGN: Couldn't get UEFI db list
> > 3integrity: Couldn't parse db signatures: %d
> > 6integrity: Couldn't get UEFI MokListRT
> > 3integrity: Couldn't parse MokListRT signatures: %d
> > 6integrity: Couldn't get UEFI dbx list
> > 3integrity: Couldn't parse dbx signatures: %d
> >
> > 5integrity: Platform Keyring initialized
> > 6integrity: Error adding keys to platform keyring %s
> >
> > ---
> > security/integrity/platform_certs/load_powerpc.c | 3 +++
> > security/integrity/platform_certs/load_uefi.c | 2 ++
> > security/integrity/platform_certs/platform_keyring.c | 2 ++
> > 3 files changed, 7 insertions(+)
> >
> > diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> > index a2900c..5cfbd0 100644
> > --- a/security/integrity/platform_certs/load_powerpc.c
> > +++ b/security/integrity/platform_certs/load_powerpc.c
> > @@ -5,6 +5,9 @@
> > *
> > * - loads keys and hashes stored and controlled by the firmware.
> > */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
>
> Looks good. How about slight addition in it as below:
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": load_powerpc: " fmt
Perhaps another way to do that is to use:
---
security/integrity/integrity.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/security/integrity/integrity.h
b/security/integrity/integrity.h
index 543d277..b78469 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -6,6 +6,12 @@
* Mimi Zohar <zohar@us.ibm.com>
*/
+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " KBUILD_BASENAME ": " fmt
+
#include <linux/types.h>
#include <linux/integrity.h>
#include <crypto/sha.h>
That would create these string differences in security:
$ diff -urN old new
--- old 2020-02-03 11:11:46.403134346 -0800
+++ new 2020-02-03 11:12:15.407135326 -0800
@@ -43,76 +43,81 @@
2 48 8 14 xattr_data:271 80 68 8 data:306
2 48 8 9 entry:136 80 72 14 event_data:138
2 48 8 9 entry:305 80 72 14 event_data:306
-2ima: ahash calculation failed: err: %d
+2ima: ima_crypto: ahash calculation failed: err: %d
3 32 4 13 hash_algo:318 48 4 14 digestsize:320 64 8 10 digest:319
3 32 4 13 hash_algo:339 48 4 18 cur_digestsize:341 64 8 14 cur_digest:340
3 32 4 7 pcr:134 48 4 11 namelen:134 64 4 21 template_data_len:134
3 32 8 8 data:277 64 8 9 datap:278 96 8 8 size:279
3 48 4 19 datalen_to_hash:489 64 256 10 buffer:486 384 376 16 __shash_desc:474
3 48 8 8 lnum:973 80 8 8 rule:952 112 48 8 args:971
-3Couldn't get size: 0x%lx
-3Couldn't parse db signatures: %d
-3Couldn't parse dbx signatures: %d
-3Couldn't parse MokListRT signatures: %d
-3Error reading db var: 0x%lx
-3evm: Can not allocate %s (reason: %ld)
-3evm: HMAC key is not set
-3evm: key initialization failed
-3ima: attempting to initialize the template "%s" failed
-3ima: attempting to restore a incompatible measurement list
-3ima: attempting to restore an unsupported template "%s" failed
-3ima: attempting to restore a template name that is too long
-3ima: attempting to restore the template fmt "%s" failed
-3ima: attempting to restore too many measurements
-3ima: Can not allocate %s (reason: %d)
-3ima: Can not allocate %s (reason: %ld)
-3ima: Error Communicating to TPM chip
-3ima: Error Communicating to TPM chip, result: %d
-3ima: field '%s' not found
-3ima: format string '%s' contains too many fields
-3ima: format string '%s' not valid, using template %s
-3ima: impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.
-3ima: impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help
-3ima: Invalid field with length %d
-3ima: lsm rule update error %d
-3ima: OUT OF MEMORY ERROR creating queue entry
-3ima: Prevent firmware loading_store.
-3ima: Prevent firmware sysfs fallback loading.
-3ima: %s: buf end mismatch: expected: %p, current: %p
-3ima: signed policy file (specified as an absolute pathname) required
-3ima: %s: nr of fields mismatch: expected: %d, current: %d
-3ima: template does not support hash alg
-3ima: template %s init failed, result: %d
-3ima: template %s not found, using %s
-3ima: Unable to open file: %s (%d)
-3integrity: Key '%s' is in ima_blacklist_keyring
-3integrity: no %s keyring: %d
-3integrity: Problem loading X.509 certificate %d
-3integrity: Request for unknown key '%s' err %ld
-3integrity: Unable to open file: %s (%d)
-3MODSIGN: Couldn't get UEFI db list
-3Unable to create integrity sysfs dir: %d
+3evm: evm_crypto: Can not allocate %s (reason: %ld)
+3evm: evm_crypto: HMAC key is not set
+3evm: evm_crypto: key initialization failed
+3ima: ima_crypto: Can not allocate %s (reason: %d)
+3ima: ima_crypto: Can not allocate %s (reason: %ld)
+3ima: ima_crypto: Error Communicating to TPM chip
+3ima: ima_fs: signed policy file (specified as an absolute pathname) required
+3ima: ima_fs: Unable to open file: %s (%d)
+3ima: ima_main: impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.
+3ima: ima_main: impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help
+3ima: ima_main: Prevent firmware loading_store.
+3ima: ima_main: Prevent firmware sysfs fallback loading.
+3ima: ima_main: template %s init failed, result: %d
+3ima: ima_policy: lsm rule update error %d
+3ima: ima_queue: Error Communicating to TPM chip, result: %d
+3ima: ima_queue: OUT OF MEMORY ERROR creating queue entry
+3ima: ima_template: attempting to initialize the template "%s" failed
+3ima: ima_template: attempting to restore a incompatible measurement list
+3ima: ima_template: attempting to restore an unsupported template "%s" failed
+3ima: ima_template: attempting to restore a template name that is too long
+3ima: ima_template: attempting to restore the template fmt "%s" failed
+3ima: ima_template: attempting to restore too many measurements
+3ima: ima_template: field '%s' not found
+3ima: ima_template: format string '%s' contains too many fields
+3ima: ima_template: format string '%s' not valid, using template %s
+3ima: ima_template: Invalid field with length %d
+3ima: ima_template_lib: %s: buf end mismatch: expected: %p, current: %p
+3ima: ima_template_lib: %s: nr of fields mismatch: expected: %d, current: %d
+3ima: ima_template: template does not support hash alg
+3ima: ima_template: template %s init failed, result: %d
+3ima: ima_template: template %s not found, using %s
+3integrity: digsig_asymmetric: Key '%s' is in ima_blacklist_keyring
+3integrity: digsig_asymmetric: Request for unknown key '%s' err %ld
+3integrity: digsig: no %s keyring: %d
+3integrity: digsig: Problem loading X.509 certificate %d
+3integrity: digsig: Unable to open file: %s (%d)
+3integrity: iint: Unable to create integrity sysfs dir: %d
+3integrity: load_uefi: Couldn't get size: 0x%lx
+3integrity: load_uefi: Couldn't parse db signatures: %d
+3integrity: load_uefi: Couldn't parse dbx signatures: %d
+3integrity: load_uefi: Couldn't parse MokListRT signatures: %d
+3integrity: load_uefi: Error reading db var: 0x%lx
+3integrity: load_uefi: MODSIGN: Couldn't get UEFI db list
4 32 8 8 entry:46 64 72 13 event_data:48 176 24 7 hash:55 240 240 11 tmp_iint:47
4 48 16 8 rbuf:209 80 16 13 rbuf_size:214 112 32 6 sg:212 176 104 8 wait:213
4 48 8 8 bufp:358 80 8 12 hdr_mask:362 112 64 7 hdr:353 208 34 17 template_name:350
-4ima: Couldn't register LSM notifier, error %d
-4ima: rule for LSM '%s' is undefined
-4ima: Skipping unknown architecture policy rule: %s
-5ima: template with 'modsig' field also needs 'd-modsig' field
-5integrity: Loaded X.509 cert '%s'
-6evm: Error registering secfs
-6evm: HMAC attrs: 0x%x
-6evm: init_desc failed
-6evm: Initialising EVM extended attributes:
-6evm: key initialized
-6evm: %s
-6ima: Allocated hash algorithm: %s
-6ima: Allocating %s failed, going to use default hash algorithm %s
-6ima: No architecture policies found
-6ima: No TPM chip found, activating TPM-bypass!
-6ima: policy update %s
-6ima: Unable to reopen file for reading.
-6integrity: Can't allocate %s keyring (%d)
-6integrity: Loading X.509 certificate: %s
+4ima: ima_main: Couldn't register LSM notifier, error %d
+4ima: ima_policy: rule for LSM '%s' is undefined
+4ima: ima_policy: Skipping unknown architecture policy rule: %s
+5ima: ima_policy: template with 'modsig' field also needs 'd-modsig' field
+5integrity: digsig: Loaded X.509 cert '%s'
+5integrity: platform_keyring: Platform Keyring initialized
+6evm: evm_crypto: init_desc failed
+6evm: evm_crypto: key initialized
+6evm: evm_main: Error registering secfs
+6evm: evm_main: HMAC attrs: 0x%x
+6evm: evm_main: Initialising EVM extended attributes:
+6evm: evm_main: %s
+6ima: ima_crypto: Allocated hash algorithm: %s
+6ima: ima_crypto: Unable to reopen file for reading.
+6ima: ima_fs: policy update %s
+6ima: ima_init: No TPM chip found, activating TPM-bypass!
+6ima: ima_main: Allocating %s failed, going to use default hash algorithm %s
+6ima: ima_policy: No architecture policies found
+6integrity: digsig: Can't allocate %s keyring (%d)
+6integrity: digsig: Loading X.509 certificate: %s
+6integrity: load_uefi: Couldn't get UEFI dbx list
+6integrity: load_uefi: Couldn't get UEFI MokListRT
+6integrity: platform_keyring: Error adding keys to platform keyring %s
7 48 4 7 pcr:203 64 8 17 template_desc:198 96 8 11 pathbuf:199 128 8 12 pathname:201 160 8 15 xattr_value:204 192 8 10 modsig:205 224 255 12 filename:200
7 48 4 9 secid:706 64 4 7 pcr:690 80 8 9 entry:693 112 8 12 template:699 144 72 14 event_data:695 256 68 8 hash:703 368 240 8 iint:694
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Joe Perches @ 2020-02-03 19:02 UTC (permalink / raw)
To: Shuah Khan, Mimi Zohar, jmorris, serge, mpe, erichte, nayna,
yuehaibing
Cc: linux-security-module, linux-kernel
In-Reply-To: <475ab05c-300b-fdbe-5de0-6fce8d1a360d@linuxfoundation.org>
On Mon, 2020-02-03 at 11:55 -0700, Shuah Khan wrote:
> On 2/3/20 6:21 AM, Mimi Zohar wrote:
> > On Wed, 2020-01-29 at 19:08 -0800, Joe Perches wrote:
> > > On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
> > > > Change messages to messages to make it easier to debug. The following
> > > > error message isn't informative enough to figure out what failed.
> > > > Change messages to include function information.
> > > >
> > > > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > > > ---
> > > > .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
> > > > security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
> > > > 2 files changed, 18 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> > >
> > > perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > so all the pr_<level> logging is more specific.
> > >
> > > This would prefix all pr_<level> output with "integrity: "
>
> Joe! Sorry for the delay in getting back to you.
>
> > Agreed. Joe, could you post a patch with a proper patch description
> > for this?
> >
>
> I have been looking into this a bit more and there is an opportunity
> here to add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt to integrity.h
> and get rid of duplicate defines.
That might work but:
$ git grep --name-only 'integrity\.h' security | xargs grep pr_fmt
security/integrity/digsig.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
security/integrity/digsig_asymmetric.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
security/integrity/evm/evm_main.c:#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
security/security.c:#define pr_fmt(fmt) "LSM: " fmt
Here security.c already uses "LSM: "
Does anyone care about the LSM: prefix?
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Shuah Khan @ 2020-02-03 18:55 UTC (permalink / raw)
To: Mimi Zohar, Joe Perches, jmorris, serge, mpe, erichte, nayna,
yuehaibing
Cc: linux-security-module, linux-kernel, Shuah Khan
In-Reply-To: <1580736077.5585.4.camel@linux.ibm.com>
On 2/3/20 6:21 AM, Mimi Zohar wrote:
> On Wed, 2020-01-29 at 19:08 -0800, Joe Perches wrote:
>> On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
>>> Change messages to messages to make it easier to debug. The following
>>> error message isn't informative enough to figure out what failed.
>>> Change messages to include function information.
>>>
>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>> ---
>>> .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
>>> security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
>>> 2 files changed, 18 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
>>
>> perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> so all the pr_<level> logging is more specific.
>>
>> This would prefix all pr_<level> output with "integrity: "
>
Joe! Sorry for the delay in getting back to you.
> Agreed. Joe, could you post a patch with a proper patch description
> for this?
>
I have been looking into this a bit more and there is an opportunity
here to add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt to integrity.h
and get rid of duplicate defines.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Stephen Smalley @ 2020-02-03 18:54 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <446935fa-2926-c346-a273-ae1ecbb072cd@schaufler-ca.com>
On 1/31/20 5:10 PM, Casey Schaufler wrote:
> From c4085435215653b7c4d07a35a9df308120441d79 Mon Sep 17 00:00:00 2001
> From: Casey Schaufler <casey@schaufler-ca.com>
> Date: Fri, 31 Jan 2020 13:57:23 -0800
> Subject: [PATCH v14] LSM: Move "context" format enforcement into security
> modules
>
> Document in lsm_hooks.h what is expected of a security module that
> supplies the "context" attribute. Add handling of the "context"
> attribute to SELinux, Smack and AppArmor security modules. The
> AppArmor implementation provides a different string for "context"
> than it does for other attributes to conform with the "context"
> format.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 6 ++++++
> security/apparmor/include/procattr.h | 2 +-
> security/apparmor/lsm.c | 8 ++++++--
> security/apparmor/procattr.c | 11 +++++++----
> security/security.c | 2 +-
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> 7 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 2bf82e1cf347..61977a33f2c3 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1321,6 +1321,12 @@
> * @pages contains the number of pages.
> * Return 0 if permission is granted.
> *
> + * @getprocattr:
> + * Provide the named process attribute for display in special files in
> + * the /proc/.../attr directory. Attribute naming and the data displayed
> + * is at the discretion of the security modules. The exception is the
> + * "context" attribute, which will contain the security context of the
> + * task as a nul terminated text string without trailing whitespace.
I'd suggest something like the following instead:
* @getprocattr
* Get the value of process attribute @name for task @p into a buffer
* allocated by the security module and returned via @value. The
* caller will free the returned buffer via kfree. The set of
* attribute names is fixed by proc but the format of @value is up
* to the security module authors except for the "context" attribute,
* whose value is required to be a NUL-terminated printable ASCII
* string without trailing whitespace.
* @p the task whose attribute is being fetched
* @name the name of the process attribute being fetched
* @value set to point to the buffer containing the attribute value
* Return the length of @value including the NUL on success, or -errno
on error.
The printable ASCII bit is based on what the dbus maintainer requested
in previous discussions. The question of whether the terminating NUL
should be included in the returned length was otherwise left ambiguous
and inconsistent in your patch among the different security modules; if
you prefer not including it in the length returned by the security
modules, you'll need to adjust SELinux at least to not do so for "context".
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Nayna @ 2020-02-03 15:15 UTC (permalink / raw)
To: Joe Perches, Shuah Khan, jmorris, serge, mpe, zohar, erichte,
nayna, yuehaibing
Cc: linux-security-module, linux-kernel
In-Reply-To: <ab2e19123cc15e3f8039b0d36e6ebae385db700e.camel@perches.com>
On 1/29/20 10:08 PM, Joe Perches wrote:
> On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
>> Change messages to messages to make it easier to debug. The following
>> error message isn't informative enough to figure out what failed.
>> Change messages to include function information.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>> .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
>> security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
>> 2 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> so all the pr_<level> logging is more specific.
>
> This would prefix all pr_<level> output with "integrity: "
>
> 3integrity: Couldn't get size: 0x%lx
> 3integrity: Error reading db var: 0x%lx
> 3integrity: MODSIGN: Couldn't get UEFI db list
> 3integrity: Couldn't parse db signatures: %d
> 6integrity: Couldn't get UEFI MokListRT
> 3integrity: Couldn't parse MokListRT signatures: %d
> 6integrity: Couldn't get UEFI dbx list
> 3integrity: Couldn't parse dbx signatures: %d
>
> 5integrity: Platform Keyring initialized
> 6integrity: Error adding keys to platform keyring %s
>
> ---
> security/integrity/platform_certs/load_powerpc.c | 3 +++
> security/integrity/platform_certs/load_uefi.c | 2 ++
> security/integrity/platform_certs/platform_keyring.c | 2 ++
> 3 files changed, 7 insertions(+)
>
> diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
> index a2900c..5cfbd0 100644
> --- a/security/integrity/platform_certs/load_powerpc.c
> +++ b/security/integrity/platform_certs/load_powerpc.c
> @@ -5,6 +5,9 @@
> *
> * - loads keys and hashes stored and controlled by the firmware.
> */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
Looks good. How about slight addition in it as below:
#define pr_fmt(fmt) KBUILD_MODNAME ": load_powerpc: " fmt
> #include <linux/kernel.h>
> #include <linux/sched.h>
> #include <linux/cred.h>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index 111898a..480450a 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Similarly...
#define pr_fmt(fmt) KBUILD_MODNAME ": load_uefi: " fmt
> +
> #include <linux/kernel.h>
> #include <linux/sched.h>
> #include <linux/cred.h>
> diff --git a/security/integrity/platform_certs/platform_keyring.c b/security/integrity/platform_certs/platform_keyring.c
> index 7646e35..9bd2846 100644
> --- a/security/integrity/platform_certs/platform_keyring.c
> +++ b/security/integrity/platform_certs/platform_keyring.c
> @@ -6,6 +6,8 @@
> * Author(s): Nayna Jain <nayna@linux.ibm.com>
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Same here...
#define pr_fmt(fmt) KBUILD_MODNAME ": platform_keyring: " fmt
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: SELinux: How to split permissions for keys?
From: Stephen Smalley @ 2020-02-03 14:48 UTC (permalink / raw)
To: Richard Haines, David Howells, paul
Cc: keyrings, selinux, linux-security-module, linux-kernel
In-Reply-To: <de8d82ba7db7abdf2a72d88c8cbc590e289f5f6f.camel@btinternet.com>
On 2/3/20 9:03 AM, Richard Haines wrote:
> On Mon, 2020-02-03 at 08:13 -0500, Stephen Smalley wrote:
>> Was that kernel patch ever posted to selinux list and/or the selinux
>> kernel maintainers? I don't recall seeing it. If not, please send
>> it
>> to the selinux list for review; at least one selinux maintainer
>> should
>> ack it before it gets accepted into any other tree.
>>
>>
>
> Not formally. I did post it in a discussion about keys in [2]. Since
> then it's been modified to support the split permissions.
Yes, that doesn't count since a) it wasn't the final version of the
patch which changed significantly afterward and b) even it had been the
final version, there was no acked-by or reviewed-by from a selinux
maintainer, just some suggestions. A non-trivial patch that modifies
security/selinux needs to be at least acked by a selinux maintainer and
often should go through the upstream selinux maintainer (Paul).
> I've extracted the patch from [1] and will post that to list for
> comments.
Thanks.
> [2]
> https://lore.kernel.org/selinux/35455b30b5185780628e92c98ec8191c70f39bde.camel@btinternet.com/
^ permalink raw reply
* Re: SELinux: How to split permissions for keys?
From: Richard Haines @ 2020-02-03 14:03 UTC (permalink / raw)
To: Stephen Smalley, David Howells, paul
Cc: keyrings, selinux, linux-security-module, linux-kernel
In-Reply-To: <3d1923ec-f02b-6be7-b0c0-d3d6f539b034@tycho.nsa.gov>
On Mon, 2020-02-03 at 08:13 -0500, Stephen Smalley wrote:
> On 2/2/20 2:30 PM, Richard Haines wrote:
> > On Thu, 2020-01-23 at 15:35 -0500, Stephen Smalley wrote:
> > > On 1/23/20 10:46 AM, Stephen Smalley wrote:
> > > > On 1/23/20 10:12 AM, David Howells wrote:
> > > > > Hi Stephen,
> > > > >
> > > > > I have patches to split the permissions that are used for
> > > > > keys to
> > > > > make
> > > > > them a
> > > > > bit finer grained and easier to use - and also to move to
> > > > > ACLs
> > > > > rather
> > > > > than
> > > > > fixed masks. See patch "keys: Replace uid/gid/perm
> > > > > permissions
> > > > > checking with
> > > > > an ACL" here:
> > > > >
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-acl
> > > > >
> > > > >
> > > > >
> > > > > However, I may not have managed the permission mask
> > > > > transformation inside
> > > > > SELinux correctly. Could you lend an eyeball? The change to
> > > > > the
> > > > > permissions
> > > > > model is as follows:
> > > > >
> > > > > The SETATTR permission is split to create two new
> > > > > permissions:
> > > > > (1) SET_SECURITY - which allows the key's owner, group
> > > > > and
> > > > > ACL
> > > > > to be
> > > > > changed and a restriction to be placed on a
> > > > > keyring.
> > > > > (2) REVOKE - which allows a key to be revoked.
> > > > > The SEARCH permission is split to create:
> > > > > (1) SEARCH - which allows a keyring to be search and a
> > > > > key
> > > > > to be
> > > > > found.
> > > > > (2) JOIN - which allows a keyring to be joined as a
> > > > > session
> > > > > keyring.
> > > > > (3) INVAL - which allows a key to be invalidated.
> > > > > The WRITE permission is also split to create:
> > > > > (1) WRITE - which allows a key's content to be altered
> > > > > and
> > > > > links
> > > > > to be
> > > > > added, removed and replaced in a keyring.
> > > > > (2) CLEAR - which allows a keyring to be cleared
> > > > > completely.
> > > > > This is
> > > > > split out to make it possible to give just this to
> > > > > an
> > > > > administrator.
> > > > > (3) REVOKE - see above.
> > > > >
> > > > > The change to SELinux is attached below.
> > > > >
> > > > > Should the split be pushed down into the SELinux policy
> > > > > rather
> > > > > than
> > > > > trying to
> > > > > calculate it?
> > > >
> > > > My understanding is that you must provide full backward
> > > > compatibility
> > > > with existing policies; hence, you must ensure that you always
> > > > check the
> > > > same SELinux permission(s) for the same operation when using an
> > > > existing
> > > > policy.
> > > >
> > > > In order to support finer-grained distinctions in SELinux with
> > > > future
> > > > policies, you can define a new SELinux policy capability along
> > > > with
> > > > the
> > > > new permissions, and if the policy capability is enabled in the
> > > > policy,
> > > > check the new permissions rather than the old ones. A recent
> > > > example of
> > > > adding a new policy capability and using it can be seen in:
> > > > https://lore.kernel.org/selinux/20200116194530.8696-1-jeffv@google.com/T/#u
> > > > although that patch was rejected for other reasons.
> > > >
> > > > Another example was when we introduced fine-grained
> > > > distinctions
> > > > for all
> > > > network address families, commit
> > > > da69a5306ab92e07224da54aafee8b1dccf024f6.
> > > >
> > > > The new policy capability also needs to be defined in libsepol
> > > > for
> > > > use
> > > > by the policy compiler; an example can be seen in:
> > > > https://lore.kernel.org/selinux/20170714164801.6346-1-sds@tycho.nsa.gov/
> > > >
> > > > Then future policies can declare the policy capability when
> > > > they
> > > > are
> > > > ready to start using the new permissions instead of the old.
> > > >
> > > > > Thanks,
> > > > > David
> > > > > ---
> > > > > diff --git a/security/selinux/hooks.c
> > > > > b/security/selinux/hooks.c
> > > > > index 116b4d644f68..c8db5235b01f 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -6556,6 +6556,7 @@ static int
> > > > > selinux_key_permission(key_ref_t
> > > > > key_ref,
> > > > > {
> > > > > struct key *key;
> > > > > struct key_security_struct *ksec;
> > > > > + unsigned oldstyle_perm;
> > > > > u32 sid;
> > > > > /* if no specific permissions are requested, we skip
> > > > > the
> > > > > @@ -6564,13 +6565,26 @@ static int
> > > > > selinux_key_permission(key_ref_t
> > > > > key_ref,
> > > > > if (perm == 0)
> > > > > return 0;
> > > > > + oldstyle_perm = perm & (KEY_NEED_VIEW | KEY_NEED_READ |
> > > > > KEY_NEED_WRITE |
> > > > > + KEY_NEED_SEARCH | KEY_NEED_LINK);
> > > > > + if (perm & KEY_NEED_SETSEC)
> > > > > + oldstyle_perm |= OLD_KEY_NEED_SETATTR;
> > > > > + if (perm & KEY_NEED_INVAL)
> > > > > + oldstyle_perm |= KEY_NEED_SEARCH;
> > > > > + if (perm & KEY_NEED_REVOKE && !(perm &
> > > > > OLD_KEY_NEED_SETATTR))
> > > > > + oldstyle_perm |= KEY_NEED_WRITE;
> > > > > + if (perm & KEY_NEED_JOIN)
> > > > > + oldstyle_perm |= KEY_NEED_SEARCH;
> > > > > + if (perm & KEY_NEED_CLEAR)
> > > > > + oldstyle_perm |= KEY_NEED_WRITE;
> > > > > +
> > > >
> > > > I don't know offhand if this ensures that the same SELinux
> > > > permission is
> > > > always checked as it would have been previously for the same
> > > > operation+arguments. That's what you have to preserve for
> > > > existing
> > > > policies.
> > >
> > > As Richard pointed out in his email, your key-acl series replaces
> > > two
> > > different old permissions (LINK, SEARCH) with a single permission
> > > (JOIN)
> > > in different callers, so by the time we reach the SELinux hook we
> > > cannot
> > > map it back unambiguously and provide full backward
> > > compatibility. The
> > > REVOKE case also seems fragile although there you seem to
> > > distinguish
> > > by
> > > sometimes passing in OLD_KEY_NEED_SETATTR and sometimes
> > > not? You'll
> > > have to fix the JOIN case to avoid userspace breakage.
> > >
> > > You may want to go ahead and explicitly translate all of the
> > > KEY_NEED
> > > permissions to SELinux permissions rather than passing the key
> > > permissions directly here to avoid requiring that the values
> > > always
> > > match. The SELinux permission symbols are of the form
> > > CLASS__PERMISSION
> > > (NB double underscore), e.g. KEY__SETATTR, generated
> > > automatically
> > > from
> > > the security/selinux/include/classmap.h tables to the
> > > security/selinux/av_permissions.h generated header. Most hooks
> > > perform
> > > such translation, e.g. file_mask_to_av(). You will almost
> > > certainly
> > > need to do this if/when you introduce support for the new
> > > permissions
> > > to
> > > SELinux.
> >
> > This problem has now been fixed in [1].
> > It passes the current selinux-test-suite (except test/fs_filesystem
> > regression).
> >
> > As the fix now includes a new 'key_perms' policy capability to
> > allow
> > use of the extended key permissions, I've updated libsepol and the
> > selinux-testsuite test/keys to test these.
> >
> > I'll submit two RFC patches that will allow [1] to be tested with
> > 'key_perms' true or false.
> >
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next
>
> Was that kernel patch ever posted to selinux list and/or the selinux
> kernel maintainers? I don't recall seeing it. If not, please send
> it
> to the selinux list for review; at least one selinux maintainer
> should
> ack it before it gets accepted into any other tree.
>
>
Not formally. I did post it in a discussion about keys in [2]. Since
then it's been modified to support the split permissions.
I've extracted the patch from [1] and will post that to list for
comments.
[2]
https://lore.kernel.org/selinux/35455b30b5185780628e92c98ec8191c70f39bde.camel@btinternet.com/
>
^ permalink raw reply
* Re: [PATCH] security/integrity: Include __func__ in messages for easier debug
From: Mimi Zohar @ 2020-02-03 13:21 UTC (permalink / raw)
To: Joe Perches, Shuah Khan, jmorris, serge, mpe, erichte, nayna,
yuehaibing
Cc: linux-security-module, linux-kernel
In-Reply-To: <ab2e19123cc15e3f8039b0d36e6ebae385db700e.camel@perches.com>
On Wed, 2020-01-29 at 19:08 -0800, Joe Perches wrote:
> On Wed, 2020-01-29 at 19:01 -0700, Shuah Khan wrote:
> > Change messages to messages to make it easier to debug. The following
> > error message isn't informative enough to figure out what failed.
> > Change messages to include function information.
> >
> > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> > ---
> > .../integrity/platform_certs/load_powerpc.c | 14 ++++++++------
> > security/integrity/platform_certs/load_uefi.c | 17 ++++++++++-------
> > 2 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/security/integrity/platform_certs/load_powerpc.c b/security/integrity/platform_certs/load_powerpc.c
>
> perhaps instead add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> so all the pr_<level> logging is more specific.
>
> This would prefix all pr_<level> output with "integrity: "
Agreed. Joe, could you post a patch with a proper patch description
for this?
thanks,
Mimi
^ permalink raw reply
* Re: SELinux: How to split permissions for keys?
From: Stephen Smalley @ 2020-02-03 13:13 UTC (permalink / raw)
To: Richard Haines, David Howells, paul
Cc: keyrings, selinux, linux-security-module, linux-kernel
In-Reply-To: <459818a9ad1c808298bf3d7c9bcb130323d30e97.camel@btinternet.com>
On 2/2/20 2:30 PM, Richard Haines wrote:
> On Thu, 2020-01-23 at 15:35 -0500, Stephen Smalley wrote:
>> On 1/23/20 10:46 AM, Stephen Smalley wrote:
>>> On 1/23/20 10:12 AM, David Howells wrote:
>>>> Hi Stephen,
>>>>
>>>> I have patches to split the permissions that are used for keys to
>>>> make
>>>> them a
>>>> bit finer grained and easier to use - and also to move to ACLs
>>>> rather
>>>> than
>>>> fixed masks. See patch "keys: Replace uid/gid/perm permissions
>>>> checking with
>>>> an ACL" here:
>>>>
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-acl
>>>>
>>>>
>>>>
>>>> However, I may not have managed the permission mask
>>>> transformation inside
>>>> SELinux correctly. Could you lend an eyeball? The change to
>>>> the
>>>> permissions
>>>> model is as follows:
>>>>
>>>> The SETATTR permission is split to create two new
>>>> permissions:
>>>> (1) SET_SECURITY - which allows the key's owner, group and
>>>> ACL
>>>> to be
>>>> changed and a restriction to be placed on a keyring.
>>>> (2) REVOKE - which allows a key to be revoked.
>>>> The SEARCH permission is split to create:
>>>> (1) SEARCH - which allows a keyring to be search and a key
>>>> to be
>>>> found.
>>>> (2) JOIN - which allows a keyring to be joined as a
>>>> session
>>>> keyring.
>>>> (3) INVAL - which allows a key to be invalidated.
>>>> The WRITE permission is also split to create:
>>>> (1) WRITE - which allows a key's content to be altered and
>>>> links
>>>> to be
>>>> added, removed and replaced in a keyring.
>>>> (2) CLEAR - which allows a keyring to be cleared
>>>> completely.
>>>> This is
>>>> split out to make it possible to give just this to an
>>>> administrator.
>>>> (3) REVOKE - see above.
>>>>
>>>> The change to SELinux is attached below.
>>>>
>>>> Should the split be pushed down into the SELinux policy rather
>>>> than
>>>> trying to
>>>> calculate it?
>>>
>>> My understanding is that you must provide full backward
>>> compatibility
>>> with existing policies; hence, you must ensure that you always
>>> check the
>>> same SELinux permission(s) for the same operation when using an
>>> existing
>>> policy.
>>>
>>> In order to support finer-grained distinctions in SELinux with
>>> future
>>> policies, you can define a new SELinux policy capability along with
>>> the
>>> new permissions, and if the policy capability is enabled in the
>>> policy,
>>> check the new permissions rather than the old ones. A recent
>>> example of
>>> adding a new policy capability and using it can be seen in:
>>> https://lore.kernel.org/selinux/20200116194530.8696-1-jeffv@google.com/T/#u
>>> although that patch was rejected for other reasons.
>>>
>>> Another example was when we introduced fine-grained distinctions
>>> for all
>>> network address families, commit
>>> da69a5306ab92e07224da54aafee8b1dccf024f6.
>>>
>>> The new policy capability also needs to be defined in libsepol for
>>> use
>>> by the policy compiler; an example can be seen in:
>>> https://lore.kernel.org/selinux/20170714164801.6346-1-sds@tycho.nsa.gov/
>>>
>>> Then future policies can declare the policy capability when they
>>> are
>>> ready to start using the new permissions instead of the old.
>>>
>>>> Thanks,
>>>> David
>>>> ---
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 116b4d644f68..c8db5235b01f 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -6556,6 +6556,7 @@ static int
>>>> selinux_key_permission(key_ref_t
>>>> key_ref,
>>>> {
>>>> struct key *key;
>>>> struct key_security_struct *ksec;
>>>> + unsigned oldstyle_perm;
>>>> u32 sid;
>>>> /* if no specific permissions are requested, we skip the
>>>> @@ -6564,13 +6565,26 @@ static int
>>>> selinux_key_permission(key_ref_t
>>>> key_ref,
>>>> if (perm == 0)
>>>> return 0;
>>>> + oldstyle_perm = perm & (KEY_NEED_VIEW | KEY_NEED_READ |
>>>> KEY_NEED_WRITE |
>>>> + KEY_NEED_SEARCH | KEY_NEED_LINK);
>>>> + if (perm & KEY_NEED_SETSEC)
>>>> + oldstyle_perm |= OLD_KEY_NEED_SETATTR;
>>>> + if (perm & KEY_NEED_INVAL)
>>>> + oldstyle_perm |= KEY_NEED_SEARCH;
>>>> + if (perm & KEY_NEED_REVOKE && !(perm &
>>>> OLD_KEY_NEED_SETATTR))
>>>> + oldstyle_perm |= KEY_NEED_WRITE;
>>>> + if (perm & KEY_NEED_JOIN)
>>>> + oldstyle_perm |= KEY_NEED_SEARCH;
>>>> + if (perm & KEY_NEED_CLEAR)
>>>> + oldstyle_perm |= KEY_NEED_WRITE;
>>>> +
>>>
>>> I don't know offhand if this ensures that the same SELinux
>>> permission is
>>> always checked as it would have been previously for the same
>>> operation+arguments. That's what you have to preserve for
>>> existing
>>> policies.
>>
>> As Richard pointed out in his email, your key-acl series replaces
>> two
>> different old permissions (LINK, SEARCH) with a single permission
>> (JOIN)
>> in different callers, so by the time we reach the SELinux hook we
>> cannot
>> map it back unambiguously and provide full backward
>> compatibility. The
>> REVOKE case also seems fragile although there you seem to distinguish
>> by
>> sometimes passing in OLD_KEY_NEED_SETATTR and sometimes not? You'll
>> have to fix the JOIN case to avoid userspace breakage.
>>
>> You may want to go ahead and explicitly translate all of the
>> KEY_NEED
>> permissions to SELinux permissions rather than passing the key
>> permissions directly here to avoid requiring that the values always
>> match. The SELinux permission symbols are of the form
>> CLASS__PERMISSION
>> (NB double underscore), e.g. KEY__SETATTR, generated automatically
>> from
>> the security/selinux/include/classmap.h tables to the
>> security/selinux/av_permissions.h generated header. Most hooks
>> perform
>> such translation, e.g. file_mask_to_av(). You will almost certainly
>> need to do this if/when you introduce support for the new permissions
>> to
>> SELinux.
>
>
> This problem has now been fixed in [1].
> It passes the current selinux-test-suite (except test/fs_filesystem
> regression).
>
> As the fix now includes a new 'key_perms' policy capability to allow
> use of the extended key permissions, I've updated libsepol and the
> selinux-testsuite test/keys to test these.
>
> I'll submit two RFC patches that will allow [1] to be tested with
> 'key_perms' true or false.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next
Was that kernel patch ever posted to selinux list and/or the selinux
kernel maintainers? I don't recall seeing it. If not, please send it
to the selinux list for review; at least one selinux maintainer should
ack it before it gets accepted into any other tree.
^ permalink raw reply
* Re: SELinux: How to split permissions for keys?
From: Richard Haines @ 2020-02-02 19:30 UTC (permalink / raw)
To: Stephen Smalley, David Howells, paul
Cc: keyrings, selinux, linux-security-module, linux-kernel
In-Reply-To: <50f98f04-d00e-ae54-9a90-d0ff10be515a@tycho.nsa.gov>
On Thu, 2020-01-23 at 15:35 -0500, Stephen Smalley wrote:
> On 1/23/20 10:46 AM, Stephen Smalley wrote:
> > On 1/23/20 10:12 AM, David Howells wrote:
> > > Hi Stephen,
> > >
> > > I have patches to split the permissions that are used for keys to
> > > make
> > > them a
> > > bit finer grained and easier to use - and also to move to ACLs
> > > rather
> > > than
> > > fixed masks. See patch "keys: Replace uid/gid/perm permissions
> > > checking with
> > > an ACL" here:
> > >
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-acl
> > >
> > >
> > >
> > > However, I may not have managed the permission mask
> > > transformation inside
> > > SELinux correctly. Could you lend an eyeball? The change to
> > > the
> > > permissions
> > > model is as follows:
> > >
> > > The SETATTR permission is split to create two new
> > > permissions:
> > > (1) SET_SECURITY - which allows the key's owner, group and
> > > ACL
> > > to be
> > > changed and a restriction to be placed on a keyring.
> > > (2) REVOKE - which allows a key to be revoked.
> > > The SEARCH permission is split to create:
> > > (1) SEARCH - which allows a keyring to be search and a key
> > > to be
> > > found.
> > > (2) JOIN - which allows a keyring to be joined as a
> > > session
> > > keyring.
> > > (3) INVAL - which allows a key to be invalidated.
> > > The WRITE permission is also split to create:
> > > (1) WRITE - which allows a key's content to be altered and
> > > links
> > > to be
> > > added, removed and replaced in a keyring.
> > > (2) CLEAR - which allows a keyring to be cleared
> > > completely.
> > > This is
> > > split out to make it possible to give just this to an
> > > administrator.
> > > (3) REVOKE - see above.
> > >
> > > The change to SELinux is attached below.
> > >
> > > Should the split be pushed down into the SELinux policy rather
> > > than
> > > trying to
> > > calculate it?
> >
> > My understanding is that you must provide full backward
> > compatibility
> > with existing policies; hence, you must ensure that you always
> > check the
> > same SELinux permission(s) for the same operation when using an
> > existing
> > policy.
> >
> > In order to support finer-grained distinctions in SELinux with
> > future
> > policies, you can define a new SELinux policy capability along with
> > the
> > new permissions, and if the policy capability is enabled in the
> > policy,
> > check the new permissions rather than the old ones. A recent
> > example of
> > adding a new policy capability and using it can be seen in:
> > https://lore.kernel.org/selinux/20200116194530.8696-1-jeffv@google.com/T/#u
> > although that patch was rejected for other reasons.
> >
> > Another example was when we introduced fine-grained distinctions
> > for all
> > network address families, commit
> > da69a5306ab92e07224da54aafee8b1dccf024f6.
> >
> > The new policy capability also needs to be defined in libsepol for
> > use
> > by the policy compiler; an example can be seen in:
> > https://lore.kernel.org/selinux/20170714164801.6346-1-sds@tycho.nsa.gov/
> >
> > Then future policies can declare the policy capability when they
> > are
> > ready to start using the new permissions instead of the old.
> >
> > > Thanks,
> > > David
> > > ---
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 116b4d644f68..c8db5235b01f 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -6556,6 +6556,7 @@ static int
> > > selinux_key_permission(key_ref_t
> > > key_ref,
> > > {
> > > struct key *key;
> > > struct key_security_struct *ksec;
> > > + unsigned oldstyle_perm;
> > > u32 sid;
> > > /* if no specific permissions are requested, we skip the
> > > @@ -6564,13 +6565,26 @@ static int
> > > selinux_key_permission(key_ref_t
> > > key_ref,
> > > if (perm == 0)
> > > return 0;
> > > + oldstyle_perm = perm & (KEY_NEED_VIEW | KEY_NEED_READ |
> > > KEY_NEED_WRITE |
> > > + KEY_NEED_SEARCH | KEY_NEED_LINK);
> > > + if (perm & KEY_NEED_SETSEC)
> > > + oldstyle_perm |= OLD_KEY_NEED_SETATTR;
> > > + if (perm & KEY_NEED_INVAL)
> > > + oldstyle_perm |= KEY_NEED_SEARCH;
> > > + if (perm & KEY_NEED_REVOKE && !(perm &
> > > OLD_KEY_NEED_SETATTR))
> > > + oldstyle_perm |= KEY_NEED_WRITE;
> > > + if (perm & KEY_NEED_JOIN)
> > > + oldstyle_perm |= KEY_NEED_SEARCH;
> > > + if (perm & KEY_NEED_CLEAR)
> > > + oldstyle_perm |= KEY_NEED_WRITE;
> > > +
> >
> > I don't know offhand if this ensures that the same SELinux
> > permission is
> > always checked as it would have been previously for the same
> > operation+arguments. That's what you have to preserve for
> > existing
> > policies.
>
> As Richard pointed out in his email, your key-acl series replaces
> two
> different old permissions (LINK, SEARCH) with a single permission
> (JOIN)
> in different callers, so by the time we reach the SELinux hook we
> cannot
> map it back unambiguously and provide full backward
> compatibility. The
> REVOKE case also seems fragile although there you seem to distinguish
> by
> sometimes passing in OLD_KEY_NEED_SETATTR and sometimes not? You'll
> have to fix the JOIN case to avoid userspace breakage.
>
> You may want to go ahead and explicitly translate all of the
> KEY_NEED
> permissions to SELinux permissions rather than passing the key
> permissions directly here to avoid requiring that the values always
> match. The SELinux permission symbols are of the form
> CLASS__PERMISSION
> (NB double underscore), e.g. KEY__SETATTR, generated automatically
> from
> the security/selinux/include/classmap.h tables to the
> security/selinux/av_permissions.h generated header. Most hooks
> perform
> such translation, e.g. file_mask_to_av(). You will almost certainly
> need to do this if/when you introduce support for the new permissions
> to
> SELinux.
This problem has now been fixed in [1].
It passes the current selinux-test-suite (except test/fs_filesystem
regression).
As the fix now includes a new 'key_perms' policy capability to allow
use of the extended key permissions, I've updated libsepol and the
selinux-testsuite test/keys to test these.
I'll submit two RFC patches that will allow [1] to be tested with
'key_perms' true or false.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next
^ permalink raw reply
* Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to HASH_ALGO__LAST
From: Jarkko Sakkinen @ 2020-02-01 17:10 UTC (permalink / raw)
To: Mimi Zohar
Cc: Roberto Sassu, james.bottomley@hansenpartnership.com,
linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu
In-Reply-To: <1580477590.6104.61.camel@linux.ibm.com>
On Fri, Jan 31, 2020 at 08:33:10AM -0500, Mimi Zohar wrote:
> On Thu, 2020-01-30 at 16:11 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > > Sent: Thursday, January 30, 2020 9:48 AM
> > > To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> > > james.bottomley@hansenpartnership.com; linux-integrity@vger.kernel.org
> > > Cc: linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>
> > > Subject: Re: [PATCH 1/8] tpm: initialize crypto_id of allocated_banks to
> > > HASH_ALGO__LAST
> > >
> > > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote:
> > > > chip->allocated_banks contains the list of TPM algorithm IDs of allocated
> > > > PCR banks. It also contains the corresponding ID of the crypto subsystem,
> > > > so that users of the TPM driver can calculate a digest for a PCR extend
> > > > operation.
> > > >
> > > > However, if there is no mapping between TPM algorithm ID and crypto ID,
> > > the
> > > > crypto_id field in chip->allocated_banks remains set to zero (the array is
> > > > allocated and initialized with kcalloc() in tpm2_get_pcr_allocation()).
> > > > Zero should not be used as value for unknown mappings, as it is a valid
> > > > crypto ID (HASH_ALGO_MD4).
> > > >
> > > > This patch initializes crypto_id to HASH_ALGO__LAST.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>---
> > >
> > > Remarks:
> > >
> > > * After the subsystem tag, short summary starts with a capital lettter.
> > > * Missing fixes tag and cc tag to stable.
> > > * A struct called allocated_banks does not exist.
> > > * Please prefer using an imperative sentence when describing the action
> > > to take e.g. "Thus, initialize crypto_id to HASH_ALGO__LAST".
> >
> > Thanks. I will fix these issues in the next version of the patch set.
>
> Jarkko, I realize this is a TPM patch, but this patch set is dependent
> on it. When this patch is ready, could you create a topic branch,
> which both of us could merge?
WFM.
/Jarkko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox