* [RFC PATCH v2 2/5] x86/sgx: Require userspace to define enclave pages' protection bits
From: Sean Christopherson @ 2019-06-06 2:11 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-1-sean.j.christopherson@intel.com>
Existing Linux Security Module policies restrict userspace's ability to
map memory, e.g. may require priveleged permissions to map a page that
is simultaneously writable and executable. Said permissions are often
tied to the file which backs the mapped memory, i.e. vm_file.
For reasons explained below, SGX does not allow LSMs to enforce policies
using existing LSM hooks such as file_mprotect(). Explicitly track the
protection bits for an enclave page (separate from the vma/pte bits) and
require userspace to explicit define each page's protection bit when the
page is added to the enclave. Enclave page protection bits pave the way
adding security_enclave_load() as an SGX equivalent to file_mprotect(),
e.g. SGX can pass the page's protection bits and source vma to LSMs.
The source vma will allow LSMs to tie permissions to files, e.g. the
file containing the enclave's code and initial data, and the protection
bits will allow LSMs to make decisions based on the capabilities of the
enclave, e.g. if a page can be converted from RW to RX.
Due to the nature of the Enclave Page Cache, and because the EPC is
manually managed by SGX, all enclave vmas are backed by the same file,
i.e. /dev/sgx/enclave. Specifically, a single file allows SGX to use
file op hooks to move pages in/out of the EPC.
Furthermore, EPC pages for any given enclave are fundamentally shared
between processes, i.e. CoW semantics are not possible with EPC pages
due to hardware restrictions such as 1:1 mappings between virtual and
physical addresses (within the enclave).
Lastly, all real world enclaves will need read, write and execute
permissions to EPC pages.
As a result, SGX does not play nice with existing LSM behavior as it is
impossible to apply policies to enclaves with reasonable granularity,
e.g. an LSM can deny access to EPC altogether, but can't deny
potentially unwanted behavior such as mapping pages RW->RW or RWX.
For example, because all (practical) enclaves need RW pages for data and
RX pages for code, SELinux's existing policies will require all enclaves
to have FILE__READ, FILE__WRITE and FILE__EXECUTE permissions on
/dev/sgx/enclave. Witholding FILE__WRITE or FILE__EXECUTE in an attempt
to deny RW->RX or RWX would prevent running *any* enclave, even those
that cleanly separate RW and RX pages. And because /dev/sgx/enclave
requires MAP_SHARED, the anonymous/CoW checks that would trigger
FILE__EXECMOD or PROCESS__EXECMEM permissions will never fire.
Taking protection bits has a second use in that it can be used to
prevent loading an enclave from a noexec file system. On SGX2 hardware,
regardless of kernel support for SGX2, userspace could EADD a page from
a noexec path using read-only permissions and later mprotect() and
ENCLU[EMODPE] the page to gain execute permissions. By requiring
the enclave's page protections up front, SGX will be able to enforce
noexec paths when building enclaves.
To prevent userspace from circumventing the allowed protections, do not
allow PROT_{READ,WRITE,EXEC} mappings to an enclave without an
associated enclave page, i.e. prevent creating a mapping with unchecked
protection bits.
Alternatively, SGX could pre-check what transitions are/aren't allowed
using some form of proxy for the enclave, e.g. its sigstruct, and
dynamically track protections in the SGX driver. Dynamically tracking
protections and pre-checking permissions has several drawbacks:
- Complicates the SGX implementation due to the need to coordinate
tracking across multiple mm structs and vmas.
- LSM auditing would log denials that never manifest in failure.
- Requires additional SGX specific flags/definitions be passed to/from
LSMs.
A second alternative would be to again use sigstruct as a proxy for the
enclave when performing access control checks, but hold a reference to
the sigstruct file and perform LSM checks during mmap()/mmprotect() as
opposed to pre-checking permissions at enclave build time. The big
downside to this approach is that it effecitvely requires userspace to
place sigstruct in a file, and the SGX driver must "pin" said file by
holding a reference to the file for the lifetime of the enclave.
A third alternative would be to pull the protection bits from the page's
SECINFO, i.e. make decisions based on the protections enforced by
hardware. However, with SGX2, userspace can extend the hardware-
enforced protections via ENCLU[EMODPE], e.g. can add a page as RW and
later convert it to RX. With SGX2, making a decision based on the
initial protections would either create a security hole or force SGX to
dynamically track "dirty" pages (see first alternative above).
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/include/uapi/asm/sgx.h | 2 +
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 +++++--
arch/x86/kernel/cpu/sgx/driver/main.c | 5 +++
arch/x86/kernel/cpu/sgx/encl.c | 53 ++++++++++++++++++++++++++
arch/x86/kernel/cpu/sgx/encl.h | 4 ++
5 files changed, 74 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 9ed690a38c70..2c6198ffeaf8 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -37,12 +37,14 @@ struct sgx_enclave_create {
* @addr: address within the ELRANGE
* @src: address for the page data
* @secinfo: address for the SECINFO data
+ * @flags: flags, e.g. PROT_{READ,WRITE,EXEC}
* @mrmask: bitmask for the measured 256 byte chunks
*/
struct sgx_enclave_add_page {
__u64 addr;
__u64 src;
__u64 secinfo;
+ __u32 flags;
__u16 mrmask;
} __attribute__((__packed__));
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index a27ec26a9350..ef5c2ce0f37b 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -235,7 +235,8 @@ static int sgx_validate_secs(const struct sgx_secs *secs,
}
static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
- unsigned long addr)
+ unsigned long addr,
+ unsigned long prot)
{
struct sgx_encl_page *encl_page;
int ret;
@@ -247,6 +248,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
return ERR_PTR(-ENOMEM);
encl_page->desc = addr;
encl_page->encl = encl;
+ encl_page->prot = prot;
ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
encl_page);
if (ret) {
@@ -531,7 +533,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
void *data, struct sgx_secinfo *secinfo,
- unsigned int mrmask)
+ unsigned int mrmask, unsigned long prot)
{
u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
struct sgx_encl_page *encl_page;
@@ -557,7 +559,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
goto out;
}
- encl_page = sgx_encl_page_alloc(encl, addr);
+ encl_page = sgx_encl_page_alloc(encl, addr, prot);
if (IS_ERR(encl_page)) {
ret = PTR_ERR(encl_page);
goto out;
@@ -599,6 +601,7 @@ static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd,
struct sgx_enclave_add_page *addp = (void *)arg;
struct sgx_encl *encl = filep->private_data;
struct sgx_secinfo secinfo;
+ unsigned long prot;
struct page *data_page;
void *data;
int ret;
@@ -618,7 +621,10 @@ static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd,
goto out;
}
- ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask);
+ prot = addp->flags & (PROT_READ | PROT_WRITE | PROT_EXEC);
+
+ ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask,
+ prot);
if (ret)
goto out;
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 129d356aff30..65a87c2fdf02 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -63,6 +63,11 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
{
struct sgx_encl *encl = file->private_data;
+ int ret;
+
+ ret = sgx_map_allowed(encl, vma->vm_start, vma->vm_end, vma->vm_flags);
+ if (ret)
+ return ret;
vma->vm_ops = &sgx_vm_ops;
vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 7216bdf07bd0..a5a412220058 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -235,6 +235,58 @@ static void sgx_vma_close(struct vm_area_struct *vma)
kref_put(&encl->refcount, sgx_encl_release);
}
+
+/**
+ * sgx_map_allowed - check vma protections against the associated enclave page
+ * @encl: an enclave
+ * @start: start address of the mapping (inclusive)
+ * @end: end address of the mapping (exclusive)
+ * @prot: protection bits of the mapping
+ *
+ * Verify a userspace mapping to an enclave page would not violate the security
+ * requirements of the *kernel*. Note, this is in no way related to the
+ * page protections enforced by hardware via the EPCM. The EPCM protections
+ * can be directly extended by the enclave, i.e. cannot be relied upon by the
+ * kernel for security guarantees of any kind.
+ *
+ * Return:
+ * 0 on success,
+ * -EACCES if the mapping is disallowed
+ */
+int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
+ unsigned long end, unsigned long prot)
+{
+ struct sgx_encl_page *page;
+ unsigned long addr;
+
+ prot &= (VM_READ | VM_WRITE | VM_EXEC);
+ if (!prot || !encl)
+ return 0;
+
+ mutex_lock(&encl->lock);
+
+ for (addr = start; addr < end; addr += PAGE_SIZE) {
+ page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
+
+ /*
+ * Do not allow R|W|X to a non-existent page, or protections
+ * beyond those of the existing enclave page.
+ */
+ if (!page || (prot & ~page->prot))
+ return -EACCES;
+ }
+
+ mutex_unlock(&encl->lock);
+
+ return 0;
+}
+
+static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
+ unsigned long end, unsigned long prot)
+{
+ return sgx_map_allowed(vma->vm_private_data, start, end, prot);
+}
+
static unsigned int sgx_vma_fault(struct vm_fault *vmf)
{
unsigned long addr = (unsigned long)vmf->address;
@@ -372,6 +424,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
const struct vm_operations_struct sgx_vm_ops = {
.close = sgx_vma_close,
.open = sgx_vma_open,
+ .may_mprotect = sgx_vma_mprotect,
.fault = sgx_vma_fault,
.access = sgx_vma_access,
};
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index c557f0374d74..176467c0eb22 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -41,6 +41,7 @@ enum sgx_encl_page_desc {
struct sgx_encl_page {
unsigned long desc;
+ unsigned long prot;
struct sgx_epc_page *epc_page;
struct sgx_va_page *va_page;
struct sgx_encl *encl;
@@ -106,6 +107,9 @@ static inline unsigned long sgx_pcmd_offset(pgoff_t page_index)
sizeof(struct sgx_pcmd);
}
+int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
+ unsigned long end, unsigned long prot);
+
enum sgx_encl_mm_iter {
SGX_ENCL_MM_ITER_DONE = 0,
SGX_ENCL_MM_ITER_NEXT = 1,
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v2 5/5] security/selinux: Add enclave_load() implementation
From: Sean Christopherson @ 2019-06-06 2:11 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-1-sean.j.christopherson@intel.com>
The goal of selinux_enclave_load() is to provide a facsimile of the
existing selinux_file_mprotect() and file_map_prot_check() policies,
but tailored to the unique properties of SGX.
For example, an enclave page is technically backed by a MAP_SHARED file,
but the "file" is essentially shared memory that is never persisted
anywhere and also requires execute permissions (for some pages).
The basic concept is to require appropriate execute permissions on the
source of the enclave for pages that are requesting PROT_EXEC, e.g. if
an enclave page is being loaded from a regular file, require
FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
anonymous/private mapping, require PROCESS__EXECMEM since the process
is essentially executing from the mapping, albeit in a roundabout way.
Note, FILE__READ and FILE__WRITE are intentionally not required even if
the source page is backed by a regular file. Writes to the enclave page
are contained to the EPC, i.e. never hit the original file, and read
permissions have already been vetted (or the VMA doesn't have PROT_READ,
in which case loading the page into the enclave will fail).
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
security/selinux/hooks.c | 69 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702cf46ca..3c5418edf51c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6726,6 +6726,71 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
}
#endif
+#ifdef CONFIG_INTEL_SGX
+int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)
+{
+ const struct cred *cred = current_cred();
+ u32 sid = cred_sid(cred);
+ int ret;
+
+ /* SGX is supported only in 64-bit kernels. */
+ WARN_ON_ONCE(!default_noexec);
+
+ /* Only executable enclave pages are restricted in any way. */
+ if (!(prot & PROT_EXEC))
+ return 0;
+
+ /*
+ * The source page is exectuable, i.e. has already passed SELinux's
+ * checks, and userspace is not requesting RW->RX capabilities.
+ */
+ if ((vma->vm_flags & VM_EXEC) && !(prot & PROT_WRITE))
+ return 0;
+
+ /*
+ * The source page is not executable, or userspace is requesting the
+ * ability to do a RW->RX conversion. Permissions are required as
+ * follows, in order of increasing privelege:
+ *
+ * EXECUTE - Load an executable enclave page without RW->RX intent from
+ * a non-executable vma that is backed by a shared mapping to
+ * a regular file that has not undergone COW.
+ *
+ * EXECMOD - Load an executable enclave page without RW->RX intent from
+ * a non-executable vma that is backed by a shared mapping to
+ * a regular file that *has* undergone COW.
+ *
+ * - Load an enclave page *with* RW->RX intent from a shared
+ * mapping to a regular file.
+ *
+ * EXECMEM - Load an exectuable enclave page from an anonymous mapping.
+ *
+ * - Load an exectuable enclave page from a private file, e.g.
+ * from a shared mapping to a hugetlbfs file.
+ *
+ * - Load an enclave page *with* RW->RX intent from a private
+ * mapping to a regular file.
+ *
+ * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and
+ * reflects the nature of enclaves and the EPC, e.g. EPC is effectively
+ * a non-persistent shared file, but each enclave is a private domain
+ * within that shared file, so delegate to the source of the enclave.
+ */
+ if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file) &&
+ ((vma->vm_flags & VM_SHARED) || !(prot & PROT_WRITE)))) {
+ if (!vma->anon_vma && !(prot & PROT_WRITE))
+ ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
+ else
+ ret = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
+ } else {
+ ret = avc_has_perm(&selinux_state,
+ sid, sid, SECCLASS_PROCESS,
+ PROCESS__EXECMEM, NULL);
+ }
+ return ret;
+}
+#endif
+
struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct task_security_struct),
.lbs_file = sizeof(struct file_security_struct),
@@ -6968,6 +7033,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
#endif
+
+#ifdef CONFIG_INTEL_SGX
+ LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
+#endif
};
static __init int selinux_init(void)
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v2 3/5] x86/sgx: Enforce noexec filesystem restriction for enclaves
From: Sean Christopherson @ 2019-06-06 2:11 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-1-sean.j.christopherson@intel.com>
Do not allow an enclave page to be mapped with PROT_EXEC if the source
vma does not have VM_MAYEXEC. This effectively enforces noexec as
do_mmap() clears VM_MAYEXEC if the vma is being loaded from a noexec
path, i.e. prevents executing a file by loading it into an enclave.
Checking noexec indirectly by way of VM_MAYEXEC naturally handles any
other cases that clear VM_MAYEXEC to deny execute permissions.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 47 +++++++++++++++++++++++---
1 file changed, 42 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index ef5c2ce0f37b..44b2d73de7c3 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -577,6 +577,44 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
return ret;
}
+static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
+{
+ struct vm_area_struct *vma;
+ int ret;
+
+ if (!(prot & VM_EXEC))
+ return 0;
+
+ /* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */
+ down_read(¤t->mm->mmap_sem);
+
+ vma = find_vma(current->mm, src);
+ if (!vma) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ /*
+ * Query VM_MAYEXEC as an indirect path_noexec() check (see do_mmap()),
+ * but with some future proofing against other cases that may deny
+ * execute permissions.
+ */
+ if (!(vma->vm_flags & VM_MAYEXEC)) {
+ ret = -EACCES;
+ goto out;
+ }
+
+ if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
+ ret = -EFAULT;
+ else
+ ret = 0;
+
+out:
+ up_read(¤t->mm->mmap_sem);
+
+ return ret;
+}
+
/**
* sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
*
@@ -616,13 +654,12 @@ static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd,
data = kmap(data_page);
- if (copy_from_user((void *)data, (void __user *)addp->src, PAGE_SIZE)) {
- ret = -EFAULT;
- goto out;
- }
-
prot = addp->flags & (PROT_READ | PROT_WRITE | PROT_EXEC);
+ ret = sgx_encl_page_copy(data, addp->src, prot);
+ if (ret)
+ goto out;
+
ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask,
prot);
if (ret)
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v2 4/5] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Sean Christopherson @ 2019-06-06 2:11 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190606021145.12604-1-sean.j.christopherson@intel.com>
enclave_load() is roughly analogous to the existing file_mprotect().
Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
MAP_SHARED. Furthermore, all enclaves need read, write and execute
VMAs. As a result, the existing/standard call to file_mprotect() does
not provide any meaningful security for enclaves since an LSM can only
deny/grant access to the EPC as a whole.
security_enclave_load() is called when SGX is first loading an enclave
page, i.e. copying a page from normal memory into the EPC. Although
the prototype for enclave_load() is similar to file_mprotect(), e.g.
SGX could theoretically use file_mprotect() and set reqprot=prot, a
separate hook is desirable as the semantics of an enclave's protection
bits are different than those of vmas, e.g. an enclave page tracks the
maximal set of protections, whereas file_mprotect() operates on the
actual protections being provided. In other words, LSMs will likely
want to implement different policies for enclave page protections.
Note, extensive discussion yielded no sane alternative to some form of
SGX specific LSM hook[1].
[1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 12 ++++++------
include/linux/lsm_hooks.h | 13 +++++++++++++
include/linux/security.h | 12 ++++++++++++
security/security.c | 7 +++++++
4 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 44b2d73de7c3..29c0df672250 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -8,6 +8,7 @@
#include <linux/highmem.h>
#include <linux/ratelimit.h>
#include <linux/sched/signal.h>
+#include <linux/security.h>
#include <linux/shmem_fs.h>
#include <linux/slab.h>
#include <linux/suspend.h>
@@ -582,9 +583,6 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
struct vm_area_struct *vma;
int ret;
- if (!(prot & VM_EXEC))
- return 0;
-
/* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */
down_read(¤t->mm->mmap_sem);
@@ -599,15 +597,17 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
* but with some future proofing against other cases that may deny
* execute permissions.
*/
- if (!(vma->vm_flags & VM_MAYEXEC)) {
+ if ((prot & VM_EXEC) && !(vma->vm_flags & VM_MAYEXEC)) {
ret = -EACCES;
goto out;
}
+ ret = security_enclave_load(vma, prot);
+ if (ret)
+ goto out;
+
if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
ret = -EFAULT;
- else
- ret = 0;
out:
up_read(¤t->mm->mmap_sem);
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..c6f47a7eef70 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1446,6 +1446,12 @@
* @bpf_prog_free_security:
* Clean up the security information stored inside bpf prog.
*
+ * Security hooks for Intel SGX enclaves.
+ *
+ * @enclave_load:
+ * @vma: the source memory region of the enclave page being loaded.
+ * @prot: the (maximal) protections of the enclave page.
+ * Return 0 if permission is granted.
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1807,6 +1813,10 @@ union security_list_options {
int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
#endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+ int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot);
+#endif /* CONFIG_INTEL_SGX */
};
struct security_hook_heads {
@@ -2046,6 +2056,9 @@ struct security_hook_heads {
struct hlist_head bpf_prog_alloc_security;
struct hlist_head bpf_prog_free_security;
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+ struct hlist_head enclave_load;
+#endif /* CONFIG_INTEL_SGX */
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..0b6d1eb7368b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1829,5 +1829,17 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
#endif /* CONFIG_SECURITY */
#endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+#ifdef CONFIG_SECURITY
+int security_enclave_load(struct vm_area_struct *vma, unsigned long prot);
+#else
+static inline int security_enclave_load(struct vm_area_struct *vma,
+ unsigned long prot)
+{
+ return 0;
+}
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_INTEL_SGX */
+
#endif /* ! __LINUX_SECURITY_H */
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..c6f7f26969b2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
call_void_hook(bpf_prog_free_security, aux);
}
#endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+int security_enclave_load(struct vm_area_struct *vma, unsigned long prot)
+{
+ return call_int_hook(enclave_load, 0, vma, prot);
+}
+#endif /* CONFIG_INTEL_SGX */
--
2.21.0
^ permalink raw reply related
* Re: KASAN: use-after-free Read in tomoyo_realpath_from_path
From: Tetsuo Handa @ 2019-06-06 2:08 UTC (permalink / raw)
To: Al Viro, linux-fsdevel
Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
syzkaller-bugs, takedakn
In-Reply-To: <0000000000004f43fa058a97f4d3@google.com>
Here is a reproducer.
The problem is that TOMOYO is accessing already freed socket from security_file_open()
which later fails with -ENXIO (because we can't get file descriptor of sockets via
/proc/pid/fd/n interface), and the file descriptor is getting released before
security_file_open() completes because we do not raise "struct file"->f_count of
the file which is accessible via /proc/pid/fd/n interface. We can avoid this problem
if we can avoid calling security_file_open() which after all fails with -ENXIO.
How should we handle this race? Let LSM modules check if security_file_open() was
called on a socket?
----------------------------------------
diff --git a/fs/open.c b/fs/open.c
index b5b80469b93d..995ffcb37128 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -765,6 +765,12 @@ static int do_dentry_open(struct file *f,
error = security_file_open(f);
if (error)
goto cleanup_all;
+ if (!strcmp(current->comm, "a.out") &&
+ f->f_path.dentry->d_sb->s_magic == SOCKFS_MAGIC) {
+ printk("Start open(socket) delay\n");
+ schedule_timeout_killable(HZ * 5);
+ printk("End open(socket) delay\n");
+ }
error = break_lease(locks_inode(f), f->f_flags);
if (error)
----------------------------------------
----------------------------------------
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/socket.h>
int main(int argc, char *argv[])
{
pid_t pid = getpid();
int fd = socket(AF_ISDN, SOCK_RAW, 0);
char buffer[128] = { };
if (fork() == 0) {
close(fd);
snprintf(buffer, sizeof(buffer) - 1, "/proc/%u/fd/%u", pid, fd);
open(buffer, 3);
_exit(0);
}
sleep(2);
close(fd);
return 0;
}
----------------------------------------
----------------------------------------
getpid() = 32504
socket(AF_ISDN, SOCK_RAW, 0) = 3
clone(strace: Process 32505 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7efea30dda10) = 32505
[pid 32504] rt_sigprocmask(SIG_BLOCK, [CHLD], <unfinished ...>
[pid 32505] close(3 <unfinished ...>
[pid 32504] <... rt_sigprocmask resumed> [], 8) = 0
[pid 32505] <... close resumed> ) = 0
[pid 32504] rt_sigaction(SIGCHLD, NULL, {SIG_DFL, [], 0}, 8) = 0
[pid 32505] open("/proc/32504/fd/3", O_ACCMODE <unfinished ...>
[pid 32504] rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[pid 32504] nanosleep({2, 0}, 0x7ffd3c608150) = 0
[pid 32504] close(3) = 0
[pid 32504] exit_group(0) = ?
[pid 32504] +++ exited with 0 +++
<... open resumed> ) = -1 ENXIO (No such device or address)
exit_group(0) = ?
----------------------------------------
----------------------------------------
[ 95.109628] Start open(socket) delay
[ 97.113150] base_sock_release(00000000506a3239) sk=00000000016d0ceb
[ 100.142235] End open(socket) delay
----------------------------------------
^ permalink raw reply related
* Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Sean Christopherson @ 2019-06-06 2:04 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, Jarkko Sakkinen, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654EDB7D@ORSMSX116.amr.corp.intel.com>
On Tue, Jun 04, 2019 at 02:43:09PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Tuesday, June 04, 2019 1:37 PM
> >
> > On Tue, Jun 04, 2019 at 01:29:10PM -0700, Andy Lutomirski wrote:
> > > On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > > > static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> > > > addr, diff --git a/include/linux/lsm_hooks.h
> > > > b/include/linux/lsm_hooks.h index 47f58cfb6a19..0562775424a0 100644
> > > > --- a/include/linux/lsm_hooks.h
> > > > +++ b/include/linux/lsm_hooks.h
> > > > @@ -1446,6 +1446,14 @@
> > > > * @bpf_prog_free_security:
> > > > * Clean up the security information stored inside bpf prog.
> > > > *
> > > > + * Security hooks for Intel SGX enclaves.
> > > > + *
> > > > + * @enclave_load:
> > > > + * On success, returns 0 and optionally adjusts @allowed_prot
> > > > + * @vma: the source memory region of the enclave page being
> > loaded.
> > > > + * @prot: the initial protection of the enclave page.
> > >
> > > What do you mean "initial"? The page is always mapped PROT_NONE when
> > > this is called, right? I feel like I must be missing something here.
> >
> > Initial protection in the EPCM. Yet another reason to ignore SECINFO.
>
> I know you guys are talking in the background that all pages are mmap()'ed
> PROT_NONE. But that's an unnecessary limitation.
Not all pages have to be mmap()'d PROT_NONE, only pages that do not have
an associated enclave page.
> And @prot here should be @target_vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC).
I don't follow, there is no target_vma at this point.
^ permalink raw reply
* Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves
From: Sean Christopherson @ 2019-06-06 1:01 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190605151006.GI11331@linux.intel.com>
On Wed, Jun 05, 2019 at 06:10:18PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 04, 2019 at 01:25:10PM -0700, Andy Lutomirski wrote:
> > On Tue, Jun 4, 2019 at 9:26 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> > > > Do not allow an enclave page to be mapped with PROT_EXEC if the source
> > > > page is backed by a file on a noexec file system.
> > > >
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > >
> > > Why don't you just check in sgx_encl_add_page() that whether the path
> > > comes from noexec and deny if SECINFO contains X?
> > >
> >
> > SECINFO seems almost entirely useless for this kind of thing because
> > of SGX2. I'm thinking that SECINFO should be completely ignored for
> > anything other than its required architectural purpose.
>
> Not exactly sure why using it to pass the RWX bits to EADD ioctl would
> cause anything to SGX2 support.
Andy was pointing out that with SGX2 the enclave can do ENCLU[EMODPE] to
make the page executable, e.g. add the page with SECINFO.R and then
mprotect() the enclave VMA (whose vm_file == /dev/sgx/enclave) PROT_EXEC.
We could hard enforce SECINFO, i.e. set the enclave page's protection bits
directly from SECINFO, but that would neuter SGX2, e.g. would break
converting RW to RX.
^ permalink raw reply
* Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
From: Sean Christopherson @ 2019-06-05 23:58 UTC (permalink / raw)
To: Ayoun, Serge
Cc: Andy Lutomirski, Xing, Cedric, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Katz-zamir, Shay, Huang, Haitao,
Jarkko Sakkinen, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <88B7642769729B409B4A93D7C5E0C5E7C64475FB@hasmsx108.ger.corp.intel.com>
On Wed, Jun 05, 2019 at 04:10:44AM -0700, Ayoun, Serge wrote:
> > From: Christopherson, Sean J
> > Sent: Saturday, June 01, 2019 02:32
> >
> > /**
> > * struct sgx_enclave_add_pages - parameter structure for the
> > * %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> > @@ -39,6 +44,7 @@ struct sgx_enclave_create {
> > * @secinfo: address for the SECINFO data (common to all pages)
> > * @nr_pages: number of pages (must be virtually contiguous)
> > * @mrmask: bitmask for the measured 256 byte chunks (common to all
> > pages)
> > + * @flags: flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all
> > pages)
> > */
> > struct sgx_enclave_add_pages {
> > __u64 addr;
> > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
> > __u64 secinfo;
> > __u32 nr_pages;
> > __u16 mrmask;
> > -} __attribute__((__packed__));
> > + __u16 flags;
> > +};
>
> You are adding a flags member. The secinfo structure has already a flags member in it.
> Why do you need both - they are both coming from user mode. What kind of scenario would
> require having different values. Seems confusing.
The format of SECINFO.FLAGS is hardware defined, e.g. we can't add a flag
to tag the page as being a zero page for optimization purposes, at least
not without breaking future compatibility or doing tricky overloading.
If you're specifically talking about SECINFO.FLAGS.RWX, due to SGX2 there
are scenarios where userspace will initially want the page to be RW, and
will later want to convert the page to RX. Making decisions based solely
on the initial EPCM permissions would either create a security hole or
force SGX to track "dirty" pages along with a implementing a pre-check
scheme for LSMs (or restricting LSMs to tieing permissions to the host
process and not the enclave).
^ permalink raw reply
* Re: [PATCH 00/58] LSM: Module stacking for AppArmor
From: James Morris @ 2019-06-05 22:28 UTC (permalink / raw)
To: John Johansen
Cc: Stephen Smalley, Casey Schaufler, casey.schaufler,
linux-security-module, selinux, keescook, penguin-kernel, paul
In-Reply-To: <c27ef338-e8fc-cf60-41ed-3d74352add3d@canonical.com>
On Wed, 5 Jun 2019, John Johansen wrote:
> This does rely on apparmor doing its own namespacing and bounding. LSM
> stacking just allows us to start doing this with apparmor containers
> on smack and selinux based systems.
Ahh, ok, I thought you were using an LSM stack for each container.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: KASAN: use-after-free Read in tomoyo_realpath_from_path
From: Tetsuo Handa @ 2019-06-05 22:09 UTC (permalink / raw)
To: Alexander Viro
Cc: syzbot, jmorris, linux-kernel, linux-security-module, serge,
syzkaller-bugs, takedakn, David S. Miller
In-Reply-To: <0000000000004f43fa058a97f4d3@google.com>
Hello, Al.
syzbot found that SOCKET_I(d_backing_inode("struct path"->dentry))->sk
was already kfree()d when trying to calculate pathname for open().
"struct path"->dentry should remain valid but portion of memory
reachable via inode already became invalid. What should we do?
On 2019/06/06 3:42, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 788a0249 Merge tag 'arc-5.2-rc4' of git://git.kernel.org/p..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=179848d4a00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=60564cb52ab29d5b
> dashboard link: https://syzkaller.appspot.com/bug?extid=0341f6a4d729d4e0acf1
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13ac35baa00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: use-after-free in tomoyo_get_socket_name security/tomoyo/realpath.c:238 [inline]
> BUG: KASAN: use-after-free in tomoyo_realpath_from_path+0x722/0x7a0 security/tomoyo/realpath.c:284
> Read of size 2 at addr ffff8880a91276d0 by task syz-executor.3/17397
>
> CPU: 0 PID: 17397 Comm: syz-executor.3 Not tainted 5.2.0-rc3+ #12
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> print_address_description.cold+0x7c/0x20d mm/kasan/report.c:188
> __kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317
> kasan_report+0x12/0x20 mm/kasan/common.c:614
> __asan_report_load2_noabort+0x14/0x20 mm/kasan/generic_report.c:130
> tomoyo_get_socket_name security/tomoyo/realpath.c:238 [inline]
> tomoyo_realpath_from_path+0x722/0x7a0 security/tomoyo/realpath.c:284
> tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
> tomoyo_check_open_permission+0x2a8/0x3f0 security/tomoyo/file.c:771
> tomoyo_file_open security/tomoyo/tomoyo.c:319 [inline]
> tomoyo_file_open+0xa9/0xd0 security/tomoyo/tomoyo.c:314
> security_file_open+0x71/0x300 security/security.c:1454
> do_dentry_open+0x373/0x1250 fs/open.c:765
> vfs_open+0xa0/0xd0 fs/open.c:887
> do_last fs/namei.c:3416 [inline]
> path_openat+0x10e9/0x46d0 fs/namei.c:3533
> do_filp_open+0x1a1/0x280 fs/namei.c:3563
> do_sys_open+0x3fe/0x5d0 fs/open.c:1070
> __do_sys_open fs/open.c:1088 [inline]
> __se_sys_open fs/open.c:1083 [inline]
> __x64_sys_open+0x7e/0xc0 fs/open.c:1083
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x413161
> Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 19 00 00 c3 48 83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f 05 <48> 8b 3c 24 48 89 c2 e8 53 fa ff ff 48 89 d0 48 83 c4 08 48 3d 01
> RSP: 002b:00007f65230f8bb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000002
> RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 0000000000413161
> RDX: fffffffffffffffa RSI: 0000000000000000 RDI: 00007f65230f8bd0
> RBP: 000000000075c060 R08: 0000000000000050 R09: 000000000000000f
> R10: 0000000000000004 R11: 0000000000000293 R12: 00007f65230f96d4
> R13: 00000000004c83f6 R14: 00000000004dea40 R15: 00000000ffffffff
>
> Allocated by task 17373:
> save_stack+0x23/0x90 mm/kasan/common.c:71
> set_track mm/kasan/common.c:79 [inline]
> __kasan_kmalloc mm/kasan/common.c:489 [inline]
> __kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:462
> kasan_kmalloc+0x9/0x10 mm/kasan/common.c:503
> __do_kmalloc mm/slab.c:3660 [inline]
> __kmalloc+0x15c/0x740 mm/slab.c:3669
> kmalloc include/linux/slab.h:552 [inline]
> sk_prot_alloc+0x19c/0x2e0 net/core/sock.c:1602
> sk_alloc+0x39/0xf70 net/core/sock.c:1656
> base_sock_create drivers/isdn/mISDN/socket.c:758 [inline]
> mISDN_sock_create+0xb4/0x3a0 drivers/isdn/mISDN/socket.c:780
> __sock_create+0x3d8/0x730 net/socket.c:1424
> sock_create net/socket.c:1475 [inline]
> __sys_socket+0x103/0x220 net/socket.c:1517
> __do_sys_socket net/socket.c:1526 [inline]
> __se_sys_socket net/socket.c:1524 [inline]
> __x64_sys_socket+0x73/0xb0 net/socket.c:1524
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Freed by task 17371:
> save_stack+0x23/0x90 mm/kasan/common.c:71
> set_track mm/kasan/common.c:79 [inline]
> __kasan_slab_free+0x102/0x150 mm/kasan/common.c:451
> kasan_slab_free+0xe/0x10 mm/kasan/common.c:459
> __cache_free mm/slab.c:3432 [inline]
> kfree+0xcf/0x220 mm/slab.c:3755
> sk_prot_free net/core/sock.c:1639 [inline]
> __sk_destruct+0x4f7/0x6e0 net/core/sock.c:1725
> sk_destruct+0x7b/0x90 net/core/sock.c:1733
> __sk_free+0xce/0x300 net/core/sock.c:1744
> sk_free+0x42/0x50 net/core/sock.c:1755
> sock_put include/net/sock.h:1723 [inline]
> base_sock_release+0x269/0x279 drivers/isdn/mISDN/socket.c:628
> __sock_release+0xce/0x2a0 net/socket.c:601
> sock_close+0x1b/0x30 net/socket.c:1273
> __fput+0x2ff/0x890 fs/file_table.c:280
> ____fput+0x16/0x20 fs/file_table.c:313
> task_work_run+0x145/0x1c0 kernel/task_work.c:113
> tracehook_notify_resume include/linux/tracehook.h:185 [inline]
> exit_to_usermode_loop+0x273/0x2c0 arch/x86/entry/common.c:168
> prepare_exit_to_usermode arch/x86/entry/common.c:199 [inline]
> syscall_return_slowpath arch/x86/entry/common.c:279 [inline]
> do_syscall_64+0x58e/0x680 arch/x86/entry/common.c:304
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> The buggy address belongs to the object at ffff8880a91276c0
> which belongs to the cache kmalloc-2k of size 2048
> The buggy address is located 16 bytes inside of
> 2048-byte region [ffff8880a91276c0, ffff8880a9127ec0)
> The buggy address belongs to the page:
> page:ffffea0002a44980 refcount:1 mapcount:0 mapping:ffff8880aa400c40 index:0x0 compound_mapcount: 0
> flags: 0x1fffc0000010200(slab|head)
> raw: 01fffc0000010200 ffffea00022c9f88 ffffea0002234408 ffff8880aa400c40
> raw: 0000000000000000 ffff8880a91265c0 0000000100000003 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8880a9127580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8880a9127600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>> ffff8880a9127680: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
> ^
> ffff8880a9127700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8880a9127780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
^ permalink raw reply
* Re: [PATCH 00/58] LSM: Module stacking for AppArmor
From: John Johansen @ 2019-06-05 21:43 UTC (permalink / raw)
To: James Morris
Cc: Stephen Smalley, Casey Schaufler, casey.schaufler,
linux-security-module, selinux, keescook, penguin-kernel, paul
In-Reply-To: <alpine.LRH.2.21.1906060653060.20895@namei.org>
On 6/5/19 1:53 PM, James Morris wrote:
> On Tue, 4 Jun 2019, John Johansen wrote:
>
>> Yes, on Ubuntu & suse you can lauch lxd system containers with the
>> container having a system policy bounding the container, and the container
>> having its own apparmor policy namespace. So it loads and has its own
>> policy that is enforced.
>>
>> This allows for us to run older versions of ubuntu (say 16.04) on an
>> 18.04 host, and have the 16.04 policy behave just as if it was the host.
>
> How well does the LSM stacking scale to 100s or more containers?
>
Actually really well,
The cost isn't really based on how many containers but how many LSMs
are registered and how nested we are.
How we are currently handling it is apparmor is registered once, and
it is responsible for looping on its bounding. So for tasks that are
not in the container there is no additional cost.
For tasks in the first container, there is an extra cost of enforcing
the extra layer of apparmor policy loaded in the container. If you do
container in container there are two extra levels of apparmor policy.
This does rely on apparmor doing its own namespacing and bounding. LSM
stacking just allows us to start doing this with apparmor containers
on smack and selinux based systems.
>> This approach won't be an option for the 19.10 release and we will be
>> needing the full patchset. I should be able to provide some benchmark
>> and testing data soon.
>
> Great.
>
^ permalink raw reply
* Re: Rational model for UID based controls
From: David Howells @ 2019-06-05 21:06 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: dhowells, Andy Lutomirski, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <f19dcdb4-f934-34c2-f625-95c2c928d576@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> Right. You're mixing the kind of things that can generate events,
> and that makes having a single policy difficult.
Whilst that's true, the notifications are clearly marked as to type, so it
should be possible to select different policies for different notification
types.
Question for you: what does the LSM *actually* need? There are a bunch of
things available, some of which may be the same thing:
(1) The creds of the process that created a watch_queue (ie. opened
/dev/watch_queue).
(2) The creds of the process that set a watch (ie. called sb_notify,
KEYCTL_NOTIFY, ...);
(3) The creds of the process that tripped the event (which might be the
system).
(4) The security attributes of the object on which the watch was set (uid,
gid, mode, labels).
(5) The security attributes of the object on which the event was tripped.
(6) The security attributes of all the objects between the object in (5) and
the object in (4), assuming we work from (5) towards (4) if the two
aren't coincident (WATCH_INFO_RECURSIVE).
At the moment, when post_one_notification() wants to write a notification into
a queue, it calls security_post_notification() to ask if it should be allowed
to do so. This is passed (1) and (3) above plus the notification record.
The only problem I really have is that for a destruction message you want to
get the creds of who did the last put on an object and caused it to be
destroyed - I think everything else probably gets the right creds, even if
they aren't even in the same namespaces (mount propagation, yuck).
However, that one is a biggie because close()/exit() must propagate it to
deferred-fput, which must propagate it to af_unix-cleanup, and thence back to
deferred-fput and thence to implicit unmount (dissolve_on_fput()[*]).
[*] Though it should be noted that if this happens, the subtree cannot be
attached to the root of a namespace.
> > In any case, that's what I was referring to when I said I might need to call
> > inode_permission(). But UIDs don't exist for all filesystems, for example,
> > and there are no UIDs on superblocks, mount objects or hardware events.
>
> If you open() or stat() a file on those filesystems the UID
> used in the access control comes from somewhere. Setting a watch
> on things with UIDs should use the access mode on the file,
> just like any other filesystem operation.
Another question for you: Do I need to let the LSM pass judgement on a watch
that a process is trying to set? I think I probably do. This would require
separate hooks for different object types:
int security_watch_key(struct watch *watch, struct key *key);
int security_watch_sb(struct watch *watch, struct path *path);
int security_watch_mount(struct watch *watch, struct path *path);
int security_watch_devices(struct watch *watch);
so that the LSM can see the object the watch is being placed on (the last has
a global queue, so there is no object).
Further, do I need to put a "void *security" pointer in struct watch and
indicate to the LSM the object bring watched? The watch could then be passed
to security_post_notification() instead of the watch queue creds (which I
could then dispense with).
security_post_notification(const struct watch *watch,
const struct cred *trigger_cred,
struct watch_notification *n);
Also, should I let the LSM audit/edit the filter set by
IOC_WATCH_QUEUE_SET_FILTER? Userspace can't retrieve the filter, so the LSM
could edit it to exclude certain things. That might be a bit too complicated,
though.
> Things like superblocks are sticker because we don't generally
> think of them as objects. If you can do statfs(), you should be
> able to set a watch on the filesystem metadata.
>
> How would you specify a watch for a hardware event? If you say
> you have to open /dev/mumble to sent a watch for mumbles, you're
> good there, too.
That's not how that works at the moment. There's a global watch list for
device events. I've repurposed it to carry any device's events - so it will
carry blockdev events (I/O errors only at the moment) and usb events
(add/remove device, add/remove bus, reset device at the moment).
> > Now, I could see that you ignore UIDs on things like keys and
> > hardware-triggered events, but how does this interact with things like mount
> > watches that see directories that have UIDs?
> >
> > Are you advocating making it such that process B can only see events
> > triggered by process A if they have the same UID, for example?
>
> It's always seemed arbitrary to me that you can't open your process up to
> get signals from other users. What about putting mode bits on your ring
> buffer? By default you could only accept your own events, but you could do a
> rb_chmod(0222) and let all events through.
Ummm... This mechanism is pretty much about events generated by others.
Depend on what you mean by 'you' and 'your own events', it might be considered
that you would know what events you were directly causing and wouldn't need a
notification system for it.
> Subject to LSM addition restrictions, of course. That would require the cred
> of the process that triggered the event or a system cred for "hardware"
> events. If you don't like mode bits you could use an ACL for fine
> granularity or a single "let'em all in" bit for coarse.
I'm not entirely sure how an ACL would help. If someone creates a watch
queue, sets an ACL with only a "let everything in" ACE, we're back to the
situation we're in now.
As I understand it, the issue you have is stopping them getting events that
they're willing to accept that you think they shouldn't be allowed.
> I'm not against access, I'm against uncontrolled access in conflict with
> basic system policy.
David
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Stephen Smalley @ 2019-06-05 21:01 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Casey Schaufler, Andy Lutomirski, David Howells,
Al Viro, raven, Linux FS Devel, Linux API, linux-block, keyrings,
LSM List, LKML, Paul Moore
In-Reply-To: <20190605192842.GA9590@kroah.com>
On 6/5/19 3:28 PM, Greg KH wrote:
> On Wed, Jun 05, 2019 at 02:25:33PM -0400, Stephen Smalley wrote:
>> On 6/5/19 1:47 PM, Andy Lutomirski wrote:
>>>
>>>> On Jun 5, 2019, at 10:01 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>
>>>>> On 6/5/2019 9:04 AM, Andy Lutomirski wrote:
>>>>>> On Wed, Jun 5, 2019 at 7:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>> On 6/5/2019 1:41 AM, David Howells wrote:
>>>>>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>
>>>>>>>> I will try to explain the problem once again. If process A
>>>>>>>> sends a signal (writes information) to process B the kernel
>>>>>>>> checks that either process A has the same UID as process B
>>>>>>>> or that process A has privilege to override that policy.
>>>>>>>> Process B is passive in this access control decision, while
>>>>>>>> process A is active. In the event delivery case, process A
>>>>>>>> does something (e.g. modifies a keyring) that generates an
>>>>>>>> event, which is then sent to process B's event buffer.
>>>>>>> I think this might be the core sticking point here. It looks like two
>>>>>>> different situations:
>>>>>>>
>>>>>>> (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
>>>>>>>
>>>>>>> (2) A implicitly and unknowingly sends event to B as a side effect of some
>>>>>>> other action (eg. B has a watch for the event A did).
>>>>>>>
>>>>>>> The LSM treats them as the same: that is B must have MAC authorisation to send
>>>>>>> a message to A.
>>>>>> YES!
>>>>>>
>>>>>> Threat is about what you can do, not what you intend to do.
>>>>>>
>>>>>> And it would be really great if you put some thought into what
>>>>>> a rational model would be for UID based controls, too.
>>>>>>
>>>>>>> But there are problems with not sending the event:
>>>>>>>
>>>>>>> (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
>>>>>> Then B is a badly written program.
>>>>> Either I'm misunderstanding you or I strongly disagree.
>>>>
>>>> A program needs to be aware of the conditions under
>>>> which it gets event, *including the possibility that
>>>> it may not get an event that it's not allowed*. Do you
>>>> regularly write programs that go into corrupt states
>>>> if an open() fails? Or where read() returns less than
>>>> the amount of data you ask for?
>>>
>>> I do not regularly write programs that handle read() omitting data in the middle of a TCP stream. I also don’t write programs that wait for processes to die and need to handle the case where a child is dead, waitid() can see it, but SIGCHLD wasn’t sent because “security”.
>>>
>>>>
>>>>> If B has
>>>>> authority to detect a certain action, and A has authority to perform
>>>>> that action, then refusing to notify B because B is somehow missing
>>>>> some special authorization to be notified by A is nuts.
>>>>
>>>> You are hand-waving the notion of authority. You are assuming
>>>> that if A can read X and B can read X that A can write B.
>>>
>>> No, read it again please. I’m assuming that if A can *write* X and B can read X then A can send information to B.
>>
>> I guess the questions here are:
>>
>> 1) How do we handle recursive notification support, since we can't check
>> that B can read everything below a given directory easily? Perhaps we can
>> argue that if I have watch permission to / then that implies visibility to
>> everything below it but that is rather broad.
>
> How do you handle fanotify today which I think can do this?
Doesn't appear to have been given much thought; looks like
fanotify_init() checks capable(CAP_SYS_ADMIN) and fanotify_mark() checks
inode_permission(MAY_READ) on the mount/directory/file. File
descriptors for monitored files returned upon events at least get vetted
through security_file_open() so that can prevent the monitoring process
from receiving arbitrary descriptors. Would be preferable if
fanotify_mark() did some kind of security_path_watch() or similar check,
and distinguished mounts versus directories since monitoring of
directories is not recursive.
^ permalink raw reply
* Re: [PATCH 00/58] LSM: Module stacking for AppArmor
From: James Morris @ 2019-06-05 20:53 UTC (permalink / raw)
To: John Johansen
Cc: Stephen Smalley, Casey Schaufler, casey.schaufler,
linux-security-module, selinux, keescook, penguin-kernel, paul
In-Reply-To: <d86c6f89-39c5-bcf6-6491-96963d1113d3@canonical.com>
On Tue, 4 Jun 2019, John Johansen wrote:
> Yes, on Ubuntu & suse you can lauch lxd system containers with the
> container having a system policy bounding the container, and the container
> having its own apparmor policy namespace. So it loads and has its own
> policy that is enforced.
>
> This allows for us to run older versions of ubuntu (say 16.04) on an
> 18.04 host, and have the 16.04 policy behave just as if it was the host.
How well does the LSM stacking scale to 100s or more containers?
> This approach won't be an option for the 19.10 release and we will be
> needing the full patchset. I should be able to provide some benchmark
> and testing data soon.
Great.
--
James Morris <jmorris@namei.org>
^ permalink raw reply
* Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Andy Lutomirski @ 2019-06-05 20:14 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Xing, Cedric, Andy Lutomirski, Christopherson, Sean J,
Stephen Smalley, James Morris, Serge E . Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <20190605151653.GK11331@linux.intel.com>
> On Jun 5, 2019, at 8:17 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
>> On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
>> A bit off topic here. This mmap()/mprotect() discussion reminds me a
>> question (guess for Jarkko): Now that vma->vm_file->private_data keeps
>> a pointer to the enclave, why do we store it again in vma->vm_private?
>> It isn't a big deal but non-NULL vm_private does prevent mprotect()
>> from merging adjacent VMAs.
>
> Same semantics as with a regular mmap i.e. you can close the file and
> still use the mapping.
>
>
The file should be properly refcounted — vm_file should not go away while it’s mapped.
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Greg KH @ 2019-06-05 19:28 UTC (permalink / raw)
To: Stephen Smalley
Cc: Andy Lutomirski, Casey Schaufler, Andy Lutomirski, David Howells,
Al Viro, raven, Linux FS Devel, Linux API, linux-block, keyrings,
LSM List, LKML
In-Reply-To: <5dae2a59-1b91-7b35-7578-481d03c677bc@tycho.nsa.gov>
On Wed, Jun 05, 2019 at 02:25:33PM -0400, Stephen Smalley wrote:
> On 6/5/19 1:47 PM, Andy Lutomirski wrote:
> >
> > > On Jun 5, 2019, at 10:01 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >
> > > > On 6/5/2019 9:04 AM, Andy Lutomirski wrote:
> > > > > On Wed, Jun 5, 2019 at 7:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > > > On 6/5/2019 1:41 AM, David Howells wrote:
> > > > > > Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > > >
> > > > > > > I will try to explain the problem once again. If process A
> > > > > > > sends a signal (writes information) to process B the kernel
> > > > > > > checks that either process A has the same UID as process B
> > > > > > > or that process A has privilege to override that policy.
> > > > > > > Process B is passive in this access control decision, while
> > > > > > > process A is active. In the event delivery case, process A
> > > > > > > does something (e.g. modifies a keyring) that generates an
> > > > > > > event, which is then sent to process B's event buffer.
> > > > > > I think this might be the core sticking point here. It looks like two
> > > > > > different situations:
> > > > > >
> > > > > > (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
> > > > > >
> > > > > > (2) A implicitly and unknowingly sends event to B as a side effect of some
> > > > > > other action (eg. B has a watch for the event A did).
> > > > > >
> > > > > > The LSM treats them as the same: that is B must have MAC authorisation to send
> > > > > > a message to A.
> > > > > YES!
> > > > >
> > > > > Threat is about what you can do, not what you intend to do.
> > > > >
> > > > > And it would be really great if you put some thought into what
> > > > > a rational model would be for UID based controls, too.
> > > > >
> > > > > > But there are problems with not sending the event:
> > > > > >
> > > > > > (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
> > > > > Then B is a badly written program.
> > > > Either I'm misunderstanding you or I strongly disagree.
> > >
> > > A program needs to be aware of the conditions under
> > > which it gets event, *including the possibility that
> > > it may not get an event that it's not allowed*. Do you
> > > regularly write programs that go into corrupt states
> > > if an open() fails? Or where read() returns less than
> > > the amount of data you ask for?
> >
> > I do not regularly write programs that handle read() omitting data in the middle of a TCP stream. I also don’t write programs that wait for processes to die and need to handle the case where a child is dead, waitid() can see it, but SIGCHLD wasn’t sent because “security”.
> >
> > >
> > > > If B has
> > > > authority to detect a certain action, and A has authority to perform
> > > > that action, then refusing to notify B because B is somehow missing
> > > > some special authorization to be notified by A is nuts.
> > >
> > > You are hand-waving the notion of authority. You are assuming
> > > that if A can read X and B can read X that A can write B.
> >
> > No, read it again please. I’m assuming that if A can *write* X and B can read X then A can send information to B.
>
> I guess the questions here are:
>
> 1) How do we handle recursive notification support, since we can't check
> that B can read everything below a given directory easily? Perhaps we can
> argue that if I have watch permission to / then that implies visibility to
> everything below it but that is rather broad.
How do you handle fanotify today which I think can do this?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Paul Moore @ 2019-06-05 19:15 UTC (permalink / raw)
To: Janne Karhunen
Cc: Stephen Smalley, zohar, linux-integrity, linux-security-module
In-Reply-To: <20190605083606.4209-1-janne.karhunen@gmail.com>
On Wed, Jun 5, 2019 at 4:36 AM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Atomic policy updaters are not very useful as they cannot
> usually perform the policy updates on their own. Since it
> seems that there is no strict need for the atomicity,
> switch to the blocking variant. While doing so, rename
> the functions accordingly.
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---
> drivers/infiniband/core/device.c | 6 +++---
> include/linux/security.h | 6 +++---
> security/security.c | 23 +++++++++++++----------
> security/selinux/hooks.c | 2 +-
> security/selinux/selinuxfs.c | 2 +-
> 5 files changed, 21 insertions(+), 18 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 78dc07c6ac4b..61c0c93a2e73 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2499,7 +2499,7 @@ static int __init ib_core_init(void)
> goto err_mad;
> }
>
> - ret = register_lsm_notifier(&ibdev_lsm_nb);
> + ret = register_blocking_lsm_notifier(&ibdev_lsm_nb);
> if (ret) {
> pr_warn("Couldn't register LSM notifier. ret %d\n", ret);
> goto err_sa;
> @@ -2518,7 +2518,7 @@ static int __init ib_core_init(void)
> return 0;
>
> err_compat:
> - unregister_lsm_notifier(&ibdev_lsm_nb);
> + unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
> err_sa:
> ib_sa_cleanup();
> err_mad:
> @@ -2544,7 +2544,7 @@ static void __exit ib_core_cleanup(void)
> nldev_exit();
> rdma_nl_unregister(RDMA_NL_LS);
> unregister_pernet_device(&rdma_dev_net_ops);
> - unregister_lsm_notifier(&ibdev_lsm_nb);
> + unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
> ib_sa_cleanup();
> ib_mad_cleanup();
> addr_cleanup();
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..fc655fbe44ad 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -189,9 +189,9 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>
> #ifdef CONFIG_SECURITY
>
> -int call_lsm_notifier(enum lsm_event event, void *data);
> -int register_lsm_notifier(struct notifier_block *nb);
> -int unregister_lsm_notifier(struct notifier_block *nb);
> +int call_blocking_lsm_notifier(enum lsm_event event, void *data);
> +int register_blocking_lsm_notifier(struct notifier_block *nb);
> +int unregister_blocking_lsm_notifier(struct notifier_block *nb);
>
> /* prototypes */
> extern int security_init(void);
> diff --git a/security/security.c b/security/security.c
> index c01a88f65ad8..6bfc7636ddb7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -39,7 +39,7 @@
> #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>
> struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> -static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> +static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
>
> static struct kmem_cache *lsm_file_cache;
> static struct kmem_cache *lsm_inode_cache;
> @@ -430,23 +430,26 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> panic("%s - Cannot get early memory.\n", __func__);
> }
>
> -int call_lsm_notifier(enum lsm_event event, void *data)
> +int call_blocking_lsm_notifier(enum lsm_event event, void *data)
> {
> - return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
> + return blocking_notifier_call_chain(&blocking_lsm_notifier_chain,
> + event, data);
> }
> -EXPORT_SYMBOL(call_lsm_notifier);
> +EXPORT_SYMBOL(call_blocking_lsm_notifier);
>
> -int register_lsm_notifier(struct notifier_block *nb)
> +int register_blocking_lsm_notifier(struct notifier_block *nb)
> {
> - return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
> + return blocking_notifier_chain_register(&blocking_lsm_notifier_chain,
> + nb);
> }
> -EXPORT_SYMBOL(register_lsm_notifier);
> +EXPORT_SYMBOL(register_blocking_lsm_notifier);
>
> -int unregister_lsm_notifier(struct notifier_block *nb)
> +int unregister_blocking_lsm_notifier(struct notifier_block *nb)
> {
> - return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
> + return blocking_notifier_chain_unregister(&blocking_lsm_notifier_chain,
> + nb);
> }
> -EXPORT_SYMBOL(unregister_lsm_notifier);
> +EXPORT_SYMBOL(unregister_blocking_lsm_notifier);
>
> /**
> * lsm_cred_alloc - allocate a composite cred blob
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..c1e37018c8eb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -197,7 +197,7 @@ static int selinux_lsm_notifier_avc_callback(u32 event)
> {
> if (event == AVC_CALLBACK_RESET) {
> sel_ib_pkey_flush();
> - call_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> + call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> }
>
> return 0;
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 145ee62f205a..1e2e3e4b5fdb 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -180,7 +180,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
> selnl_notify_setenforce(new_value);
> selinux_status_update_setenforce(state, new_value);
> if (!new_value)
> - call_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> + call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
> }
> length = count;
> out:
> --
> 2.17.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Paul Moore @ 2019-06-05 19:14 UTC (permalink / raw)
To: Casey Schaufler, Janne Karhunen
Cc: Stephen Smalley, Mimi Zohar, linux-integrity,
linux-security-module
In-Reply-To: <1edfbd72-f492-db17-8717-a8cfe30d9654@schaufler-ca.com>
On Wed, Jun 5, 2019 at 1:05 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> > On Wed, Jun 5, 2019 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> >>> -int call_lsm_notifier(enum lsm_event event, void *data);
> >>> -int register_lsm_notifier(struct notifier_block *nb);
> >>> -int unregister_lsm_notifier(struct notifier_block *nb);
> >>> +int call_blocking_lsm_notifier(enum lsm_event event, void *data);
> >>> +int register_blocking_lsm_notifier(struct notifier_block *nb);
> >>> +int unregister_blocking_lsm_notifier(struct notifier_block *nb);
> >> Why is it important to change the names of these hooks?
> >> It's not like you had call_atomic_lsm_notifier() before.
> >> It seems like a lot of unnecessary code churn.
> > Paul was thinking there will eventually be two sets of notifiers
> > (atomic and blocking) and this creates the clear separation.
>
> One hook with an added "bool blocking" argument, if
> that's the only difference?
I think there is value in keeping a similar convention to the notifier
code on which this is based, see include/linux/notifier.h.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* KASAN: use-after-free Read in tomoyo_realpath_from_path
From: syzbot @ 2019-06-05 18:42 UTC (permalink / raw)
To: jmorris, linux-kernel, linux-security-module, penguin-kernel,
serge, syzkaller-bugs, takedakn
Hello,
syzbot found the following crash on:
HEAD commit: 788a0249 Merge tag 'arc-5.2-rc4' of git://git.kernel.org/p..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=179848d4a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=60564cb52ab29d5b
dashboard link: https://syzkaller.appspot.com/bug?extid=0341f6a4d729d4e0acf1
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13ac35baa00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+0341f6a4d729d4e0acf1@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in tomoyo_get_socket_name
security/tomoyo/realpath.c:238 [inline]
BUG: KASAN: use-after-free in tomoyo_realpath_from_path+0x722/0x7a0
security/tomoyo/realpath.c:284
Read of size 2 at addr ffff8880a91276d0 by task syz-executor.3/17397
CPU: 0 PID: 17397 Comm: syz-executor.3 Not tainted 5.2.0-rc3+ #12
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
print_address_description.cold+0x7c/0x20d mm/kasan/report.c:188
__kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317
kasan_report+0x12/0x20 mm/kasan/common.c:614
__asan_report_load2_noabort+0x14/0x20 mm/kasan/generic_report.c:130
tomoyo_get_socket_name security/tomoyo/realpath.c:238 [inline]
tomoyo_realpath_from_path+0x722/0x7a0 security/tomoyo/realpath.c:284
tomoyo_get_realpath security/tomoyo/file.c:151 [inline]
tomoyo_check_open_permission+0x2a8/0x3f0 security/tomoyo/file.c:771
tomoyo_file_open security/tomoyo/tomoyo.c:319 [inline]
tomoyo_file_open+0xa9/0xd0 security/tomoyo/tomoyo.c:314
security_file_open+0x71/0x300 security/security.c:1454
do_dentry_open+0x373/0x1250 fs/open.c:765
vfs_open+0xa0/0xd0 fs/open.c:887
do_last fs/namei.c:3416 [inline]
path_openat+0x10e9/0x46d0 fs/namei.c:3533
do_filp_open+0x1a1/0x280 fs/namei.c:3563
do_sys_open+0x3fe/0x5d0 fs/open.c:1070
__do_sys_open fs/open.c:1088 [inline]
__se_sys_open fs/open.c:1083 [inline]
__x64_sys_open+0x7e/0xc0 fs/open.c:1083
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x413161
Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 19 00 00 c3 48
83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f 05 <48> 8b 3c 24 48
89 c2 e8 53 fa ff ff 48 89 d0 48 83 c4 08 48 3d 01
RSP: 002b:00007f65230f8bb0 EFLAGS: 00000293 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 0000000000413161
RDX: fffffffffffffffa RSI: 0000000000000000 RDI: 00007f65230f8bd0
RBP: 000000000075c060 R08: 0000000000000050 R09: 000000000000000f
R10: 0000000000000004 R11: 0000000000000293 R12: 00007f65230f96d4
R13: 00000000004c83f6 R14: 00000000004dea40 R15: 00000000ffffffff
Allocated by task 17373:
save_stack+0x23/0x90 mm/kasan/common.c:71
set_track mm/kasan/common.c:79 [inline]
__kasan_kmalloc mm/kasan/common.c:489 [inline]
__kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:462
kasan_kmalloc+0x9/0x10 mm/kasan/common.c:503
__do_kmalloc mm/slab.c:3660 [inline]
__kmalloc+0x15c/0x740 mm/slab.c:3669
kmalloc include/linux/slab.h:552 [inline]
sk_prot_alloc+0x19c/0x2e0 net/core/sock.c:1602
sk_alloc+0x39/0xf70 net/core/sock.c:1656
base_sock_create drivers/isdn/mISDN/socket.c:758 [inline]
mISDN_sock_create+0xb4/0x3a0 drivers/isdn/mISDN/socket.c:780
__sock_create+0x3d8/0x730 net/socket.c:1424
sock_create net/socket.c:1475 [inline]
__sys_socket+0x103/0x220 net/socket.c:1517
__do_sys_socket net/socket.c:1526 [inline]
__se_sys_socket net/socket.c:1524 [inline]
__x64_sys_socket+0x73/0xb0 net/socket.c:1524
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 17371:
save_stack+0x23/0x90 mm/kasan/common.c:71
set_track mm/kasan/common.c:79 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/common.c:451
kasan_slab_free+0xe/0x10 mm/kasan/common.c:459
__cache_free mm/slab.c:3432 [inline]
kfree+0xcf/0x220 mm/slab.c:3755
sk_prot_free net/core/sock.c:1639 [inline]
__sk_destruct+0x4f7/0x6e0 net/core/sock.c:1725
sk_destruct+0x7b/0x90 net/core/sock.c:1733
__sk_free+0xce/0x300 net/core/sock.c:1744
sk_free+0x42/0x50 net/core/sock.c:1755
sock_put include/net/sock.h:1723 [inline]
base_sock_release+0x269/0x279 drivers/isdn/mISDN/socket.c:628
__sock_release+0xce/0x2a0 net/socket.c:601
sock_close+0x1b/0x30 net/socket.c:1273
__fput+0x2ff/0x890 fs/file_table.c:280
____fput+0x16/0x20 fs/file_table.c:313
task_work_run+0x145/0x1c0 kernel/task_work.c:113
tracehook_notify_resume include/linux/tracehook.h:185 [inline]
exit_to_usermode_loop+0x273/0x2c0 arch/x86/entry/common.c:168
prepare_exit_to_usermode arch/x86/entry/common.c:199 [inline]
syscall_return_slowpath arch/x86/entry/common.c:279 [inline]
do_syscall_64+0x58e/0x680 arch/x86/entry/common.c:304
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff8880a91276c0
which belongs to the cache kmalloc-2k of size 2048
The buggy address is located 16 bytes inside of
2048-byte region [ffff8880a91276c0, ffff8880a9127ec0)
The buggy address belongs to the page:
page:ffffea0002a44980 refcount:1 mapcount:0 mapping:ffff8880aa400c40
index:0x0 compound_mapcount: 0
flags: 0x1fffc0000010200(slab|head)
raw: 01fffc0000010200 ffffea00022c9f88 ffffea0002234408 ffff8880aa400c40
raw: 0000000000000000 ffff8880a91265c0 0000000100000003 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff8880a9127580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8880a9127600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> ffff8880a9127680: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
^
ffff8880a9127700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8880a9127780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Stephen Smalley @ 2019-06-05 18:25 UTC (permalink / raw)
To: Andy Lutomirski, Casey Schaufler
Cc: Andy Lutomirski, David Howells, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <15CBE0B8-2797-433B-B9D7-B059FD1B9266@amacapital.net>
On 6/5/19 1:47 PM, Andy Lutomirski wrote:
>
>> On Jun 5, 2019, at 10:01 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>> On 6/5/2019 9:04 AM, Andy Lutomirski wrote:
>>>> On Wed, Jun 5, 2019 at 7:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 6/5/2019 1:41 AM, David Howells wrote:
>>>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>
>>>>>> I will try to explain the problem once again. If process A
>>>>>> sends a signal (writes information) to process B the kernel
>>>>>> checks that either process A has the same UID as process B
>>>>>> or that process A has privilege to override that policy.
>>>>>> Process B is passive in this access control decision, while
>>>>>> process A is active. In the event delivery case, process A
>>>>>> does something (e.g. modifies a keyring) that generates an
>>>>>> event, which is then sent to process B's event buffer.
>>>>> I think this might be the core sticking point here. It looks like two
>>>>> different situations:
>>>>>
>>>>> (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
>>>>>
>>>>> (2) A implicitly and unknowingly sends event to B as a side effect of some
>>>>> other action (eg. B has a watch for the event A did).
>>>>>
>>>>> The LSM treats them as the same: that is B must have MAC authorisation to send
>>>>> a message to A.
>>>> YES!
>>>>
>>>> Threat is about what you can do, not what you intend to do.
>>>>
>>>> And it would be really great if you put some thought into what
>>>> a rational model would be for UID based controls, too.
>>>>
>>>>> But there are problems with not sending the event:
>>>>>
>>>>> (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
>>>> Then B is a badly written program.
>>> Either I'm misunderstanding you or I strongly disagree.
>>
>> A program needs to be aware of the conditions under
>> which it gets event, *including the possibility that
>> it may not get an event that it's not allowed*. Do you
>> regularly write programs that go into corrupt states
>> if an open() fails? Or where read() returns less than
>> the amount of data you ask for?
>
> I do not regularly write programs that handle read() omitting data in the middle of a TCP stream. I also don’t write programs that wait for processes to die and need to handle the case where a child is dead, waitid() can see it, but SIGCHLD wasn’t sent because “security”.
>
>>
>>> If B has
>>> authority to detect a certain action, and A has authority to perform
>>> that action, then refusing to notify B because B is somehow missing
>>> some special authorization to be notified by A is nuts.
>>
>> You are hand-waving the notion of authority. You are assuming
>> that if A can read X and B can read X that A can write B.
>
> No, read it again please. I’m assuming that if A can *write* X and B can read X then A can send information to B.
I guess the questions here are:
1) How do we handle recursive notification support, since we can't check
that B can read everything below a given directory easily? Perhaps we
can argue that if I have watch permission to / then that implies
visibility to everything below it but that is rather broad.
2) Is there always a corresponding labeled object in view for each of
these notifications to which we can check access when the watch is set?
3) Are notifications only generated for write events or can they be
generated by processes that only have read access to the object?
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Casey Schaufler @ 2019-06-05 18:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, David Howells, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML, casey
In-Reply-To: <15CBE0B8-2797-433B-B9D7-B059FD1B9266@amacapital.net>
On 6/5/2019 10:47 AM, Andy Lutomirski wrote:
>> On Jun 5, 2019, at 10:01 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>> On 6/5/2019 9:04 AM, Andy Lutomirski wrote:
>>>> On Wed, Jun 5, 2019 at 7:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 6/5/2019 1:41 AM, David Howells wrote:
>>>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>
>>>>>> I will try to explain the problem once again. If process A
>>>>>> sends a signal (writes information) to process B the kernel
>>>>>> checks that either process A has the same UID as process B
>>>>>> or that process A has privilege to override that policy.
>>>>>> Process B is passive in this access control decision, while
>>>>>> process A is active. In the event delivery case, process A
>>>>>> does something (e.g. modifies a keyring) that generates an
>>>>>> event, which is then sent to process B's event buffer.
>>>>> I think this might be the core sticking point here. It looks like two
>>>>> different situations:
>>>>>
>>>>> (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
>>>>>
>>>>> (2) A implicitly and unknowingly sends event to B as a side effect of some
>>>>> other action (eg. B has a watch for the event A did).
>>>>>
>>>>> The LSM treats them as the same: that is B must have MAC authorisation to send
>>>>> a message to A.
>>>> YES!
>>>>
>>>> Threat is about what you can do, not what you intend to do.
>>>>
>>>> And it would be really great if you put some thought into what
>>>> a rational model would be for UID based controls, too.
>>>>
>>>>> But there are problems with not sending the event:
>>>>>
>>>>> (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
>>>> Then B is a badly written program.
>>> Either I'm misunderstanding you or I strongly disagree.
>> A program needs to be aware of the conditions under
>> which it gets event, *including the possibility that
>> it may not get an event that it's not allowed*. Do you
>> regularly write programs that go into corrupt states
>> if an open() fails? Or where read() returns less than
>> the amount of data you ask for?
> I do not regularly write programs that handle read() omitting data in the middle of a TCP stream. I also don’t write programs that wait for processes to die and need to handle the case where a child is dead, waitid() can see it, but SIGCHLD wasn’t sent because “security”.
>
>>> If B has
>>> authority to detect a certain action, and A has authority to perform
>>> that action, then refusing to notify B because B is somehow missing
>>> some special authorization to be notified by A is nuts.
>> You are hand-waving the notion of authority. You are assuming
>> that if A can read X and B can read X that A can write B.
> No, read it again please. I’m assuming that if A can *write* X and B can read X then A can send information to B.
That is *not* a valid assumption:
A can write to /dev/null.
B can read from /dev/null.
Does not imply B can read what A wrote.
Does not imply A can send a signal to B.
A can send a UDP datagram to port 3343
B can is bound to port 3343
Does not imply the packet will be delivered
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: Andy Lutomirski @ 2019-06-05 17:47 UTC (permalink / raw)
To: Casey Schaufler
Cc: Andy Lutomirski, David Howells, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <9a9406ba-eda4-e3ec-2100-9f7cf1d5c130@schaufler-ca.com>
> On Jun 5, 2019, at 10:01 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> On 6/5/2019 9:04 AM, Andy Lutomirski wrote:
>>> On Wed, Jun 5, 2019 at 7:51 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 6/5/2019 1:41 AM, David Howells wrote:
>>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>
>>>>> I will try to explain the problem once again. If process A
>>>>> sends a signal (writes information) to process B the kernel
>>>>> checks that either process A has the same UID as process B
>>>>> or that process A has privilege to override that policy.
>>>>> Process B is passive in this access control decision, while
>>>>> process A is active. In the event delivery case, process A
>>>>> does something (e.g. modifies a keyring) that generates an
>>>>> event, which is then sent to process B's event buffer.
>>>> I think this might be the core sticking point here. It looks like two
>>>> different situations:
>>>>
>>>> (1) A explicitly sends event to B (eg. signalling, sendmsg, etc.)
>>>>
>>>> (2) A implicitly and unknowingly sends event to B as a side effect of some
>>>> other action (eg. B has a watch for the event A did).
>>>>
>>>> The LSM treats them as the same: that is B must have MAC authorisation to send
>>>> a message to A.
>>> YES!
>>>
>>> Threat is about what you can do, not what you intend to do.
>>>
>>> And it would be really great if you put some thought into what
>>> a rational model would be for UID based controls, too.
>>>
>>>> But there are problems with not sending the event:
>>>>
>>>> (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
>>> Then B is a badly written program.
>> Either I'm misunderstanding you or I strongly disagree.
>
> A program needs to be aware of the conditions under
> which it gets event, *including the possibility that
> it may not get an event that it's not allowed*. Do you
> regularly write programs that go into corrupt states
> if an open() fails? Or where read() returns less than
> the amount of data you ask for?
I do not regularly write programs that handle read() omitting data in the middle of a TCP stream. I also don’t write programs that wait for processes to die and need to handle the case where a child is dead, waitid() can see it, but SIGCHLD wasn’t sent because “security”.
>
>> If B has
>> authority to detect a certain action, and A has authority to perform
>> that action, then refusing to notify B because B is somehow missing
>> some special authorization to be notified by A is nuts.
>
> You are hand-waving the notion of authority. You are assuming
> that if A can read X and B can read X that A can write B.
No, read it again please. I’m assuming that if A can *write* X and B can read X then A can send information to B.
^ permalink raw reply
* Re: Rational model for UID based controls
From: Casey Schaufler @ 2019-06-05 17:40 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, Al Viro, raven, Linux FS Devel, Linux API,
linux-block, keyrings, LSM List, LKML, casey
In-Reply-To: <18357.1559753807@warthog.procyon.org.uk>
On 6/5/2019 9:56 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> YES!
> I'm trying to decide if that's fervour or irritation at this point ;-)
I think I finally got the point that the underlying mechanism,
direct or indirect, isn't the issue. It's the end result that
matters. That makes me happier.
>> And it would be really great if you put some thought into what
>> a rational model would be for UID based controls, too.
> I have put some thought into it, but I don't see a single rational model. It
> depends very much on the situation.
Right. You're mixing the kind of things that can generate events,
and that makes having a single policy difficult.
> In any case, that's what I was referring to when I said I might need to call
> inode_permission(). But UIDs don't exist for all filesystems, for example,
> and there are no UIDs on superblocks, mount objects or hardware events.
If you open() or stat() a file on those filesystems the UID
used in the access control comes from somewhere. Setting a watch
on things with UIDs should use the access mode on the file,
just like any other filesystem operation.
Things like superblocks are sticker because we don't generally
think of them as objects. If you can do statfs(), you should be
able to set a watch on the filesystem metadata.
How would you specify a watch for a hardware event? If you say
you have to open /dev/mumble to sent a watch for mumbles, you're
good there, too.
> Now, I could see that you ignore UIDs on things like keys and
> hardware-triggered events, but how does this interact with things like mount
> watches that see directories that have UIDs?
>
> Are you advocating making it such that process B can only see events triggered
> by process A if they have the same UID, for example?
It's always seemed arbitrary to me that you can't open
your process up to get signals from other users. What about
putting mode bits on your ring buffer? By default you could
only accept your own events, but you could do a rb_chmod(0222)
and let all events through. Subject to LSM addition restrictions,
of course. That would require the cred of the process that
triggered the event or a system cred for "hardware" events.
If you don't like mode bits you could use an ACL for fine
granularity or a single "let'em all in" bit for coarse.
I'm not against access, I'm against uncontrolled access
in conflict with basic system policy.
> David
^ permalink raw reply
* Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From: David Howells @ 2019-06-05 17:21 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, Andy Lutomirski, Al Viro, raven, Linux FS Devel,
Linux API, linux-block, keyrings, LSM List, LKML
In-Reply-To: <e4c19d1b-9827-5949-ecb8-6c3cb4648f58@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> > But there are problems with not sending the event:
> >
> > (1) B's internal state is then corrupt (or, at least, unknowingly invalid).
>
> Then B is a badly written program.
No. It may have the expectation that it will get events but then it is denied
those events and doesn't even know they've happened.
> > (2) B can potentially figure out that the event happened by other means.
>
> Then why does it need the event mechanism in the first place?
Why does a CPU have interrupt lines? It can always continuously poll the
hardware. Why do poll() and select() exist?
> > I've implemented four event sources so far:
> >
> > (1) Keys/keyrings. You can only get events on a key you have View permission
> > on and the other process has to have write access to it, so I think this
> > is good enough.
>
> Sounds fine.
>
> > (2) Block layer. Currently this will only get you hardware error events,
> > which is probably safe. I'm not sure you can manipulate those without
> > permission to directly access the device files.
>
> There's an argument to be made that this should require CAP_SYS_ADMIN,
> or that an LSM like SELinux might include hardware error events in
> policy, but generally I agree that system generated events like this
> are both harmless and pointless for the general public to watch.
CAP_SYS_ADMIN is probably too broad a hammer - this is something you might
want to let a file manager or desktop environment use. I wonder if we could
add a CAP_SYS_NOTIFY - or is it too late for adding new caps?
> > (3) Superblock. This is trickier since it can see events that can be
> > manufactured (R/W <-> R/O remounting, EDQUOT) as well as events that
> > can't without hardware control (EIO, network link loss, RF kill).
>
> The events generated by processes (the 1st set) need controls
> like keys. The events generated by the system (the 2nd set) may
> need controls like the block layer.
>
>
> > (4) Mount topology. This is the trickiest since it allows you to see
> > events beyond the point at which you placed your watch (in essence,
> > you place a subtree watch).
>
> Like keys.
>
> > The question is what permission checking should I do? Ideally, I'd
> > emulate a pathwalk between the watchpoint and the eventing object to
> > see if the owner of the watchpoint could reach it.
>
> That will depend, as I've been saying, on what causes
> the event to be generated. If it's from a process, the
> question is "can the active process, the one that generated
> the event, write to the passive, watching process?"
> If it's the system on a hardware event, you may want the watcher
> to have CAP_SYS_ADMIN.
>
> > I'd need to do a reverse walk, calling
> > inode_permission(MAY_NOT_BLOCK) for each directory between the
> > eventing object and the watchpoint to see if one rejects it - but
> > some filesystems have a permission check that can't be called in this
> > state.
>
> This is for setting the watch, right?
No. Setting the watch requires execute permission on the directory on which
you're setting the watch, but there's no way to know what permissions will be
required for an event at that point.
I'm talking about when an event is generated (hence "eventing object").
Imagine you have a subpath:
dirA/dirB/dirC/dirD/dirE
where dir* are directories. If you place a watch on dirA and then an event
occurs on dirB (such as someone mounting on it), I do a walk back up the
parental tree, in the order:
dirE, dirD, dirC, dirB, dirA
If I need to check permissions on all the directories, I would find the
watchpoint on dirA, then I would have to repeat the walk to find out whether
the owner of the watchpoint can access all of those directories (perhaps
skipping dirA since I had permission to place a watchpoint thereon).
Note that this is subject to going awry if there's a race versus rename().
> > It would also be necessary to do this separately for each watchpoint in
> > the parental chain.
> >
> > Further, each permissions check would generate an audit event and
> > could generate FAN_ACCESS and/or FAN_ACCESS_PERM fanotify events -
> > which could be a problem if fanotify is also trying to post those
> > events to the same watch queue.
>
> If you required that the watching process open(dir) what
> you want to watch you'd get this for free. Or did I miss
> something obvious?
A subtree watch, such as the mount topology watch, watches not only the
directory and mount object you pointed directly at, but the subtree rooted
thereon.
Take the sample program in the last patch. It places a watch on "/" with no
filter against WATCH_INFO_RECURSIVE, so it sees all mount topology events that
happen under the VFS path subtree rooted at "/" - whether or not it can
actually pathwalk to those mounts.
David
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Casey Schaufler @ 2019-06-05 17:05 UTC (permalink / raw)
To: Janne Karhunen
Cc: Stephen Smalley, Mimi Zohar, Paul Moore, linux-integrity,
linux-security-module, casey
In-Reply-To: <CAE=NcrahPmzmB-xJwxzXqaPGtJY+ijbxV4wXz7K=y-ocw4Cmwg@mail.gmail.com>
On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> On Wed, Jun 5, 2019 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>> -int call_lsm_notifier(enum lsm_event event, void *data);
>>> -int register_lsm_notifier(struct notifier_block *nb);
>>> -int unregister_lsm_notifier(struct notifier_block *nb);
>>> +int call_blocking_lsm_notifier(enum lsm_event event, void *data);
>>> +int register_blocking_lsm_notifier(struct notifier_block *nb);
>>> +int unregister_blocking_lsm_notifier(struct notifier_block *nb);
>> Why is it important to change the names of these hooks?
>> It's not like you had call_atomic_lsm_notifier() before.
>> It seems like a lot of unnecessary code churn.
> Paul was thinking there will eventually be two sets of notifiers
> (atomic and blocking) and this creates the clear separation.
One hook with an added "bool blocking" argument, if
that's the only difference?
> That's
> probably true, but it does indeed create a pretty big change that it
> is not really needed yet. I'm fine either way.
>
>
> --
> Janne
^ 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