* [PATCH v27 10/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-02-23 17:25 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, Haitao Huang,
Jethro Beekman
In-Reply-To: <20200223172559.6912-1-jarkko.sakkinen@linux.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
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>
Tested-by: Haitao Huang <haitao.huang@linux.intel.com>
Tested-by: Jethro Beekman <jethro@fortanix.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 | 336 +++++++++
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, 1426 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..f349697c7508
--- /dev/null
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -0,0 +1,336 @@
+// 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;
+
+ /*
+ * Disallow RIE tasks as their VMA permissions might conflict with the
+ * enclave page permissions.
+ */
+ if (!!(current->personality & READ_IMPLIES_EXEC))
+ return -EACCES;
+
+ /* 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 215371588a25..9e6d3e147aa2 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
* Re: [PATCH net] ipv4: ensure rcu_read_lock() in cipso_v4_error()
From: David Miller @ 2020-02-23 5:47 UTC (permalink / raw)
To: mcroce
Cc: netdev, linux-security-module, linux-kernel, paul, kuznet,
yoshfuji, kuba, gnault, eric.dumazet
In-Reply-To: <20200221112838.11324-1-mcroce@redhat.com>
From: Matteo Croce <mcroce@redhat.com>
Date: Fri, 21 Feb 2020 12:28:38 +0100
> Similarly to commit c543cb4a5f07 ("ipv4: ensure rcu_read_lock() in
> ipv4_link_failure()"), __ip_options_compile() must be called under rcu
> protection.
>
> Fixes: 3da1ed7ac398 ("net: avoid use IPCB in cipso_v4_error")
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* [PATCH] device_cgroup: Fix RCU list debugging warning
From: Amol Grover @ 2020-02-22 17:19 UTC (permalink / raw)
To: James Morris, Serge E . Hallyn
Cc: linux-kernel, linux-kernel-mentees, Joel Fernandes,
Madhuparna Bhowmik, Paul E . McKenney, linux-security-module,
Amol Grover
exceptions may be traversed using list_for_each_entry_rcu()
outside of an RCU read side critical section BUT under the
protection of decgroup_mutex. Hence add the corresponding
lockdep expression to fix the following false-positive
warning:
[ 2.304417] =============================
[ 2.304418] WARNING: suspicious RCU usage
[ 2.304420] 5.5.4-stable #17 Tainted: G E
[ 2.304422] -----------------------------
[ 2.304424] security/device_cgroup.c:355 RCU-list traversed in non-reader section!!
Signed-off-by: Amol Grover <frextrite@gmail.com>
---
security/device_cgroup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 7d0f8f7431ff..b7da9e0970d9 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -352,7 +352,8 @@ static bool match_exception_partial(struct list_head *exceptions, short type,
{
struct dev_exception_item *ex;
- list_for_each_entry_rcu(ex, exceptions, list) {
+ list_for_each_entry_rcu(ex, exceptions, list,
+ lockdep_is_held(&devcgroup_mutex)) {
if ((type & DEVCG_DEV_BLOCK) && !(ex->type & DEVCG_DEV_BLOCK))
continue;
if ((type & DEVCG_DEV_CHAR) && !(ex->type & DEVCG_DEV_CHAR))
--
2.24.1
^ permalink raw reply related
* Re: [PATCH 7/7] proc: Ensure we see the exit of each process tid exactly once
From: Eric W. Biederman @ 2020-02-22 15:46 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Linus Torvalds, Al Viro, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Solar Designer
In-Reply-To: <20200221165036.GB16646@redhat.com>
Oleg Nesterov <oleg@redhat.com> writes:
> On 02/20, Eric W. Biederman wrote:
>>
>> +void exchange_tids(struct task_struct *ntask, struct task_struct *otask)
>> +{
>> + /* pid_links[PIDTYPE_PID].next is always NULL */
>> + struct pid *npid = READ_ONCE(ntask->thread_pid);
>> + struct pid *opid = READ_ONCE(otask->thread_pid);
>> +
>> + rcu_assign_pointer(opid->tasks[PIDTYPE_PID].first, &ntask->pid_links[PIDTYPE_PID]);
>> + rcu_assign_pointer(npid->tasks[PIDTYPE_PID].first, &otask->pid_links[PIDTYPE_PID]);
>> + rcu_assign_pointer(ntask->thread_pid, opid);
>> + rcu_assign_pointer(otask->thread_pid, npid);
>
> this breaks has_group_leader_pid()...
>
> proc_pid_readdir() can miss a process doing mt-exec but this looks fixable,
> just we need to update ntask->thread_pid before updating ->first.
>
> The more problematic case is __exit_signal() which does
>
> if (unlikely(has_group_leader_pid(tsk)))
> posix_cpu_timers_exit_group(tsk);
Along with the comment:
/*
* This can only happen if the caller is de_thread().
* FIXME: this is the temporary hack, we should teach
* posix-cpu-timers to handle this case correctly.
*/
So I suspect this is fixable and the above fix might be part of that.
Hmm looking at your commit:
commit e0a70217107e6f9844628120412cb27bb4cea194
Author: Oleg Nesterov <oleg@redhat.com>
Date: Fri Nov 5 16:53:42 2010 +0100
posix-cpu-timers: workaround to suppress the problems with mt exec
posix-cpu-timers.c correctly assumes that the dying process does
posix_cpu_timers_exit_group() and removes all !CPUCLOCK_PERTHREAD
timers from signal->cpu_timers list.
But, it also assumes that timer->it.cpu.task is always the group
leader, and thus the dead ->task means the dead thread group.
This is obviously not true after de_thread() changes the leader.
After that almost every posix_cpu_timer_ method has problems.
It is not simple to fix this bug correctly. First of all, I think
that timer->it.cpu should use struct pid instead of task_struct.
Also, the locking should be reworked completely. In particular,
tasklist_lock should not be used at all. This all needs a lot of
nontrivial and hard-to-test changes.
Change __exit_signal() to do posix_cpu_timers_exit_group() when
the old leader dies during exec. This is not the fix, just the
temporary hack to hide the problem for 2.6.37 and stable. IOW,
this is obviously wrong but this is what we currently have anyway:
cpu timers do not work after mt exec.
In theory this change adds another race. The exiting leader can
detach the timers which were attached to the new leader. However,
the window between de_thread() and release_task() is small, we
can pretend that sys_timer_create() was called before de_thread().
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
It looks like the data structures need fixing. Possibly to use struct
pid. Possibly to move the group data to signal struct.
I think I played with some of that awhile ago.
I am going to move this change to another patchset. So I don't wind up
playing shift the bug around. I thought I would need this to get the
other code working but it turns out we remain bug compatible without
this.
Hopefully I can get something out in the next week or so that addresses
the issues you have pointed out.
Eric
^ permalink raw reply
* Re: [PATCH bpf-next v4 4/8] bpf: lsm: Add support for enabling/disabling BPF hooks
From: Kees Cook @ 2020-02-22 4:26 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Thomas Garnier, Michael Halcrow,
Paul Turner, Brendan Gregg, Jann Horn, Matthew Garrett,
Christian Brauner, Florent Revest, Brendan Jackman,
Martin KaFai Lau, Song Liu, Yonghong Song, Serge E. Hallyn,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <20200220175250.10795-5-kpsingh@chromium.org>
On Thu, Feb 20, 2020 at 06:52:46PM +0100, KP Singh wrote:
> index aa111392a700..569cc07d5e34 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -804,6 +804,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> break;
> }
> }
> +#ifdef CONFIG_BPF_LSM
> + if (HAS_BPF_LSM_PROG(vm_enough_memory)) {
> + rc = bpf_lsm_vm_enough_memory(mm, pages);
> + if (rc <= 0)
> + cap_sys_admin = 0;
> + }
> +#endif
This pattern of using #ifdef in code is not considered best practice.
Using in-code IS_ENABLED(CONFIG_BPF_LSM) is preferred. But since this
pattern always uses HAS_BPF_LSM_PROG(), you could fold the
IS_ENABLED() into the definition of HAS_BPF_LSM_PROG itself -- or more
likely, have the macro defined as:
#ifdef CONFIG_BPF_LSM
# define HAS_BPF_LSM_PROG(x) ....existing implementation....
#else
# define HAS_BPF_LSM_PROG(x) false
#endif
Then none of these ifdefs are needed.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Kees Cook @ 2020-02-22 4:22 UTC (permalink / raw)
To: Casey Schaufler
Cc: KP Singh, LKML, Linux Security Module list, Alexei Starovoitov,
James Morris
In-Reply-To: <0ef26943-9619-3736-4452-fec536a8d169@schaufler-ca.com>
On Thu, Feb 20, 2020 at 03:49:05PM -0800, Casey Schaufler wrote:
> On 2/20/2020 9:52 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
>
> Sorry about the heavy list pruning - the original set
> blows thunderbird up.
(I've added some people back; I had to dig this thread back out of lkml
since I didn't get a direct copy...)
> > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > overhead of retpolines. These programs cannot be attached to security_*
> > wrappers as there are quite a few security_* functions that do more than
> > just calling the LSM callbacks.
> >
> > This was discussed on the lists in:
> >
> > https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> >
> > Adding a NOP callback after all the static LSM callbacks are called has
> > the following benefits:
> >
> > - The BPF programs run at the right stage of the security_* wrappers.
> > - They run after all the static LSM hooks allowed the operation,
> > therefore cannot allow an action that was already denied.
>
> I still say that the special call-out to BPF is unnecessary.
> I remain unconvinced by the arguments. You aren't doing anything
> so special that the general mechanism won't work.
If I'm understanding this correctly, there are two issues:
1- BPF needs to be run last due to fexit trampolines (?)
2- BPF hooks don't know what may be attached at any given time, so
ALL LSM hooks need to be universally hooked. THIS turns out to create
a measurable performance problem in that the cost of the indirect call
on the (mostly/usually) empty BPF policy is too high.
"1" can be solved a lot of ways, and doesn't seem to be a debated part
of this series.
"2" is interesting -- it creates a performance problem for EVERYONE that
builds in this kernel feature, regardless of them using it. Excepting
SELinux, "traditional" LSMs tends to be relatively sparse in their hooking:
$ grep '^ struct hlist_head' include/linux/lsm_hooks.h | wc -l
230
$ for i in apparmor loadpin lockdown safesetid selinux smack tomoyo yama ; \
do echo -n "$i " && (cd $i && git grep LSM_HOOK_INIT | wc -l) ; done
apparmor 68
loadpin 3
lockdown 1
safesetid 2
selinux 202
smack 108
tomoyo 28
yama 4
So, trying to avoid the indirect calls is, as you say, an optimization,
but it might be a needed one due to the other limitations.
To me, some questions present themselves:
a) What, exactly, are the performance characteristics of:
"before"
"with indirect calls"
"with static keys optimization"
b) Would there actually be a global benefit to using the static keys
optimization for other LSMs? (Especially given that they're already
sparsely populated and policy likely determines utility -- all the
LSMs would just turn ON all their static keys or turn off ALL their
static keys depending on having policy loaded.)
If static keys are justified for KRSI (by "a") then it seems the approach
here should stand. If "b" is also true, then we need an additional
series to apply this optimization for the other LSMs (but that seems
distinctly separate from THIS series).
--
Kees Cook
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: Kees Cook @ 2020-02-22 3:36 UTC (permalink / raw)
To: Casey Schaufler
Cc: KP Singh, Linux Security Module list, LKML, bpf, James Morris
In-Reply-To: <7fd415e0-35c8-e30e-e4b8-af0ba286f628@schaufler-ca.com>
On Fri, Feb 21, 2020 at 05:04:38PM -0800, Casey Schaufler wrote:
> On 2/21/2020 4:22 PM, Kees Cook wrote:
> > I really like this approach: it actually _simplifies_ the LSM piece in
> > that there is no need to keep the union and the hook lists in sync any
> > more: they're defined once now. (There were already 2 lists, and this
> > collapses the list into 1 place for all 3 users.) It's very visible in
> > the diffstat too (~300 lines removed):
>
> Erk. Too many smart people like this. I still don't, but it's possible
> that I could learn to.
Well, I admit that I am, perhaps, overly infatuatied with "fancy" macros,
but in cases like this where we're operating on a list of stuff and doing
the same thing over and over but with different elements, I've found
this is actually much nicer way to do it. (E.g. I did something like
this in drivers/misc/lkdtm/core.c to avoid endless typing, and Mimi did
something similar in include/linux/fs.h for keeping kernel_read_file_id
and kernel_read_file_str automatically in sync.) KP's macros are more
extensive, but I think it's a clever to avoid going crazy as LSM hooks
evolve.
> > Also, there is no need to worry about divergence: the BPF will always
> > track the exposed LSM. Backward compat is (AIUI) explicitly a
> > non-feature.
>
> As written you're correct, it can't diverge. My concern is about
> what happens when someone decides that they want the BPF and hook
> to be different. I fear there will be a hideous solution.
This is related to some of the discussion at the last Maintainer's
Summit and tracepoints: i.e. the exposure of what is basically kernel
internals to a userspace system. The conclusion there (which, I think,
has been extended strongly into BPF) is that things that produce BPF are
accepted to be strongly tied to kernel version, so if a hook changes, so
much the userspace side. This appears to be proven out in the existing
BPF world, which gives me some evidence that this claim (close tie to
kernel version) isn't an empty promise.
> > I don't see why anything here is "harmful"?
>
> Injecting large chucks of code via an #include does nothing
> for readability. I've seen it fail disastrously many times,
> usually after the original author has moved on and entrusted
> the code to someone who missed some of the nuance.
I totally agree about wanting to avoid reduced readability. In this case,
I actually think readability is improved since the macro "implementation"
are right above each #include. And then looking at the resulting included
header, all the metadata is visible in one place. But I agree: it is
"unusual", but I think on the sum it's an improvement. (But I share some
of the frustration of the kernel being filled with weird preprocessor
insanity. I will never get back the weeks I spent on trying to improve
the min/max macros.... *sob*)
> I'll drop objection to this bit, but still object to making
> BPF special in the infrastructure. It doesn't need to be and
> it is exactly the kind of additional complexity we need to
> avoid.
You mean 3/8's RUN_BPF_LSM_*_PROGS() additions to the call_*_hook()s?
I'll go comment on that thread directly instead of splitting the
discussion. :)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: Casey Schaufler @ 2020-02-22 1:04 UTC (permalink / raw)
To: Kees Cook
Cc: KP Singh, Linux Security Module list, LKML, bpf, James Morris,
Casey Schaufler
In-Reply-To: <202002211617.28EAC6826@keescook>
On 2/21/2020 4:22 PM, Kees Cook wrote:
> On Fri, Feb 21, 2020 at 02:31:18PM -0800, Casey Schaufler wrote:
>> On 2/21/2020 11:41 AM, KP Singh wrote:
>>> On 21-Feb 11:19, Casey Schaufler wrote:
>>>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>>>> From: KP Singh <kpsingh@google.com>
>>>>> # v3 -> v4
>>>>>
>>>>> https://lkml.org/lkml/2020/1/23/515
>>>>>
>>>>> * Moved away from allocating a separate security_hook_heads and adding a
>>>>> new special case for arch_prepare_bpf_trampoline to using BPF fexit
>>>>> trampolines called from the right place in the LSM hook and toggled by
>>>>> static keys based on the discussion in:
>>>>>
>>>>> https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
>>>>>
>>>>> * Since the code does not deal with security_hook_heads anymore, it goes
>>>>> from "being a BPF LSM" to "BPF program attachment to LSM hooks".
>>>> I've finally been able to review the entire patch set.
>>>> I can't imagine how it can make sense to add this much
>>>> complexity to the LSM infrastructure in support of this
>>>> feature. There is macro magic going on that is going to
>>>> break, and soon. You are introducing dependencies on BPF
>>>> into the infrastructure, and that's unnecessary and most
>>>> likely harmful.
>>> We will be happy to document each of the macros in detail. Do note a
>>> few things here:
>>>
>>> * There is really nothing magical about them though,
>>
>> +#define LSM_HOOK_void(NAME, ...) \
>> + noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
>> +
>> +#include <linux/lsm_hook_names.h>
>> +#undef LSM_HOOK
>>
>> I haven't seen anything this ... novel ... in a very long time.
>> I see why you want to do this, but you're tying the two sets
>> of code together unnaturally. When (not if) the two sets diverge
>> you're going to be introducing another clever way to deal with
>> the special case.
> I really like this approach: it actually _simplifies_ the LSM piece in
> that there is no need to keep the union and the hook lists in sync any
> more: they're defined once now. (There were already 2 lists, and this
> collapses the list into 1 place for all 3 users.) It's very visible in
> the diffstat too (~300 lines removed):
Erk. Too many smart people like this. I still don't, but it's possible
that I could learn to.
>
> include/linux/lsm_hook_names.h | 353 +++++++++++++++++++
> include/linux/lsm_hooks.h | 622 +--------------------------------
> 2 files changed, 359 insertions(+), 616 deletions(-)
>
> Also, there is no need to worry about divergence: the BPF will always
> track the exposed LSM. Backward compat is (AIUI) explicitly a
> non-feature.
As written you're correct, it can't diverge. My concern is about
what happens when someone decides that they want the BPF and hook
to be different. I fear there will be a hideous solution.
> I don't see why anything here is "harmful"?
Injecting large chucks of code via an #include does nothing
for readability. I've seen it fail disastrously many times,
usually after the original author has moved on and entrusted
the code to someone who missed some of the nuance.
I'll drop objection to this bit, but still object to making
BPF special in the infrastructure. It doesn't need to be and
it is exactly the kind of additional complexity we need to
avoid.
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: Kees Cook @ 2020-02-22 0:22 UTC (permalink / raw)
To: Casey Schaufler
Cc: KP Singh, Linux Security Module list, LKML, bpf, James Morris
In-Reply-To: <8a2a2d59-ec4b-80d1-2710-c2ead588e638@schaufler-ca.com>
On Fri, Feb 21, 2020 at 02:31:18PM -0800, Casey Schaufler wrote:
> On 2/21/2020 11:41 AM, KP Singh wrote:
> > On 21-Feb 11:19, Casey Schaufler wrote:
> >> On 2/20/2020 9:52 AM, KP Singh wrote:
> >>> From: KP Singh <kpsingh@google.com>
> >>> # v3 -> v4
> >>>
> >>> https://lkml.org/lkml/2020/1/23/515
> >>>
> >>> * Moved away from allocating a separate security_hook_heads and adding a
> >>> new special case for arch_prepare_bpf_trampoline to using BPF fexit
> >>> trampolines called from the right place in the LSM hook and toggled by
> >>> static keys based on the discussion in:
> >>>
> >>> https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
> >>>
> >>> * Since the code does not deal with security_hook_heads anymore, it goes
> >>> from "being a BPF LSM" to "BPF program attachment to LSM hooks".
> >> I've finally been able to review the entire patch set.
> >> I can't imagine how it can make sense to add this much
> >> complexity to the LSM infrastructure in support of this
> >> feature. There is macro magic going on that is going to
> >> break, and soon. You are introducing dependencies on BPF
> >> into the infrastructure, and that's unnecessary and most
> >> likely harmful.
> > We will be happy to document each of the macros in detail. Do note a
> > few things here:
> >
> > * There is really nothing magical about them though,
>
>
> +#define LSM_HOOK_void(NAME, ...) \
> + noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
> +
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
>
> I haven't seen anything this ... novel ... in a very long time.
> I see why you want to do this, but you're tying the two sets
> of code together unnaturally. When (not if) the two sets diverge
> you're going to be introducing another clever way to deal with
> the special case.
I really like this approach: it actually _simplifies_ the LSM piece in
that there is no need to keep the union and the hook lists in sync any
more: they're defined once now. (There were already 2 lists, and this
collapses the list into 1 place for all 3 users.) It's very visible in
the diffstat too (~300 lines removed):
include/linux/lsm_hook_names.h | 353 +++++++++++++++++++
include/linux/lsm_hooks.h | 622 +--------------------------------
2 files changed, 359 insertions(+), 616 deletions(-)
Also, there is no need to worry about divergence: the BPF will always
track the exposed LSM. Backward compat is (AIUI) explicitly a
non-feature.
I don't see why anything here is "harmful"?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: Casey Schaufler @ 2020-02-21 23:49 UTC (permalink / raw)
To: KP Singh
Cc: Linux Security Module list, LKML, bpf, James Morris, Kees Cook,
Alexei Starovoitov, Daniel Borkmann, Casey Schaufler
In-Reply-To: <20200221230933.GA23663@chromium.org>
On 2/21/2020 3:09 PM, KP Singh wrote:
> Thanks Casey,
>
> I appreciate your quick responses!
>
> On 21-Feb 14:31, Casey Schaufler wrote:
>> On 2/21/2020 11:41 AM, KP Singh wrote:
>>> On 21-Feb 11:19, Casey Schaufler wrote:
>>>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>>>> From: KP Singh <kpsingh@google.com>
>>>> Again, apologies for the CC list trimming.
>>>>
>>>>> # v3 -> v4
>>>>>
>>>>> https://lkml.org/lkml/2020/1/23/515
>>>>>
>>>>> * Moved away from allocating a separate security_hook_heads and adding a
>>>>> new special case for arch_prepare_bpf_trampoline to using BPF fexit
>>>>> trampolines called from the right place in the LSM hook and toggled by
>>>>> static keys based on the discussion in:
>>>>>
>>>>> https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
>>>>>
>>>>> * Since the code does not deal with security_hook_heads anymore, it goes
>>>>> from "being a BPF LSM" to "BPF program attachment to LSM hooks".
> [...]
>
>>>> likely harmful.
>>> We will be happy to document each of the macros in detail. Do note a
>>> few things here:
>>>
>>> * There is really nothing magical about them though,
>>
>> +#define LSM_HOOK_void(NAME, ...) \
>> + noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
>> +
>> +#include <linux/lsm_hook_names.h>
>> +#undef LSM_HOOK
>>
>> I haven't seen anything this ... novel ... in a very long time.
> This is not "novel", it's a fairly common pattern followed in tracing:
>
> For example, the TRACE_INCLUDE macro which is used for tracepoints:
>
> include/trace/define_trace.h
>
> and used in:
>
> * include/trace/bpf_probe.h
>
> https://github.com/torvalds/linux/blob/master/include/trace/bpf_probe.h#L110
>
> * include/trace/perf.h
>
> https://github.com/torvalds/linux/blob/master/include/trace/perf.h#L90
>
> * include/trace/trace_events.h
>
> https://github.com/torvalds/linux/blob/master/include/trace/trace_events.h#L402
I can't say I care for that, either, and it's a simpler case.
>> I see why you want to do this, but you're tying the two sets
>> of code together unnaturally. When (not if) the two sets diverge
>> you're going to be introducing another clever way to deal with
> I don't fully understand what "two sets diverge means" here. All the
> BPF headers need is the name, return type and the args. This is the
> same information which is needed by the call_{int, void}_hooks and the
> LSM declarataions (i.e. security_hook_heads and
> security_list_options).
As you've noticed, not all the interfaces can use call_{int,void}_hooks.
If you've been following the stacking efforts, you'll see that increasing.
At some point I anticipate a BPF hook that needs different information
than the LSM hook. That's been discussed, too. Asserting that it will
never happen does not make me comfortable.
>> the special case.
>>
>> It's not that I don't understand what you're doing. It's that
>> I don't like what you're doing. Explanation doesn't make me like
>> it better.
> As I have previously said, we will be happy to (and have already)
> updated our approach based on the consensus we arrive at here.
Not to put too fine a point on it, but I have raised the same
objection - that you should use the infrastructure as it is -
each time. I do not see consensus, I see you plowing ahead with
the direction you've chosen in spite of the significant objection.
> The
> best outcome would be to not sacrifice performance as the LSM hooks
> are called from various performance critical code-paths.
Then help me tune the infrastructure to be better in those cases.
> It would be great to know the maintainers' (BPF and Security)
> perspective on this as well.
Many eyes and all that, but the BPF maintainers haven't been working
with the LSM infrastructure and won't be familiar with its quirks.
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: KP Singh @ 2020-02-21 23:09 UTC (permalink / raw)
To: Casey Schaufler
Cc: Linux Security Module list, LKML, bpf, James Morris, Kees Cook,
Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <8a2a2d59-ec4b-80d1-2710-c2ead588e638@schaufler-ca.com>
Thanks Casey,
I appreciate your quick responses!
On 21-Feb 14:31, Casey Schaufler wrote:
> On 2/21/2020 11:41 AM, KP Singh wrote:
> > On 21-Feb 11:19, Casey Schaufler wrote:
> >> On 2/20/2020 9:52 AM, KP Singh wrote:
> >>> From: KP Singh <kpsingh@google.com>
> >> Again, apologies for the CC list trimming.
> >>
> >>> # v3 -> v4
> >>>
> >>> https://lkml.org/lkml/2020/1/23/515
> >>>
> >>> * Moved away from allocating a separate security_hook_heads and adding a
> >>> new special case for arch_prepare_bpf_trampoline to using BPF fexit
> >>> trampolines called from the right place in the LSM hook and toggled by
> >>> static keys based on the discussion in:
> >>>
> >>> https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
> >>>
> >>> * Since the code does not deal with security_hook_heads anymore, it goes
> >>> from "being a BPF LSM" to "BPF program attachment to LSM hooks".
[...]
> >> likely harmful.
> > We will be happy to document each of the macros in detail. Do note a
> > few things here:
> >
> > * There is really nothing magical about them though,
>
>
> +#define LSM_HOOK_void(NAME, ...) \
> + noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
> +
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
>
> I haven't seen anything this ... novel ... in a very long time.
This is not "novel", it's a fairly common pattern followed in tracing:
For example, the TRACE_INCLUDE macro which is used for tracepoints:
include/trace/define_trace.h
and used in:
* include/trace/bpf_probe.h
https://github.com/torvalds/linux/blob/master/include/trace/bpf_probe.h#L110
* include/trace/perf.h
https://github.com/torvalds/linux/blob/master/include/trace/perf.h#L90
* include/trace/trace_events.h
https://github.com/torvalds/linux/blob/master/include/trace/trace_events.h#L402
> I see why you want to do this, but you're tying the two sets
> of code together unnaturally. When (not if) the two sets diverge
> you're going to be introducing another clever way to deal with
I don't fully understand what "two sets diverge means" here. All the
BPF headers need is the name, return type and the args. This is the
same information which is needed by the call_{int, void}_hooks and the
LSM declarataions (i.e. security_hook_heads and
security_list_options).
> the special case.
>
> It's not that I don't understand what you're doing. It's that
> I don't like what you're doing. Explanation doesn't make me like
> it better.
As I have previously said, we will be happy to (and have already)
updated our approach based on the consensus we arrive at here. The
best outcome would be to not sacrifice performance as the LSM hooks
are called from various performance critical code-paths.
It would be great to know the maintainers' (BPF and Security)
perspective on this as well.
- KP
>
> > the LSM hooks are
> > collectively declared in lsm_hook_names.h and are used to delcare
> > the security_list_options and security_hook_heads for the LSM
> > framework (this was previously maitained in two different places):
> >
> > For BPF, they declare:
> >
> > * bpf_lsm_<name> attachment points and their prototypes.
> > * A static key (bpf_lsm_key_<name>) to enable and disable these
> > hooks with a function to set its value i.e.
> > (bpf_lsm_<name>_set_enabled).
> >
> > * We have kept the BPF related macros out of security/.
> > * All the BPF calls in the LSM infrastructure are guarded by
> > CONFIG_BPF_LSM (there are only two main calls though, i.e.
> > call_int_hook, call_void_hook).
> >
> > Honestly, the macros aren't any more complicated than
> > call_int_progs/call_void_progs.
> >
> > - KP
> >
> >> Would you please drop the excessive optimization? I understand
> >> that there's been a lot of discussion and debate about it,
> >> but this implementation is out of control, disruptive, and
> >> dangerous to the code around it.
> >>
> >>
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: Casey Schaufler @ 2020-02-21 22:31 UTC (permalink / raw)
To: KP Singh
Cc: Linux Security Module list, LKML, bpf, James Morris, Kees Cook,
Casey Schaufler
In-Reply-To: <20200221194149.GA9207@chromium.org>
On 2/21/2020 11:41 AM, KP Singh wrote:
> On 21-Feb 11:19, Casey Schaufler wrote:
>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>> Again, apologies for the CC list trimming.
>>
>>> # v3 -> v4
>>>
>>> https://lkml.org/lkml/2020/1/23/515
>>>
>>> * Moved away from allocating a separate security_hook_heads and adding a
>>> new special case for arch_prepare_bpf_trampoline to using BPF fexit
>>> trampolines called from the right place in the LSM hook and toggled by
>>> static keys based on the discussion in:
>>>
>>> https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
>>>
>>> * Since the code does not deal with security_hook_heads anymore, it goes
>>> from "being a BPF LSM" to "BPF program attachment to LSM hooks".
>> I've finally been able to review the entire patch set.
>> I can't imagine how it can make sense to add this much
>> complexity to the LSM infrastructure in support of this
>> feature. There is macro magic going on that is going to
>> break, and soon. You are introducing dependencies on BPF
>> into the infrastructure, and that's unnecessary and most
>> likely harmful.
> We will be happy to document each of the macros in detail. Do note a
> few things here:
>
> * There is really nothing magical about them though,
+#define LSM_HOOK_void(NAME, ...) \
+ noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
+
+#include <linux/lsm_hook_names.h>
+#undef LSM_HOOK
I haven't seen anything this ... novel ... in a very long time.
I see why you want to do this, but you're tying the two sets
of code together unnaturally. When (not if) the two sets diverge
you're going to be introducing another clever way to deal with
the special case.
It's not that I don't understand what you're doing. It's that
I don't like what you're doing. Explanation doesn't make me like
it better.
> the LSM hooks are
> collectively declared in lsm_hook_names.h and are used to delcare
> the security_list_options and security_hook_heads for the LSM
> framework (this was previously maitained in two different places):
>
> For BPF, they declare:
>
> * bpf_lsm_<name> attachment points and their prototypes.
> * A static key (bpf_lsm_key_<name>) to enable and disable these
> hooks with a function to set its value i.e.
> (bpf_lsm_<name>_set_enabled).
>
> * We have kept the BPF related macros out of security/.
> * All the BPF calls in the LSM infrastructure are guarded by
> CONFIG_BPF_LSM (there are only two main calls though, i.e.
> call_int_hook, call_void_hook).
>
> Honestly, the macros aren't any more complicated than
> call_int_progs/call_void_progs.
>
> - KP
>
>> Would you please drop the excessive optimization? I understand
>> that there's been a lot of discussion and debate about it,
>> but this implementation is out of control, disruptive, and
>> dangerous to the code around it.
>>
>>
^ permalink raw reply
* Re: [PATCH net] ipv4: ensure rcu_read_lock() in cipso_v4_error()
From: Paul Moore @ 2020-02-21 21:18 UTC (permalink / raw)
To: Matteo Croce
Cc: netdev, linux-security-module, linux-kernel, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Jakub Kicinski,
Guillaume Nault, Eric Dumazet
In-Reply-To: <20200221112838.11324-1-mcroce@redhat.com>
On Fri, Feb 21, 2020 at 6:28 AM Matteo Croce <mcroce@redhat.com> wrote:
>
> Similarly to commit c543cb4a5f07 ("ipv4: ensure rcu_read_lock() in
> ipv4_link_failure()"), __ip_options_compile() must be called under rcu
> protection.
>
> Fixes: 3da1ed7ac398 ("net: avoid use IPCB in cipso_v4_error")
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
> net/ipv4/cipso_ipv4.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
This seems consistent with the ipv4_link_failure() fix, even though
ipv4_link_failure() has changed a bit since the fix.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 376882215919..0bd10a1f477f 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1724,6 +1724,7 @@ void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
> {
> unsigned char optbuf[sizeof(struct ip_options) + 40];
> struct ip_options *opt = (struct ip_options *)optbuf;
> + int res;
>
> if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
> return;
> @@ -1735,7 +1736,11 @@ void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
>
> memset(opt, 0, sizeof(struct ip_options));
> opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
> - if (__ip_options_compile(dev_net(skb->dev), opt, skb, NULL))
> + rcu_read_lock();
> + res = __ip_options_compile(dev_net(skb->dev), opt, skb, NULL);
> + rcu_read_unlock();
> +
> + if (res)
> return;
>
> if (gateway)
> --
> 2.24.1
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: KP Singh @ 2020-02-21 19:41 UTC (permalink / raw)
To: Casey Schaufler
Cc: Linux Security Module list, LKML, bpf, James Morris, Kees Cook
In-Reply-To: <85e89b0c-5f2c-a4b1-17d3-47cc3bdab38b@schaufler-ca.com>
On 21-Feb 11:19, Casey Schaufler wrote:
> On 2/20/2020 9:52 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
>
> Again, apologies for the CC list trimming.
>
> >
> > # v3 -> v4
> >
> > https://lkml.org/lkml/2020/1/23/515
> >
> > * Moved away from allocating a separate security_hook_heads and adding a
> > new special case for arch_prepare_bpf_trampoline to using BPF fexit
> > trampolines called from the right place in the LSM hook and toggled by
> > static keys based on the discussion in:
> >
> > https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
> >
> > * Since the code does not deal with security_hook_heads anymore, it goes
> > from "being a BPF LSM" to "BPF program attachment to LSM hooks".
>
> I've finally been able to review the entire patch set.
> I can't imagine how it can make sense to add this much
> complexity to the LSM infrastructure in support of this
> feature. There is macro magic going on that is going to
> break, and soon. You are introducing dependencies on BPF
> into the infrastructure, and that's unnecessary and most
> likely harmful.
We will be happy to document each of the macros in detail. Do note a
few things here:
* There is really nothing magical about them though, the LSM hooks are
collectively declared in lsm_hook_names.h and are used to delcare
the security_list_options and security_hook_heads for the LSM
framework (this was previously maitained in two different places):
For BPF, they declare:
* bpf_lsm_<name> attachment points and their prototypes.
* A static key (bpf_lsm_key_<name>) to enable and disable these
hooks with a function to set its value i.e.
(bpf_lsm_<name>_set_enabled).
* We have kept the BPF related macros out of security/.
* All the BPF calls in the LSM infrastructure are guarded by
CONFIG_BPF_LSM (there are only two main calls though, i.e.
call_int_hook, call_void_hook).
Honestly, the macros aren't any more complicated than
call_int_progs/call_void_progs.
- KP
>
> Would you please drop the excessive optimization? I understand
> that there's been a lot of discussion and debate about it,
> but this implementation is out of control, disruptive, and
> dangerous to the code around it.
>
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/8] MAC and Audit policy using eBPF (KRSI)
From: Casey Schaufler @ 2020-02-21 19:19 UTC (permalink / raw)
To: KP Singh
Cc: Linux Security Module list, LKML, bpf, James Morris, Kees Cook,
Casey Schaufler
In-Reply-To: <20200220175250.10795-1-kpsingh@chromium.org>
On 2/20/2020 9:52 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
Again, apologies for the CC list trimming.
>
> # v3 -> v4
>
> https://lkml.org/lkml/2020/1/23/515
>
> * Moved away from allocating a separate security_hook_heads and adding a
> new special case for arch_prepare_bpf_trampoline to using BPF fexit
> trampolines called from the right place in the LSM hook and toggled by
> static keys based on the discussion in:
>
> https://lore.kernel.org/bpf/CAG48ez25mW+_oCxgCtbiGMX07g_ph79UOJa07h=o_6B6+Q-u5g@mail.gmail.com/
>
> * Since the code does not deal with security_hook_heads anymore, it goes
> from "being a BPF LSM" to "BPF program attachment to LSM hooks".
I've finally been able to review the entire patch set.
I can't imagine how it can make sense to add this much
complexity to the LSM infrastructure in support of this
feature. There is macro magic going on that is going to
break, and soon. You are introducing dependencies on BPF
into the infrastructure, and that's unnecessary and most
likely harmful.
Would you please drop the excessive optimization? I understand
that there's been a lot of discussion and debate about it,
but this implementation is out of control, disruptive, and
dangerous to the code around it.
^ permalink raw reply
* Re: [PATCH bpf-next v4 4/8] bpf: lsm: Add support for enabling/disabling BPF hooks
From: James Morris @ 2020-02-21 19:11 UTC (permalink / raw)
To: Casey Schaufler; +Cc: KP Singh, LKML, Linux Security Module list, bpf
In-Reply-To: <bb32f155-5213-71df-c679-85c614c0ac26@schaufler-ca.com>
On Fri, 21 Feb 2020, Casey Schaufler wrote:
> On 2/20/2020 9:52 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
>
> Again, sorry for trimming the CC list, but thunderbird ...
Fix your mail client, please.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH bpf-next v4 4/8] bpf: lsm: Add support for enabling/disabling BPF hooks
From: Casey Schaufler @ 2020-02-21 18:57 UTC (permalink / raw)
To: KP Singh; +Cc: LKML, Linux Security Module list, bpf, Casey Schaufler
In-Reply-To: <20200220175250.10795-5-kpsingh@chromium.org>
On 2/20/2020 9:52 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
Again, sorry for trimming the CC list, but thunderbird ...
>
> Each LSM hook defines a static key i.e. bpf_lsm_<name>
> and a bpf_lsm_<name>_set_enabled function to toggle the key
> which enables/disables the branch which executes the BPF programs
> attached to the LSM hook.
>
> Use of static keys was suggested in upstream discussion:
>
> https://lore.kernel.org/bpf/1cd10710-a81b-8f9b-696d-aa40b0a67225@iogearbox.net/
>
> and results in the following assembly:
>
> 0x0000000000001e31 <+65>: jmpq 0x1e36 <security_bprm_check+70>
> 0x0000000000001e36 <+70>: nopl 0x0(%rax,%rax,1)
> 0x0000000000001e3b <+75>: xor %eax,%eax
> 0x0000000000001e3d <+77>: jmp 0x1e25 <security_bprm_check+53>
>
> which avoids an indirect branch and results in lower overhead which is
> especially helpful for LSM hooks in performance hotpaths.
>
> Given the ability to toggle the BPF trampolines, some hooks which do
> not call call_<int, void>_hooks as they have different default return
> values, also gain support for BPF program attachment.
>
> There are some hooks like security_setprocattr and security_getprocattr
> which are not instrumentable as they do not provide any monitoring or
> access control decisions. If required, generation of BTF type
> information for these hooks can be also be blacklisted.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
> include/linux/bpf_lsm.h | 30 +++++++++++++++++++++++++++---
> kernel/bpf/bpf_lsm.c | 28 ++++++++++++++++++++++++++++
> security/security.c | 32 ++++++++++++++++++++++++++++++++
> 3 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index f867f72f6aa9..53dcda8ace01 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -8,27 +8,51 @@
> #define _LINUX_BPF_LSM_H
>
> #include <linux/bpf.h>
> +#include <linux/jump_label.h>
>
> #ifdef CONFIG_BPF_LSM
>
> +#define LSM_HOOK(RET, NAME, ...) \
> +DECLARE_STATIC_KEY_FALSE(bpf_lsm_key_##NAME); \
> +void bpf_lsm_##NAME##_set_enabled(bool value);
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
This is an amazing amount of macro magic. You're creating
dependencies that will make changes to the infrastructure
much more difficult. I think. It's really hard to tell.
At the very least you should have a description of what this
accomplishes, as it's far from obvious.
> +
> #define LSM_HOOK(RET, NAME, ...) RET bpf_lsm_##NAME(__VA_ARGS__);
> #include <linux/lsm_hook_names.h>
> #undef LSM_HOOK
>
> -#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) bpf_lsm_##FUNC(__VA_ARGS__)
> +#define HAS_BPF_LSM_PROG(FUNC) (static_branch_unlikely(&bpf_lsm_key_##FUNC))
> +
> +#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) \
> + do { \
> + if (HAS_BPF_LSM_PROG(FUNC)) \
> + bpf_lsm_##FUNC(__VA_ARGS__); \
> + } while (0)
> +
> #define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) ({ \
> do { \
> - if (RC == 0) \
> - RC = bpf_lsm_##FUNC(__VA_ARGS__); \
> + if (HAS_BPF_LSM_PROG(FUNC)) { \
> + if (RC == 0) \
> + RC = bpf_lsm_##FUNC(__VA_ARGS__); \
> + } \
> } while (0); \
> RC; \
> })
>
> +int bpf_lsm_set_enabled(const char *name, bool value);
> +
> #else /* !CONFIG_BPF_LSM */
>
> +#define HAS_BPF_LSM_PROG false
> #define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) (RC)
> #define RUN_BPF_LSM_VOID_PROGS(FUNC, ...)
>
> +static inline int bpf_lsm_set_enabled(const char *name, bool value)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> #endif /* CONFIG_BPF_LSM */
>
> #endif /* _LINUX_BPF_LSM_H */
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index abc847c9b9a1..d7c44433c003 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -8,6 +8,20 @@
> #include <linux/bpf.h>
> #include <linux/btf.h>
> #include <linux/bpf_lsm.h>
> +#include <linux/jump_label.h>
> +#include <linux/kallsyms.h>
> +
> +#define LSM_HOOK(RET, NAME, ...) \
> + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##NAME); \
> + void bpf_lsm_##NAME##_set_enabled(bool value) \
> + { \
> + if (value) \
> + static_branch_enable(&bpf_lsm_key_##NAME); \
> + else \
> + static_branch_disable(&bpf_lsm_key_##NAME); \
> + }
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
>
> /* For every LSM hook that allows attachment of BPF programs, declare a NOP
> * function where a BPF program can be attached as an fexit trampoline.
> @@ -24,6 +38,20 @@
> #include <linux/lsm_hook_names.h>
> #undef LSM_HOOK
>
> +int bpf_lsm_set_enabled(const char *name, bool value)
> +{
> + char toggle_fn_name[KSYM_NAME_LEN];
> + void (*toggle_fn)(bool value);
> +
> + snprintf(toggle_fn_name, KSYM_NAME_LEN, "%s_set_enabled", name);
> + toggle_fn = (void *)kallsyms_lookup_name(toggle_fn_name);
> + if (!toggle_fn)
> + return -ESRCH;
> +
> + toggle_fn(value);
> + return 0;
> +}
> +
> const struct bpf_prog_ops lsm_prog_ops = {
> };
>
> diff --git a/security/security.c b/security/security.c
> index aa111392a700..569cc07d5e34 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -804,6 +804,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> break;
> }
> }
> +#ifdef CONFIG_BPF_LSM
> + if (HAS_BPF_LSM_PROG(vm_enough_memory)) {
> + rc = bpf_lsm_vm_enough_memory(mm, pages);
> + if (rc <= 0)
> + cap_sys_admin = 0;
> + }
> +#endif
> return __vm_enough_memory(mm, pages, cap_sys_admin);
> }
>
> @@ -1350,6 +1357,13 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
> if (rc != -EOPNOTSUPP)
> return rc;
> }
> +#ifdef CONFIG_BPF_LSM
> + if (HAS_BPF_LSM_PROG(inode_getsecurity)) {
> + rc = bpf_lsm_inode_getsecurity(inode, name, buffer, alloc);
> + if (rc != -EOPNOTSUPP)
> + return rc;
> + }
> +#endif
> return -EOPNOTSUPP;
> }
>
> @@ -1369,6 +1383,14 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
> if (rc != -EOPNOTSUPP)
> return rc;
> }
> +#ifdef CONFIG_BPF_LSM
> + if (HAS_BPF_LSM_PROG(inode_setsecurity)) {
> + rc = bpf_lsm_inode_setsecurity(inode, name, value, size,
> + flags);
> + if (rc != -EOPNOTSUPP)
> + return rc;
> + }
> +#endif
> return -EOPNOTSUPP;
> }
>
> @@ -1754,6 +1776,12 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> break;
> }
> }
> +#ifdef CONFIG_BPF_LSM
> + if (HAS_BPF_LSM_PROG(task_prctl)) {
> + if (rc == -ENOSYS)
> + rc = bpf_lsm_task_prctl(option, arg2, arg3, arg4, arg5);
> + }
> +#endif
> return rc;
> }
>
> @@ -2334,6 +2362,10 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
> rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
> break;
> }
> +#ifdef CONFIG_BPF_LSM
> + if (HAS_BPF_LSM_PROG(xfrm_state_pol_flow_match))
> + rc = bpf_lsm_xfrm_state_pol_flow_match(x, xp, fl);
> +#endif
> return rc;
> }
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Casey Schaufler @ 2020-02-21 18:23 UTC (permalink / raw)
To: KP Singh; +Cc: LKML, Linux Security Module list, bpf, Casey Schaufler
In-Reply-To: <20200221114458.GA56944@google.com>
On 2/21/2020 3:44 AM, KP Singh wrote:
> On 20-Feb 15:49, Casey Schaufler wrote:
>> On 2/20/2020 9:52 AM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>> Sorry about the heavy list pruning - the original set
>> blows thunderbird up.
>>
>>> The BPF LSM programs are implemented as fexit trampolines to avoid the
>>> overhead of retpolines. These programs cannot be attached to security_*
>>> wrappers as there are quite a few security_* functions that do more than
>>> just calling the LSM callbacks.
>>>
>>> This was discussed on the lists in:
>>>
>>> https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
>>>
>>> Adding a NOP callback after all the static LSM callbacks are called has
>>> the following benefits:
>>>
>>> - The BPF programs run at the right stage of the security_* wrappers.
>>> - They run after all the static LSM hooks allowed the operation,
>>> therefore cannot allow an action that was already denied.
>> I still say that the special call-out to BPF is unnecessary.
>> I remain unconvinced by the arguments. You aren't doing anything
>> so special that the general mechanism won't work.
> The existing mechanism would work functionally, but the cost of an
> indirect call for all the hooks, even those that are completely unused
> is not really acceptable for KRSI’s use cases.
Are you at all familiar with the way LSMs where installed
before the current list infrastructure? Every interface had
a hook that got called, even if the installed module did not
provide one. That was deemed acceptable for a good long time.
Way back in the early days of the stacking effort I seriously
considered implementing a new security module that would do
the stacking, and leave the infrastructure alone. Very much
like what you're proposing for BPF modules. It would have worked,
but the list model works better.
> It’s easy to avoid and
> I do think that what we’re doing here (with hooks being defined at
> runtime) has significant functional differences from existing LSMs.
KRSI isn't all that different from the other modules.
The way you specify where system policy is restricted
and under which circumstances is different. You're trying
to be extremely general, beyond the Mandatory Access Control
claims of the existing modules. But really, there's nothing
all that special.
I know that you don't want to be making a lot of checks on
empty BPF program lists. You've come up with clever hacks
to avoid doing so. But the cleverer the hack, the more likely
it is to haunt someone else later. It probably won't cause
KRSI any grief, but you can bet someone will take it in the
chin.
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: James Morris @ 2020-02-21 17:56 UTC (permalink / raw)
To: Daniel Colascione
Cc: Casey Schaufler, Tim Murray, Nosh Minwalla, Nick Kralevich,
Lokesh Gidra, linux-kernel, Linux API, selinux, LSM List
In-Reply-To: <CAKOZueuh2MR4UKi60-GVgPkXjncHx8J=mTTjRquB82CfS7DxBA@mail.gmail.com>
On Tue, 11 Feb 2020, Daniel Colascione wrote:
> > This must be posted to the linux Security Module list
> > <linux-security-module@vger.kernel.org>
>
> Added. I thought selinux@ was sufficient.
>
Please cc: me on these patches.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH -next] security: remove duplicated include from security.h
From: James Morris @ 2020-02-21 16:56 UTC (permalink / raw)
To: YueHaibing
Cc: dhowells, casey, sds, zohar, linux-kernel, linux-security-module
In-Reply-To: <20200221074342.16788-1-yuehaibing@huawei.com>
On Fri, 21 Feb 2020, YueHaibing wrote:
> Remove duplicated include.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> include/linux/security.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 910a1ef..fe2c566 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -30,7 +30,6 @@
> #include <linux/err.h>
> #include <linux/string.h>
> #include <linux/mm.h>
> -#include <linux/fs.h>
>
> struct linux_binprm;
> struct cred;
>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] lockdown: Allow unprivileged users to see lockdown status
From: James Morris @ 2020-02-21 16:51 UTC (permalink / raw)
To: Jeremy Cline
Cc: Serge E . Hallyn, Matthew Garrett, David Howells,
linux-security-module, linux-kernel, Frank Ch . Eigler
In-Reply-To: <20200220151738.1492852-1-jcline@redhat.com>
On Thu, 20 Feb 2020, Jeremy Cline wrote:
> A number of userspace tools, such as systemtap, need a way to see the
> current lockdown state so they can gracefully deal with the kernel being
> locked down. The state is already exposed in
> /sys/kernel/security/lockdown, but is only readable by root. Adjust the
> permissions so unprivileged users can read the state.
>
> Fixes: 000d388ed3bb ("security: Add a static lockdown policy LSM")
> Cc: Frank Ch. Eigler <fche@redhat.com>
> Signed-off-by: Jeremy Cline <jcline@redhat.com>
Looks fine to me, any objection from Matthew or others?
> ---
> security/lockdown/lockdown.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index 5a952617a0eb..87cbdc64d272 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -150,7 +150,7 @@ static int __init lockdown_secfs_init(void)
> {
> struct dentry *dentry;
>
> - dentry = securityfs_create_file("lockdown", 0600, NULL, NULL,
> + dentry = securityfs_create_file("lockdown", 0644, NULL, NULL,
> &lockdown_ops);
> return PTR_ERR_OR_ZERO(dentry);
> }
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 7/7] proc: Ensure we see the exit of each process tid exactly once
From: Oleg Nesterov @ 2020-02-21 16:50 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, Al Viro, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Solar Designer
In-Reply-To: <87r1yp7zhc.fsf_-_@x220.int.ebiederm.org>
On 02/20, Eric W. Biederman wrote:
>
> +void exchange_tids(struct task_struct *ntask, struct task_struct *otask)
> +{
> + /* pid_links[PIDTYPE_PID].next is always NULL */
> + struct pid *npid = READ_ONCE(ntask->thread_pid);
> + struct pid *opid = READ_ONCE(otask->thread_pid);
> +
> + rcu_assign_pointer(opid->tasks[PIDTYPE_PID].first, &ntask->pid_links[PIDTYPE_PID]);
> + rcu_assign_pointer(npid->tasks[PIDTYPE_PID].first, &otask->pid_links[PIDTYPE_PID]);
> + rcu_assign_pointer(ntask->thread_pid, opid);
> + rcu_assign_pointer(otask->thread_pid, npid);
this breaks has_group_leader_pid()...
proc_pid_readdir() can miss a process doing mt-exec but this looks fixable,
just we need to update ntask->thread_pid before updating ->first.
The more problematic case is __exit_signal() which does
if (unlikely(has_group_leader_pid(tsk)))
posix_cpu_timers_exit_group(tsk);
Oleg.
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-02-21 13:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Sean Christopherson, Jordan Hand, linux-kernel, x86, linux-sgx,
akpm, dave.hansen, nhorman, npmccallum, haitao.huang,
andriy.shevchenko, tglx, kai.svahn, bp, josh, luto, kai.huang,
rientjes, cedric.xing, puiterwijk, linux-security-module,
Suresh Siddha, Haitao Huang
In-Reply-To: <6AE5891F-FC0D-4062-A6CA-01C78C2D5A1A@amacapital.net>
On Thu, Feb 20, 2020 at 04:32:22PM -0800, Andy Lutomirski wrote:
> > On Feb 20, 2020, at 2:16 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 10:48:42AM -0800, Sean Christopherson wrote:
> >> My biggest concern for allowing PROT_EXEC if RIE is that it would result
> >> in #PF(SGX) (#GP on Skylake) due to an EPCM violation if the enclave
> >> actually tried to execute from such a page. This isn't a problem for the
> >> kernel as the fault will be reported cleanly through the vDSO (or get
> >> delivered as a SIGSEGV if the enclave isn't entered through the vDSO), but
> >> it's a bit weird for userspace as userspace will see the #PF(SGX) and
> >> likely assume the EPC was lost, e.g. silently restart the enclave instead
> >> of logging an error that the enclave is broken.
> >
> > I think right way to fix the current implementation is to -EACCES mmap()
> > (and mprotect) when !!(current->personality & READ_IMPLIES_EXEC).
> >
> > This way supporting RIE can be reconsidered later on without any
> > potential ABI bottlenecks.
> >
>
> Sounds good to me. I see no credible reason why anyone would use RIE and SGX.
Great, thanks Andy.
/Jarkko
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-02-21 12:53 UTC (permalink / raw)
To: Jordan Hand
Cc: Sean Christopherson, linux-kernel, x86, linux-sgx, akpm,
dave.hansen, nhorman, npmccallum, haitao.huang, andriy.shevchenko,
tglx, kai.svahn, bp, josh, luto, kai.huang, rientjes, cedric.xing,
puiterwijk, linux-security-module, Suresh Siddha, Haitao Huang
In-Reply-To: <2c077197-a8a7-feac-58ea-e901c92fb58b@linux.microsoft.com>
On Thu, Feb 20, 2020 at 04:11:04PM -0800, Jordan Hand wrote:
> On 2/20/20 2:16 PM, Jarkko Sakkinen wrote:
> > On Thu, Feb 20, 2020 at 10:48:42AM -0800, Sean Christopherson wrote:
> >> My biggest concern for allowing PROT_EXEC if RIE is that it would result
> >> in #PF(SGX) (#GP on Skylake) due to an EPCM violation if the enclave
> >> actually tried to execute from such a page. This isn't a problem for the
> >> kernel as the fault will be reported cleanly through the vDSO (or get
> >> delivered as a SIGSEGV if the enclave isn't entered through the vDSO), but
> >> it's a bit weird for userspace as userspace will see the #PF(SGX) and
> >> likely assume the EPC was lost, e.g. silently restart the enclave instead
> >> of logging an error that the enclave is broken.
> >
> > I think right way to fix the current implementation is to -EACCES mmap()
> > (and mprotect) when !!(current->personality & READ_IMPLIES_EXEC).
> >
>
> I agree. It still means userspace code with an executable stack can't
> mmap/mprotect enclave pages and request PROT_READ but the check you've
> proposed would more consistently enforce this which is easier to
> understand from userspace perspective.
Thank you. Your observation was really important because having half
working RIE support hanging around would only have potential to cause
unnecessary maintenance burden. It would even make adding a legit RIE
support later on somewhat more difficult.
I updated the commit under discussion in my tree [*] with a fix that
adds the following to the beginning of sgx_encl_may_map():
/*
* Disallow RIE tasks as their VMA permissions might conflict with the
* enclave page permissions.
*/
if (!!(current->personality & READ_IMPLIES_EXEC))
return -EACCES;
[*] https://github.com/jsakkine-intel/linux-sgx.git
/Jarkko
^ permalink raw reply
* Re: [PATCH bpf-next v4 5/8] bpf: lsm: Implement attach, detach and execution
From: KP Singh @ 2020-02-21 12:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, open list, bpf, Linux Security Module list,
Daniel Borkmann, James Morris, Kees Cook, Jann Horn,
David S. Miller, Greg Kroah-Hartman
In-Reply-To: <20200221021755.3z7ifyyeh6seo3zs@ast-mbp>
On 20-Feb 18:17, Alexei Starovoitov wrote:
> On Thu, Feb 20, 2020 at 06:52:47PM +0100, KP Singh wrote:
> > +
> > + /* This is the first program to be attached to the LSM hook, the hook
> > + * needs to be enabled.
> > + */
> > + if (prog->type == BPF_PROG_TYPE_LSM && tr->progs_cnt[kind] == 1)
> > + err = bpf_lsm_set_enabled(prog->aux->attach_func_name, true);
> > out:
> > mutex_unlock(&tr->mutex);
> > return err;
> > @@ -336,7 +348,11 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
> > }
> > hlist_del(&prog->aux->tramp_hlist);
> > tr->progs_cnt[kind]--;
> > - err = bpf_trampoline_update(prog->aux->trampoline);
> > + err = bpf_trampoline_update(prog);
> > +
> > + /* There are no more LSM programs, the hook should be disabled */
> > + if (prog->type == BPF_PROG_TYPE_LSM && tr->progs_cnt[kind] == 0)
> > + err = bpf_lsm_set_enabled(prog->aux->attach_func_name, false);
>
> Overall looks good, but I don't think above logic works.
> Consider lsm being attached, then fexit, then lsm detached, then fexit detached.
> Both are kind==fexit and static_key stays enabled.
You're right. I was weary of introducing a new kind (something like
BPF_TRAMP_LSM) since they are just fexit trampolines. For now, I
added nr_lsm_progs as a member in struct bpf_trampoline and refactored
the increment and decrement logic into inline helper functions e.g.
static inline void bpf_trampoline_dec_progs(struct bpf_prog *prog,
enum bpf_tramp_prog_type kind)
{
struct bpf_trampoline *tr = prog->aux->trampoline;
if (prog->type == BPF_PROG_TYPE_LSM)
tr->nr_lsm_progs--;
tr->progs_cnt[kind]--;
}
and doing the check as:
if (prog->type == BPF_PROG_TYPE_LSM && tr->nr_lsm_progs == 0)
err = bpf_lsm_set_enabled(prog->aux->attach_func_name, false);
This should work, If you're okay with it, I will update it in the next
revision of the patch-set.
- KP
^ 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