* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: David Howells @ 2019-05-29 10:55 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <CAG48ez2rRh2_Kq_EGJs5k-ZBNffGs_Q=vkQdinorBgo58tbGpg@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> > + /* Global root? */
> > + if (mnt != parent) {
> > + cursor.dentry = READ_ONCE(mnt->mnt_mountpoint);
> > + mnt = parent;
> > + cursor.mnt = &mnt->mnt;
> > + continue;
> > + }
> > + break;
>
> (nit: this would look clearer if you inverted the condition and wrote
> it as "if (mnt == parent) break;", then you also wouldn't need that
> "continue" or the braces)
It does look better with the logic inverted, but you *do* still need the
continue. After the if-statement, there is:
cursor.dentry = cursor.dentry->d_parent;
which we need to skip. It might make sense to move that into an
else-statement from an aesthetic point of view.
David
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: David Howells @ 2019-05-29 11:00 UTC (permalink / raw)
To: Jann Horn, casey
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <CAG48ez2rRh2_Kq_EGJs5k-ZBNffGs_Q=vkQdinorBgo58tbGpg@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> > +void post_mount_notification(struct mount *changed,
> > + struct mount_notification *notify)
> > +{
> > + const struct cred *cred = current_cred();
>
> This current_cred() looks bogus to me. Can't mount topology changes
> come from all sorts of places? For example, umount_mnt() from
> umount_tree() from dissolve_on_fput() from __fput(), which could
> happen pretty much anywhere depending on where the last reference gets
> dropped?
IIRC, that's what Casey argued is the right thing to do from a security PoV.
Casey?
Maybe I should pass in NULL creds in the case that an event is being generated
because an object is being destroyed due to the last usage[*] being removed.
[*] Usage, not ref - Superblocks are a bit weird in their accounting.
David
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: David Howells @ 2019-05-29 11:16 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <CAG48ez2SAKbPeChAf06GMazMPPThFM+OR00abRZafAP7v+ptKw@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> I don't really know. I guess it depends on how it's being used? If
> someone decides to e.g. make a file browser that installs watches for
> a bunch of mountpoints for some fancy sidebar showing the device
> mounts on the system, or something like that, that probably shouldn't
> inhibit unmounting... I don't know if that's a realistic use case.
In such a use case, I would envision the browser putting a watch on "/". A
watch sees all events in the subtree rooted at that point and you must apply a
filter that filters them out if you're not interested (filter on
WATCH_INFO_IN_SUBTREE using info_filter and info_mask).
David
^ permalink raw reply
* [PATCH v5 1/3] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-05-29 12:38 UTC (permalink / raw)
To: Andrew Morton, Christoph Lameter, Kees Cook
Cc: Alexander Potapenko, Masahiro Yamada, Michal Hocko, James Morris,
Serge E. Hallyn, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Randy Dunlap,
Jann Horn, Mark Rutland, Marco Elver, linux-mm,
linux-security-module, kernel-hardening
In-Reply-To: <20190529123812.43089-1-glider@google.com>
The new options are needed to prevent possible information leaks and
make control-flow bugs that depend on uninitialized values more
deterministic.
init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
objects with zeroes. Initialization is done at allocation time at the
places where checks for __GFP_ZERO are performed.
init_on_free=1 makes the kernel initialize freed pages and heap objects
with zeroes upon their deletion. This helps to ensure sensitive data
doesn't leak via use-after-free accesses.
Both init_on_alloc=1 and init_on_free=1 guarantee that the allocator
returns zeroed memory. The two exceptions are slab caches with
constructors and SLAB_TYPESAFE_BY_RCU flag. Those are never
zero-initialized to preserve their semantics.
Both init_on_alloc and init_on_free default to zero, but those defaults
can be overridden with CONFIG_INIT_ON_ALLOC_DEFAULT_ON and
CONFIG_INIT_ON_FREE_DEFAULT_ON.
Slowdown for the new features compared to init_on_free=0,
init_on_alloc=0:
hackbench, init_on_free=1: +7.62% sys time (st.err 0.74%)
hackbench, init_on_alloc=1: +7.75% sys time (st.err 2.14%)
Linux build with -j12, init_on_free=1: +8.38% wall time (st.err 0.39%)
Linux build with -j12, init_on_free=1: +24.42% sys time (st.err 0.52%)
Linux build with -j12, init_on_alloc=1: -0.13% wall time (st.err 0.42%)
Linux build with -j12, init_on_alloc=1: +0.57% sys time (st.err 0.40%)
The slowdown for init_on_free=0, init_on_alloc=0 compared to the
baseline is within the standard error.
The new features are also going to pave the way for hardware memory
tagging (e.g. arm64's MTE), which will require both on_alloc and on_free
hooks to set the tags for heap objects. With MTE, tagging will have the
same cost as memory initialization.
Although init_on_free is rather costly, there are paranoid use-cases where
in-memory data lifetime is desired to be minimized. There are various
arguments for/against the realism of the associated threat models, but
given that we'll need the infrastructre for MTE anyway, and there are
people who want wipe-on-free behavior no matter what the performance cost,
it seems reasonable to include it in this series.
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
To: Kees Cook <keescook@chromium.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marco Elver <elver@google.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v2:
- unconditionally initialize pages in kernel_init_free_pages()
- comment from Randy Dunlap: drop 'default false' lines from Kconfig.hardening
v3:
- don't call kernel_init_free_pages() from memblock_free_pages()
- adopted some Kees' comments for the patch description
v4:
- use NULL instead of 0 in slab_alloc_node() (found by kbuild test robot)
- don't write to NULL object in slab_alloc_node() (found by Android
testing)
v5:
- adjusted documentation wording as suggested by Kees
- disable SLAB_POISON if auto-initialization is on
- don't wipe RCU cache allocations made without __GFP_ZERO
- dropped SLOB support
---
.../admin-guide/kernel-parameters.txt | 9 +++
drivers/infiniband/core/uverbs_ioctl.c | 2 +-
include/linux/mm.h | 22 +++++++
kernel/kexec_core.c | 2 +-
mm/dmapool.c | 2 +-
mm/page_alloc.c | 63 ++++++++++++++++---
mm/slab.c | 16 ++++-
mm/slab.h | 19 ++++++
mm/slub.c | 33 ++++++++--
net/core/sock.c | 2 +-
security/Kconfig.hardening | 29 +++++++++
11 files changed, 180 insertions(+), 19 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..84ee1121a2b9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1673,6 +1673,15 @@
initrd= [BOOT] Specify the location of the initial ramdisk
+ init_on_alloc= [MM] Fill newly allocated pages and heap objects with
+ zeroes.
+ Format: 0 | 1
+ Default set by CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
+
+ init_on_free= [MM] Fill freed pages and heap objects with zeroes.
+ Format: 0 | 1
+ Default set by CONFIG_INIT_ON_FREE_DEFAULT_ON.
+
init_pkru= [x86] Specify the default memory protection keys rights
register contents for all processes. 0x55555554 by
default (disallow access to all but pkey 0). Can
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 829b0c6944d8..61758201d9b2 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -127,7 +127,7 @@ __malloc void *_uverbs_alloc(struct uverbs_attr_bundle *bundle, size_t size,
res = (void *)pbundle->internal_buffer + pbundle->internal_used;
pbundle->internal_used =
ALIGN(new_used, sizeof(*pbundle->internal_buffer));
- if (flags & __GFP_ZERO)
+ if (want_init_on_alloc(flags))
memset(res, 0, size);
return res;
}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0e8834ac32b7..7733a341c0c4 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2685,6 +2685,28 @@ static inline void kernel_poison_pages(struct page *page, int numpages,
int enable) { }
#endif
+#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
+DECLARE_STATIC_KEY_TRUE(init_on_alloc);
+#else
+DECLARE_STATIC_KEY_FALSE(init_on_alloc);
+#endif
+static inline bool want_init_on_alloc(gfp_t flags)
+{
+ if (static_branch_unlikely(&init_on_alloc))
+ return true;
+ return flags & __GFP_ZERO;
+}
+
+#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
+DECLARE_STATIC_KEY_TRUE(init_on_free);
+#else
+DECLARE_STATIC_KEY_FALSE(init_on_free);
+#endif
+static inline bool want_init_on_free(void)
+{
+ return static_branch_unlikely(&init_on_free);
+}
+
extern bool _debug_pagealloc_enabled;
static inline bool debug_pagealloc_enabled(void)
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index fd5c95ff9251..2f75dd0d0d81 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -315,7 +315,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
arch_kexec_post_alloc_pages(page_address(pages), count,
gfp_mask);
- if (gfp_mask & __GFP_ZERO)
+ if (want_init_on_alloc(gfp_mask))
for (i = 0; i < count; i++)
clear_highpage(pages + i);
}
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 76a160083506..493d151067cb 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -381,7 +381,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
#endif
spin_unlock_irqrestore(&pool->lock, flags);
- if (mem_flags & __GFP_ZERO)
+ if (want_init_on_alloc(mem_flags))
memset(retval, 0, pool->size);
return retval;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d66bc8abe0af..50a3b104a491 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -136,6 +136,48 @@ unsigned long totalcma_pages __read_mostly;
int percpu_pagelist_fraction;
gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
+#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
+DEFINE_STATIC_KEY_TRUE(init_on_alloc);
+#else
+DEFINE_STATIC_KEY_FALSE(init_on_alloc);
+#endif
+#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
+DEFINE_STATIC_KEY_TRUE(init_on_free);
+#else
+DEFINE_STATIC_KEY_FALSE(init_on_free);
+#endif
+
+static int __init early_init_on_alloc(char *buf)
+{
+ int ret;
+ bool bool_result;
+
+ if (!buf)
+ return -EINVAL;
+ ret = kstrtobool(buf, &bool_result);
+ if (bool_result)
+ static_branch_enable(&init_on_alloc);
+ else
+ static_branch_disable(&init_on_alloc);
+ return ret;
+}
+early_param("init_on_alloc", early_init_on_alloc);
+
+static int __init early_init_on_free(char *buf)
+{
+ int ret;
+ bool bool_result;
+
+ if (!buf)
+ return -EINVAL;
+ ret = kstrtobool(buf, &bool_result);
+ if (bool_result)
+ static_branch_enable(&init_on_free);
+ else
+ static_branch_disable(&init_on_free);
+ return ret;
+}
+early_param("init_on_free", early_init_on_free);
/*
* A cached value of the page's pageblock's migratetype, used when the page is
@@ -1090,6 +1132,14 @@ static int free_tail_pages_check(struct page *head_page, struct page *page)
return ret;
}
+static void kernel_init_free_pages(struct page *page, int numpages)
+{
+ int i;
+
+ for (i = 0; i < numpages; i++)
+ clear_highpage(page + i);
+}
+
static __always_inline bool free_pages_prepare(struct page *page,
unsigned int order, bool check_free)
{
@@ -1142,6 +1192,8 @@ static __always_inline bool free_pages_prepare(struct page *page,
}
arch_free_page(page, order);
kernel_poison_pages(page, 1 << order, 0);
+ if (want_init_on_free())
+ kernel_init_free_pages(page, 1 << order);
if (debug_pagealloc_enabled())
kernel_map_pages(page, 1 << order, 0);
@@ -2020,8 +2072,8 @@ static inline int check_new_page(struct page *page)
static inline bool free_pages_prezeroed(void)
{
- return IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
- page_poisoning_enabled();
+ return (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
+ page_poisoning_enabled()) || want_init_on_free();
}
#ifdef CONFIG_DEBUG_VM
@@ -2075,13 +2127,10 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
unsigned int alloc_flags)
{
- int i;
-
post_alloc_hook(page, order, gfp_flags);
- if (!free_pages_prezeroed() && (gfp_flags & __GFP_ZERO))
- for (i = 0; i < (1 << order); i++)
- clear_highpage(page + i);
+ if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
+ kernel_init_free_pages(page, 1 << order);
if (order && (gfp_flags & __GFP_COMP))
prep_compound_page(page, order);
diff --git a/mm/slab.c b/mm/slab.c
index f7117ad9b3a3..98a89d7c922d 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1830,6 +1830,14 @@ static bool set_objfreelist_slab_cache(struct kmem_cache *cachep,
cachep->num = 0;
+ /*
+ * If slab auto-initialization on free is enabled, store the freelist
+ * off-slab, so that its contents don't end up in one of the allocated
+ * objects.
+ */
+ if (unlikely(slab_want_init_on_free(cachep)))
+ return false;
+
if (cachep->ctor || flags & SLAB_TYPESAFE_BY_RCU)
return false;
@@ -3263,7 +3271,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
local_irq_restore(save_flags);
ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
- if (unlikely(flags & __GFP_ZERO) && ptr)
+ if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
memset(ptr, 0, cachep->object_size);
slab_post_alloc_hook(cachep, flags, 1, &ptr);
@@ -3320,7 +3328,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
prefetchw(objp);
- if (unlikely(flags & __GFP_ZERO) && objp)
+ if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
memset(objp, 0, cachep->object_size);
slab_post_alloc_hook(cachep, flags, 1, &objp);
@@ -3441,6 +3449,8 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
struct array_cache *ac = cpu_cache_get(cachep);
check_irq_off();
+ if (unlikely(slab_want_init_on_free(cachep)))
+ memset(objp, 0, cachep->object_size);
kmemleak_free_recursive(objp, cachep->flags);
objp = cache_free_debugcheck(cachep, objp, caller);
@@ -3528,7 +3538,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
cache_alloc_debugcheck_after_bulk(s, flags, size, p, _RET_IP_);
/* Clear memory outside IRQ disabled section */
- if (unlikely(flags & __GFP_ZERO))
+ if (unlikely(slab_want_init_on_alloc(flags, s)))
for (i = 0; i < size; i++)
memset(p[i], 0, s->object_size);
diff --git a/mm/slab.h b/mm/slab.h
index 43ac818b8592..31032d488b29 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -524,4 +524,23 @@ static inline int cache_random_seq_create(struct kmem_cache *cachep,
static inline void cache_random_seq_destroy(struct kmem_cache *cachep) { }
#endif /* CONFIG_SLAB_FREELIST_RANDOM */
+static inline bool slab_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_alloc)) {
+ if (c->ctor)
+ return false;
+ if (c->flags & SLAB_TYPESAFE_BY_RCU)
+ return flags & __GFP_ZERO;
+ return true;
+ }
+ return flags & __GFP_ZERO;
+}
+
+static inline bool slab_want_init_on_free(struct kmem_cache *c)
+{
+ if (static_branch_unlikely(&init_on_free))
+ return !(c->ctor || (c->flags & SLAB_TYPESAFE_BY_RCU));
+ return false;
+}
+
#endif /* MM_SLAB_H */
diff --git a/mm/slub.c b/mm/slub.c
index cd04dbd2b5d0..9c4a8b9a955c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1279,6 +1279,12 @@ static int __init setup_slub_debug(char *str)
if (*str == ',')
slub_debug_slabs = str + 1;
out:
+ if ((static_branch_unlikely(&init_on_alloc) ||
+ static_branch_unlikely(&init_on_free)) &&
+ (slub_debug & SLAB_POISON)) {
+ pr_warn("disabling SLAB_POISON: can't be used together with memory auto-initialization\n");
+ slub_debug &= ~SLAB_POISON;
+ }
return 1;
}
@@ -1424,6 +1430,19 @@ static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
static inline bool slab_free_freelist_hook(struct kmem_cache *s,
void **head, void **tail)
{
+
+ void *object;
+ void *next = *head;
+ void *old_tail = *tail ? *tail : *head;
+
+ if (slab_want_init_on_free(s))
+ do {
+ object = next;
+ next = get_freepointer(s, object);
+ memset(object, 0, s->size);
+ set_freepointer(s, object, next);
+ } while (object != old_tail);
+
/*
* Compiler cannot detect this function can be removed if slab_free_hook()
* evaluates to nothing. Thus, catch all relevant config debug options here.
@@ -1433,9 +1452,7 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
defined(CONFIG_DEBUG_OBJECTS_FREE) || \
defined(CONFIG_KASAN)
- void *object;
- void *next = *head;
- void *old_tail = *tail ? *tail : *head;
+ next = *head;
/* Head and tail of the reconstructed freelist */
*head = NULL;
@@ -2741,8 +2758,14 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
prefetch_freepointer(s, next_object);
stat(s, ALLOC_FASTPATH);
}
+ /*
+ * If the object has been wiped upon free, make sure it's fully
+ * initialized by zeroing out freelist pointer.
+ */
+ if (unlikely(slab_want_init_on_free(s)) && object)
+ *(void **)object = NULL;
- if (unlikely(gfpflags & __GFP_ZERO) && object)
+ if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
memset(object, 0, s->object_size);
slab_post_alloc_hook(s, gfpflags, 1, &object);
@@ -3163,7 +3186,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
local_irq_enable();
/* Clear memory outside IRQ disabled fastpath loop */
- if (unlikely(flags & __GFP_ZERO)) {
+ if (unlikely(slab_want_init_on_alloc(flags, s))) {
int j;
for (j = 0; j < i; j++)
diff --git a/net/core/sock.c b/net/core/sock.c
index 75b1c950b49f..9ceb90c875bc 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1602,7 +1602,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
if (!sk)
return sk;
- if (priority & __GFP_ZERO)
+ if (want_init_on_alloc(priority))
sk_prot_clear_nulls(sk, prot->obj_size);
} else
sk = kmalloc(prot->obj_size, priority);
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index c6cb2d9b2905..a1ffe2eb4d5f 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -160,6 +160,35 @@ config STACKLEAK_RUNTIME_DISABLE
runtime to control kernel stack erasing for kernels built with
CONFIG_GCC_PLUGIN_STACKLEAK.
+config INIT_ON_ALLOC_DEFAULT_ON
+ bool "Enable heap memory zeroing on allocation by default"
+ help
+ This has the effect of setting "init_on_alloc=1" on the kernel
+ command line. This can be disabled with "init_on_alloc=0".
+ When "init_on_alloc" is enabled, all page allocator and slab
+ allocator memory will be zeroed when allocated, eliminating
+ many kinds of "uninitialized heap memory" flaws, especially
+ heap content exposures. The performance impact varies by
+ workload, but most cases see <1% impact. Some synthetic
+ workloads have measured as high as 7%.
+
+config INIT_ON_FREE_DEFAULT_ON
+ bool "Enable heap memory zeroing on free by default"
+ help
+ This has the effect of setting "init_on_free=1" on the kernel
+ command line. This can be disabled with "init_on_free=0".
+ Similar to "init_on_alloc", when "init_on_free" is enabled,
+ all page allocator and slab allocator memory will be zeroed
+ when freed, eliminating many kinds of "uninitialized heap memory"
+ flaws, especially heap content exposures. The primary difference
+ with "init_on_free" is that data lifetime in memory is reduced,
+ as anything freed is wiped immediately, making live forensics or
+ cold boot memory attacks unable to recover freed memory contents.
+ The performance impact varies by workload, but is more expensive
+ than "init_on_alloc" due to the negative cache effects of
+ touching "cold" memory areas. Most cases see 3-5% impact. Some
+ synthetic workloads have measured as high as 8%.
+
endmenu
endmenu
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH v5 2/3] mm: init: report memory auto-initialization features at boot time
From: Alexander Potapenko @ 2019-05-29 12:38 UTC (permalink / raw)
To: Andrew Morton, Christoph Lameter
Cc: Alexander Potapenko, Kees Cook, Dmitry Vyukov, James Morris,
Jann Horn, Kostya Serebryany, Laura Abbott, Mark Rutland,
Masahiro Yamada, Matthew Wilcox, Nick Desaulniers, Randy Dunlap,
Sandeep Patil, Serge E. Hallyn, Souptick Joarder, Marco Elver,
kernel-hardening, linux-mm, linux-security-module
In-Reply-To: <20190529123812.43089-1-glider@google.com>
Print the currently enabled stack and heap initialization modes.
The possible options for stack are:
- "all" for CONFIG_INIT_STACK_ALL;
- "byref_all" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL;
- "byref" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF;
- "__user" for CONFIG_GCC_PLUGIN_STRUCTLEAK_USER;
- "off" otherwise.
Depending on the values of init_on_alloc and init_on_free boottime
options we also report "heap alloc" and "heap free" as "on"/"off".
In the init_on_free mode initializing pages at boot time may take some
time, so print a notice about that as well.
Signed-off-by: Alexander Potapenko <glider@google.com>
Suggested-by: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: Marco Elver <elver@google.com>
Cc: kernel-hardening@lists.openwall.com
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
---
init/main.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/init/main.c b/init/main.c
index 66a196c5e4c3..9d63ff1d48f3 100644
--- a/init/main.c
+++ b/init/main.c
@@ -520,6 +520,29 @@ static inline void initcall_debug_enable(void)
}
#endif
+/* Report memory auto-initialization states for this boot. */
+void __init report_meminit(void)
+{
+ const char *stack;
+
+ if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
+ stack = "all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
+ stack = "byref_all";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
+ stack = "byref";
+ else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_USER))
+ stack = "__user";
+ else
+ stack = "off";
+
+ pr_info("mem auto-init: stack:%s, heap alloc:%s, heap free:%s\n",
+ stack, want_init_on_alloc(GFP_KERNEL) ? "on" : "off",
+ want_init_on_free() ? "on" : "off");
+ if (want_init_on_free())
+ pr_info("Clearing system memory may take some time...\n");
+}
+
/*
* Set up kernel memory allocators
*/
@@ -530,6 +553,7 @@ static void __init mm_init(void)
* bigger than MAX_ORDER unless SPARSEMEM.
*/
page_ext_init_flatmem();
+ report_meminit();
mem_init();
kmem_cache_init();
pgtable_init();
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH v5 3/3] lib: introduce test_meminit module
From: Alexander Potapenko @ 2019-05-29 12:38 UTC (permalink / raw)
To: Kees Cook, Andrew Morton, Christoph Lameter
Cc: Alexander Potapenko, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Sandeep Patil, Laura Abbott, Jann Horn,
Marco Elver, linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190529123812.43089-1-glider@google.com>
Add tests for heap and pagealloc initialization.
These can be used to check init_on_alloc and init_on_free implementations
as well as other approaches to initialization.
Expected test output in the case the kernel provides heap initialization
(e.g. when running with either init_on_alloc=1 or init_on_free=1):
test_meminit: all 10 tests in test_pages passed
test_meminit: all 40 tests in test_kvmalloc passed
test_meminit: all 60 tests in test_kmemcache passed
test_meminit: all 10 tests in test_rcu_persistent passed
test_meminit: all 120 tests passed!
Signed-off-by: Alexander Potapenko <glider@google.com>
To: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
To: Christoph Lameter <cl@linux.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Sandeep Patil <sspatil@android.com>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Marco Elver <elver@google.com>
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v3:
- added example test output to the description
- fixed a missing include spotted by kbuild test robot <lkp@intel.com>
- added a missing MODULE_LICENSE
- call do_kmem_cache_size() with size >= sizeof(void*) to unbreak
debug builds
v5:
- added tests for RCU slabs and __GFP_ZERO
---
lib/Kconfig.debug | 8 +
lib/Makefile | 1 +
lib/test_meminit.c | 362 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 371 insertions(+)
create mode 100644 lib/test_meminit.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index cbdfae379896..085711f14abf 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2040,6 +2040,14 @@ config TEST_STACKINIT
If unsure, say N.
+config TEST_MEMINIT
+ tristate "Test heap/page initialization"
+ help
+ Test if the kernel is zero-initializing heap and page allocations.
+ This can be useful to test init_on_alloc and init_on_free features.
+
+ If unsure, say N.
+
endif # RUNTIME_TESTING_MENU
config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index fb7697031a79..05980c802500 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
+obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
diff --git a/lib/test_meminit.c b/lib/test_meminit.c
new file mode 100644
index 000000000000..ed7efec1387b
--- /dev/null
+++ b/lib/test_meminit.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for SL[AOU]B/page initialization at alloc/free time.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/vmalloc.h>
+
+#define GARBAGE_INT (0x09A7BA9E)
+#define GARBAGE_BYTE (0x9E)
+
+#define REPORT_FAILURES_IN_FN() \
+ do { \
+ if (failures) \
+ pr_info("%s failed %d out of %d times\n", \
+ __func__, failures, num_tests); \
+ else \
+ pr_info("all %d tests in %s passed\n", \
+ num_tests, __func__); \
+ } while (0)
+
+/* Calculate the number of uninitialized bytes in the buffer. */
+static int __init count_nonzero_bytes(void *ptr, size_t size)
+{
+ int i, ret = 0;
+ unsigned char *p = (unsigned char *)ptr;
+
+ for (i = 0; i < size; i++)
+ if (p[i])
+ ret++;
+ return ret;
+}
+
+/* Fill a buffer with garbage, skipping |skip| first bytes. */
+static void __init fill_with_garbage_skip(void *ptr, size_t size, size_t skip)
+{
+ unsigned int *p = (unsigned int *)ptr;
+ int i = 0;
+
+ if (skip) {
+ WARN_ON(skip > size);
+ p += skip;
+ }
+ while (size >= sizeof(*p)) {
+ p[i] = GARBAGE_INT;
+ i++;
+ size -= sizeof(*p);
+ }
+ if (size)
+ memset(&p[i], GARBAGE_BYTE, size);
+}
+
+static void __init fill_with_garbage(void *ptr, size_t size)
+{
+ fill_with_garbage_skip(ptr, size, 0);
+}
+
+static int __init do_alloc_pages_order(int order, int *total_failures)
+{
+ struct page *page;
+ void *buf;
+ size_t size = PAGE_SIZE << order;
+
+ page = alloc_pages(GFP_KERNEL, order);
+ buf = page_address(page);
+ fill_with_garbage(buf, size);
+ __free_pages(page, order);
+
+ page = alloc_pages(GFP_KERNEL, order);
+ buf = page_address(page);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ __free_pages(page, order);
+ return 1;
+}
+
+/* Test the page allocator by calling alloc_pages with different orders. */
+static int __init test_pages(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i;
+
+ for (i = 0; i < 10; i++)
+ num_tests += do_alloc_pages_order(i, &failures);
+
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+/* Test kmalloc() with given parameters. */
+static int __init do_kmalloc_size(size_t size, int *total_failures)
+{
+ void *buf;
+
+ buf = kmalloc(size, GFP_KERNEL);
+ fill_with_garbage(buf, size);
+ kfree(buf);
+
+ buf = kmalloc(size, GFP_KERNEL);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ kfree(buf);
+ return 1;
+}
+
+/* Test vmalloc() with given parameters. */
+static int __init do_vmalloc_size(size_t size, int *total_failures)
+{
+ void *buf;
+
+ buf = vmalloc(size);
+ fill_with_garbage(buf, size);
+ vfree(buf);
+
+ buf = vmalloc(size);
+ if (count_nonzero_bytes(buf, size))
+ (*total_failures)++;
+ fill_with_garbage(buf, size);
+ vfree(buf);
+ return 1;
+}
+
+/* Test kmalloc()/vmalloc() by allocating objects of different sizes. */
+static int __init test_kvmalloc(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i, size;
+
+ for (i = 0; i < 20; i++) {
+ size = 1 << i;
+ num_tests += do_kmalloc_size(size, &failures);
+ num_tests += do_vmalloc_size(size, &failures);
+ }
+
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+#define CTOR_BYTES (sizeof(unsigned int))
+#define CTOR_PATTERN (0x41414141)
+/* Initialize the first 4 bytes of the object. */
+static void test_ctor(void *obj)
+{
+ *(unsigned int *)obj = CTOR_PATTERN;
+}
+
+/*
+ * Check the invariants for the buffer allocated from a slab cache.
+ * If the cache has a test constructor, the first 4 bytes of the object must
+ * always remain equal to CTOR_PATTERN.
+ * If the cache isn't an RCU-typesafe one, or if the allocation is done with
+ * __GFP_ZERO, then the object contents must be zeroed after allocation.
+ * If the cache is an RCU-typesafe one, the object contents must never be
+ * zeroed after the first use. This is checked by memcmp() in
+ * do_kmem_cache_size().
+ */
+static bool __init check_buf(void *buf, int size, bool want_ctor,
+ bool want_rcu, bool want_zero)
+{
+ int bytes;
+ bool fail = false;
+
+ bytes = count_nonzero_bytes(buf, size);
+ WARN_ON(want_ctor && want_zero);
+ if (want_zero)
+ return bytes;
+ if (want_ctor) {
+ if (*(unsigned int *)buf != CTOR_PATTERN)
+ fail = 1;
+ } else {
+ if (bytes)
+ fail = !want_rcu;
+ }
+ return fail;
+}
+
+/*
+ * Test kmem_cache with given parameters:
+ * want_ctor - use a constructor;
+ * want_rcu - use SLAB_TYPESAFE_BY_RCU;
+ * want_zero - use __GFP_ZERO.
+ */
+static int __init do_kmem_cache_size(size_t size, bool want_ctor,
+ bool want_rcu, bool want_zero,
+ int *total_failures)
+{
+ struct kmem_cache *c;
+ int iter;
+ bool fail = false;
+ gfp_t alloc_mask = GFP_KERNEL | (want_zero ? __GFP_ZERO : 0);
+ void *buf, *buf_copy;
+
+ c = kmem_cache_create("test_cache", size, 1,
+ want_rcu ? SLAB_TYPESAFE_BY_RCU : 0,
+ want_ctor ? test_ctor : NULL);
+ for (iter = 0; iter < 10; iter++) {
+ buf = kmem_cache_alloc(c, alloc_mask);
+ /* Check that buf is zeroed, if it must be. */
+ fail = check_buf(buf, size, want_ctor, want_rcu, want_zero);
+ fill_with_garbage_skip(buf, size, want_ctor ? CTOR_BYTES : 0);
+ /*
+ * If this is an RCU cache, use a critical section to ensure we
+ * can touch objects after they're freed.
+ */
+ if (want_rcu) {
+ rcu_read_lock();
+ /*
+ * Copy the buffer to check that it's not wiped on
+ * free().
+ */
+ buf_copy = kmalloc(size, GFP_KERNEL);
+ if (buf_copy)
+ memcpy(buf_copy, buf, size);
+ }
+ kmem_cache_free(c, buf);
+ if (want_rcu) {
+ /*
+ * Check that |buf| is intact after kmem_cache_free().
+ * |want_zero| is false, because we wrote garbage to
+ * the buffer already.
+ */
+ fail |= check_buf(buf, size, want_ctor, want_rcu,
+ false);
+ if (buf_copy) {
+ fail |= (bool)memcmp(buf, buf_copy, size);
+ kfree(buf_copy);
+ }
+ rcu_read_unlock();
+ }
+ }
+ kmem_cache_destroy(c);
+
+ *total_failures += fail;
+ return 1;
+}
+
+/*
+ * Check that the data written to an RCU-allocated object survives
+ * reallocation.
+ */
+static int __init do_kmem_cache_rcu_persistent(int size, int *total_failures)
+{
+ struct kmem_cache *c;
+ void *buf, *buf_contents, *saved_ptr;
+ void **used_objects;
+ int i, iter, maxiter = 1024;
+ bool fail = false;
+
+ c = kmem_cache_create("test_cache", size, size, SLAB_TYPESAFE_BY_RCU,
+ NULL);
+ buf = kmem_cache_alloc(c, GFP_KERNEL);
+ saved_ptr = buf;
+ fill_with_garbage(buf, size);
+ buf_contents = kmalloc(size, GFP_KERNEL);
+ if (!buf_contents)
+ goto out;
+ used_objects = kmalloc_array(maxiter, sizeof(void *), GFP_KERNEL);
+ if (!used_objects) {
+ kfree(buf_contents);
+ goto out;
+ }
+ memcpy(buf_contents, buf, size);
+ kmem_cache_free(c, buf);
+ /*
+ * Run for a fixed number of iterations. If we never hit saved_ptr,
+ * assume the test passes.
+ */
+ for (iter = 0; iter < maxiter; iter++) {
+ buf = kmem_cache_alloc(c, GFP_KERNEL);
+ used_objects[iter] = buf;
+ if (buf == saved_ptr) {
+ fail = memcmp(buf_contents, buf, size);
+ for (i = 0; i <= iter; i++)
+ kmem_cache_free(c, used_objects[i]);
+ goto free_out;
+ }
+ }
+
+free_out:
+ kmem_cache_destroy(c);
+ kfree(buf_contents);
+ kfree(used_objects);
+out:
+ *total_failures += fail;
+ return 1;
+}
+
+/*
+ * Test kmem_cache allocation by creating caches of different sizes, with and
+ * without constructors, with and without SLAB_TYPESAFE_BY_RCU.
+ */
+static int __init test_kmemcache(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i, flags, size;
+ bool ctor, rcu, zero;
+
+ for (i = 0; i < 10; i++) {
+ size = 8 << i;
+ for (flags = 0; flags < 8; flags++) {
+ ctor = flags & 1;
+ rcu = flags & 2;
+ zero = flags & 4;
+ if (ctor & zero)
+ continue;
+ num_tests += do_kmem_cache_size(size, ctor, rcu, zero,
+ &failures);
+ }
+ }
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+/* Test the behavior of SLAB_TYPESAFE_BY_RCU caches of different sizes. */
+static int __init test_rcu_persistent(int *total_failures)
+{
+ int failures = 0, num_tests = 0;
+ int i, size;
+
+ for (i = 0; i < 10; i++) {
+ size = 8 << i;
+ num_tests += do_kmem_cache_rcu_persistent(size, &failures);
+ }
+ REPORT_FAILURES_IN_FN();
+ *total_failures += failures;
+ return num_tests;
+}
+
+/*
+ * Run the tests. Each test function returns the number of executed tests and
+ * updates |failures| with the number of failed tests.
+ */
+static int __init test_meminit_init(void)
+{
+ int failures = 0, num_tests = 0;
+
+ num_tests += test_pages(&failures);
+ num_tests += test_kvmalloc(&failures);
+ num_tests += test_kmemcache(&failures);
+ num_tests += test_rcu_persistent(&failures);
+
+ if (failures == 0)
+ pr_info("all %d tests passed!\n", num_tests);
+ else
+ pr_info("failures: %d out of %d\n", failures, num_tests);
+
+ return failures ? -EINVAL : 0;
+}
+module_init(test_meminit_init);
+
+MODULE_LICENSE("GPL");
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* Re: [PATCH 4/7] vfs: Add superblock notifications
From: David Howells @ 2019-05-29 12:58 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list
In-Reply-To: <CAG48ez2o1egR13FDd3=CgdXP_MbBsZM4SX=+aqvR6eheWddhFg@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> It might make sense to require that the path points to the root inode
> of the superblock? That way you wouldn't be able to do this on a bind
> mount that exposes part of a shared filesystem to a container.
Why prevent that? It doesn't prevent the container denizen from watching a
bind mount that exposes the root of a shared filesystem into a container.
It probably makes sense to permit the LSM to rule on whether a watch may be
emplaced, however.
> > + ret = add_watch_to_object(watch, s->s_watchers);
> > + if (ret == 0) {
> > + spin_lock(&sb_lock);
> > + s->s_count++;
> > + spin_unlock(&sb_lock);
>
> Why do watches hold references on the superblock they're watching?
Fair point. It was necessary at one point, but I don't think it is now. I'll
see if I can remove it. Note that it doesn't stop a superblock from being
unmounted and destroyed.
> > + }
> > + }
> > + up_write(&s->s_umount);
> > + if (ret < 0)
> > + kfree(watch);
> > + } else if (s->s_watchers) {
>
> This should probably have something like a READ_ONCE() for clarity?
Note that I think I'll rearrange this to:
} else {
ret = -EBADSLT;
if (s->s_watchers) {
down_write(&s->s_umount);
ret = remove_watch_from_object(s->s_watchers, wqueue,
s->s_unique_id, false);
up_write(&s->s_umount);
}
}
I'm not sure READ_ONCE() is necessary, since s_watchers can only be
instantiated once and the watch list then persists until the superblock is
deactivated. Furthermore, by the time deactivate_locked_super() is called, we
can't be calling sb_notify() on it as it's become inaccessible.
So if we see s->s_watchers as non-NULL, we should not see anything different
inside the lock. In fact, I should be able to rewrite the above to:
} else {
ret = -EBADSLT;
wlist = s->s_watchers;
if (wlist) {
down_write(&s->s_umount);
ret = remove_watch_from_object(wlist, wqueue,
s->s_unique_id, false);
up_write(&s->s_umount);
}
}
David
^ permalink raw reply
* [PATCH v2 0/3] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu
Changelog
v1:
- remove patch 2/4 (evm: reset status in evm_inode_post_setattr()); file
attributes cannot be set if the signature is portable and immutable
- patch 3/4: add __ro_after_init to ima_appraise_req_evm variable
declaration
- patch 3/4: remove ima_appraise_req_evm kernel option and introduce
'enforce-evm' and 'log-evm' as possible values for ima_appraise=
- remove patch 4/4 (ima: only audit failed appraisal verifications)
- add new patch (ima: show rules with IMA_INMASK correctly)
Roberto Sassu (3):
evm: check hash algorithm passed to init_desc()
ima: don't ignore INTEGRITY_UNKNOWN EVM status
ima: show rules with IMA_INMASK correctly
.../admin-guide/kernel-parameters.txt | 3 ++-
security/integrity/evm/evm_crypto.c | 3 +++
security/integrity/ima/ima_appraise.c | 8 +++++++
security/integrity/ima/ima_policy.c | 21 +++++++++++--------
4 files changed, 25 insertions(+), 10 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH v2 1/3] evm: check hash algorithm passed to init_desc()
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190529133035.28724-1-roberto.sassu@huawei.com>
This patch prevents memory access beyond the evm_tfm array by checking the
validity of the index (hash algorithm) passed to init_desc(). The hash
algorithm can be arbitrarily set if the security.ima xattr type is not
EVM_XATTR_HMAC.
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/evm/evm_crypto.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index e11564eb645b..82a38e801ee4 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
tfm = &hmac_tfm;
algo = evm_hmac;
} else {
+ if (hash_algo >= HASH_ALGO__LAST)
+ return ERR_PTR(-EINVAL);
+
tfm = &evm_tfm[hash_algo];
algo = hash_algo_name[hash_algo];
}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190529133035.28724-1-roberto.sassu@huawei.com>
Currently, ima_appraise_measurement() ignores the EVM status when
evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
status. The problem is that the EVM status is overwritten with the
appraisal status.
This patch mitigates the issue by selecting signature verification as the
only method allowed for appraisal when EVM is not initialized. Since the
new behavior might break user space, it must be turned on by adding the
'-evm' suffix to the value of the ima_appraise= kernel option.
Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
Documentation/admin-guide/kernel-parameters.txt | 3 ++-
security/integrity/ima/ima_appraise.c | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..d84a2e612b93 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1585,7 +1585,8 @@
Set number of hash buckets for inode cache.
ima_appraise= [IMA] appraise integrity measurements
- Format: { "off" | "enforce" | "fix" | "log" }
+ Format: { "off" | "enforce" | "fix" | "log" |
+ "enforce-evm" | "log-evm" }
default: "enforce"
ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..afef06e10fb9 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -18,6 +18,7 @@
#include "ima.h"
+static bool ima_appraise_req_evm __ro_after_init;
static int __init default_appraise_setup(char *str)
{
#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
@@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str)
else if (strncmp(str, "fix", 3) == 0)
ima_appraise = IMA_APPRAISE_FIX;
#endif
+ if (strcmp(str, "enforce-evm") == 0 ||
+ strcmp(str, "log-evm") == 0)
+ ima_appraise_req_evm = true;
return 1;
}
@@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func,
switch (status) {
case INTEGRITY_PASS:
case INTEGRITY_PASS_IMMUTABLE:
+ break;
case INTEGRITY_UNKNOWN:
+ if (ima_appraise_req_evm &&
+ xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+ goto out;
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 3/3] ima: show rules with IMA_INMASK correctly
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190529133035.28724-1-roberto.sassu@huawei.com>
Show the '^' character when a policy rule has flag IMA_INMASK.
Fixes: 80eae209d63ac ("IMA: allow reading back the current IMA policy")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/ima/ima_policy.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..ae4034f041c4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1146,10 +1146,10 @@ enum {
};
static const char *const mask_tokens[] = {
- "MAY_EXEC",
- "MAY_WRITE",
- "MAY_READ",
- "MAY_APPEND"
+ "^MAY_EXEC",
+ "^MAY_WRITE",
+ "^MAY_READ",
+ "^MAY_APPEND"
};
#define __ima_hook_stringify(str) (#str),
@@ -1209,6 +1209,7 @@ int ima_policy_show(struct seq_file *m, void *v)
struct ima_rule_entry *entry = v;
int i;
char tbuf[64] = {0,};
+ int offset = 0;
rcu_read_lock();
@@ -1232,15 +1233,17 @@ int ima_policy_show(struct seq_file *m, void *v)
if (entry->flags & IMA_FUNC)
policy_func_show(m, entry->func);
- if (entry->flags & IMA_MASK) {
+ if ((entry->flags & IMA_MASK) || (entry->flags & IMA_INMASK)) {
+ if (entry->flags & IMA_MASK)
+ offset = 1;
if (entry->mask & MAY_EXEC)
- seq_printf(m, pt(Opt_mask), mt(mask_exec));
+ seq_printf(m, pt(Opt_mask), mt(mask_exec) + offset);
if (entry->mask & MAY_WRITE)
- seq_printf(m, pt(Opt_mask), mt(mask_write));
+ seq_printf(m, pt(Opt_mask), mt(mask_write) + offset);
if (entry->mask & MAY_READ)
- seq_printf(m, pt(Opt_mask), mt(mask_read));
+ seq_printf(m, pt(Opt_mask), mt(mask_read) + offset);
if (entry->mask & MAY_APPEND)
- seq_printf(m, pt(Opt_mask), mt(mask_append));
+ seq_printf(m, pt(Opt_mask), mt(mask_append) + offset);
seq_puts(m, " ");
}
--
2.17.1
^ permalink raw reply related
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-29 14:08 UTC (permalink / raw)
To: Sean Christopherson, Xing, Cedric, William Roberts
Cc: Andy Lutomirski, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
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
In-Reply-To: <20190528202407.GB13158@linux.intel.com>
On 5/28/19 4:24 PM, Sean Christopherson wrote:
> On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
>>> From: Andy Lutomirski [mailto:luto@kernel.org]
>>> Sent: Saturday, May 25, 2019 5:58 PM
>>>
>>> On Sat, May 25, 2019 at 3:40 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>>>>
>>>> If we think of EADD as a way of mmap()'ing an enclave file into memory,
>>>> would this
>>> security_enclave_load() be the same as
>>> security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
>>> the target is now EPC instead of regular pages?
>>>
>>> Hmm, that's clever. Although it seems plausible that an LSM would want to
>>> allow RX or RWX of a given file page but only in the context of an approved
>>> enclave, so I think it should still be its own hook.
>>
>> What do you mean by "in the context of an approved enclave"? EPC pages are
>> *inaccessible* to any software until after EINIT. So it would never be a
>> security concern to EADD a page with wrong permissions as long as the enclave
>> would be denied eventually by LSM at EINIT.
>>
>> But I acknowledge the difference between loading a page into regular memory
>> vs. into EPC. So it's beneficial to have a separate hook, which if not
>> hooked, would pass through to security_mmap_file() by default?
>
> Mapping the enclave will still go through security_mmap_file(), the extra
> security_enclave_load() hook allows the mmap() to use PROT_NONE.
>
>>> If it's going to be in an arbitrary file, then I think the signature needs to be more like:
>>>
>>> int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t sigstruct_offset,
>>> const sgx_sigstruct *sigstruct);
>>>
>>> So that the LSM still has the opportunity to base its decision on the contents of the
>>> SIGSTRUCT. Actually, we need that change regardless.
>>
>> Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just
>> a pointer, because the VMA could be looked up using the pointer and the
>> offset would then be (pointer - vma->vm_start)?
>
> VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
> said, why does the LSM need the VMA? E.g. why not this?
>
> int security_enclave_init(struct file *file, struct sgx_sigstruct *sigstruct);
>
>>>> Loosely speaking, an enclave (including initial contents of all of its pages and their
>>> permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision resistant
>>> property of SHA-2). So only one is needed for a decision, and either one would lead to the
>>> same decision. So I don't see anything making any sense here.
>>>>
>>>> Theoretically speaking, if LSM can make a decision at EINIT by means of
>>> security_enclave_load(), then security_enclave_load() is never needed.
>>>>
>>>> In practice, I support keeping both because security_enclave_load() can only approve an
>>> enumerable set while security_enclave_load() can approve a non-enumerable set of enclaves.
>>> Moreover, in order to determine the validity of a MRENCLAVE (as in development of a policy
>>> or in creation of a white/black list), system admins will need the audit log produced by
>>> security_enclave_load().
>>>
>>> I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
>>> up. Also, security_enclave_load() provides no protection against loading a
>>> mishmash of two different enclave files. I see security_enclave_init() as
>>> "verify this SIGSTRUCT against your policy on who may sign enclaves and/or
>>> grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
>>> "implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
>>> check enclave files for some label."
>>
>> Sorry for the confusion. I was saying the same thing except that the decision
>> of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
>> prototype of security_enclave_load(), I think we are on the same page. I made
>> the above comment to object to the idea of "require that the sigstruct be
>> supplied before any EADD operations so that the maxperm decisions can depend
>> on the sigstruct".
>
> Except that having the sigstruct allows using the sigstruct as the proxy
> for the enclave. I think the last big disconnect is that Andy and I want
> to tie everything to an enclave-specific file, i.e. sigstruct, while you
> are proposing labeling /dev/sgx/enclave. If someone wants to cram several
> sigstructs into a single file, so be it, but using /dev/sgx/enclave means
> users can't do per-enclave permissions, period.
>
> What is your objection to working on the sigstruct?
>
>>>>>> Passing both would allow tying EXECMOD to /dev/sgx/enclave as
>>>>>> Cedric wanted (without having to play games and pass
>>>>>> /dev/sgx/enclave to security_enclave_load()), but I don't think
>>>>>> there's anything fundamentally broken with using .sigstruct for
>>>>>> EXECMOD. It requires more verbose labeling, but that's not a bad thing.
>>>>>
>>>>> The benefit of putting it on .sigstruct is that it can be per-enclave.
>>>>>
>>>>> As I understand it from Fedora packaging, the way this works on
>>>>> distros is generally that a package will include some files and
>>>>> their associated labels, and, if the package needs EXECMOD, then the
>>>>> files are labeled with EXECMOD and the author of the relevant code might get a dirty
>>> look.
>>>>>
>>>>> This could translate to the author of an exclave that needs RWX
>>>>> regions getting a dirty look without leaking this permission into other enclaves.
>>>>>
>>>>> (In my opinion, the dirty looks are actually the best security
>>>>> benefit of the entire concept of LSMs making RWX difficult. A
>>>>> sufficiently creative attacker can almost always bypass W^X
>>>>> restrictions once they’ve pwned you, but W^X makes it harder to pwn
>>>>> you in the first place, and SELinux makes it really obvious when
>>>>> packaging a program that doesn’t respect W^X. The upshot is that a
>>>>> lot of programs got fixed.)
>>>>
>>>> I'm lost here. Dynamically linked enclaves, if running on SGX2, would need RW->RX, i.e.
>>> FILE__EXECMOD on /dev/sgx/enclave. But they never need RWX, i.e. PROCESS__EXECMEM.
>>>
>>> Hmm. If we want to make this distinction, we need something a big richer
>>> than my proposed callbacks. A check of the actual mprotect() / mmap()
>>> permissions would also be needed. Specifically, allowing MAXPERM=RWX
>>> wouldn't imply that PROT_WRITE | PROT_EXEC is allowed.
>
> Actually, I think we do have everything we need from an LSM perspective.
> LSMs just need to understand that sgx_enclave_load() with a NULL vma
> implies a transition from RW. For example, SELinux would interpret
> sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
>
> As Cedric mentioned earlier, the host process doesn't necessarily know
> which pages will end up RW vs RX, i.e. sgx_enclave_load(NULL, RX)
> already has to be invoked at runtime, and when that happens, the kernel
> can take the opportunity to change the VMAs from MAY_RW to MAY_RX.
>
> For simplicity in the kernel and clarity in userspace, it makes sense to
> require an explicit ioctl() to add the to-be-EAUG'd range. That just
> leaves us wanting an ioctl() to set the post-EACCEPT{COPY} permissions.
>
> E.g.:
>
> ioctl(<prefix>_ADD_REGION, { NULL }) /* NULL == EAUG, MAY_RW */
>
> mprotect(addr, size, RW);
> ...
>
> EACCEPTCOPY -> EAUG /* page fault handler */
>
> ioctl(<prefix>_ACTIVATE_REGION, { addr, size, RX}) /* MAY_RX */
>
> mprotect(addr, size, RX);
>
> ...
>
> And making ACTIVATE_REGION a single-shot per page eliminates the need for
> the MAXPERMS concept (see below).
>
>> If we keep only one MAXPERM, wouldn't this be the current behavior of
>> mmap()/mprotect()?
>>
>> To be a bit more clear, system admin sets MAXPERM upper bound in the form of
>> FILE__{READ|WRITE|EXECUTE|EXECMOD} of /dev/sgx/enclave. Then for a
>> process/enclave, if what it requires falls below what's allowed on
>> /dev/sgx/enclave, then everything will just work. Otherwise, it fails in the
>> form of -EPERM returned from mmap()/mprotect(). Please note that MAXPERM here
>> applies to "runtime" permissions, while "initial" permissions are taken care
>> of by security_enclave_{load|init}. "initial" permissions could be more
>> permissive than "runtime" permissions, e.g., RX is still required for initial
>> code pages even though system admins could disable dynamically loaded code
>> pages by *not* giving FILE__{EXECUTE|EXECMOD}. Therefore, the "initial"
>> mapping would still have to be done by the driver (to bypass LSM), either via
>> a new ioctl or as part of IOC_EINIT.
>
> Aha!
>
> Starting with Cedric's assertion that initial permissions can be taken
> directly from SECINFO:
>
> - Initial permissions for *EADD* pages are explicitly handled via
> sgx_enclave_load() with the exact SECINFO permissions.
>
> - Initial permissions for *EAUG* are unconditionally RW. EACCEPTCOPY
> requires the target EPC page to be RW, and EACCEPT with RO is useless.
>
> - Runtime permissions break down as follows:
> R - N/A, subset of RW (EAUG)
> W - N/A, subset of RW (EAUG) and x86 paging can't do W
> X - N/A, subset of RX (x86 paging can't do XO)
> RW - Handled by EAUG LSM hook (uses RW unconditionally)
> WX - N/A, subset of RWX (x86 paging can't do WX)
> RX - Handled by ACTIVATE_REGION
> RWX - Handled by ACTIVATE_REGION
>
> In other words, if we define the SGX -> LSM calls as follows (minus the
> file pointer and other params for brevity):
>
> - <prefix>_ACTIVATE_REGION(vma, perms) -> sgx_enclave_load(NULL, perms)
>
> - <prefix>_ADD_REGION(vma) -> sgx_enclave_load(vma, SECINFO.perms)
>
> - <prefix>_ADD_REGION(NULL) -> sgx_enclave_load(NULL, RW)
>
> then SGX and LSMs have all the information and hooks needed. The catch
> is that the LSM semantics of sgx_enclave_load(..., RW) would need to be
> different than normal shared memory, e.g. FILE__WRITE should *not* be
> required, but that's ok since it's an SGX specific hook. And if for some
> reason an LSM wanted to gate access to EAUG *without* FILE__EXECMOD, it'd
> have the necessary information to do so.
Assuming that sgx_enclave_load() is a LSM hook (probably named
security_enclave_load() instead), then:
a) Does the sigstruct file get passed to this hook in every case, even
when vma is NULL? I think the answer is yes, just want to confirm.
b) Should we use a different hook for ACTIVATE_REGION than for
ADD_REGION or is the distinction between them irrelevant/unnecessary
from an access control point of view? At present LSM/SELinux won't be
able to distinguish ACTIVATE_REGION(vma, RW) from ADD_REGION(NULL) above
since they will both invoke the same hook with the same arguments IIUC.
Does it matter? It's ok if the answer is no, just want to confirm.
c) Is there still also a separate security_enclave_init() hook that will
be called, and if so, how does it differ and when is it called relative
to security_enclave_load()?
d) What checks were you envisioning each of these calls making?
With the separate security_enclave_*() hooks, we could define and use
new ENCLAVE__* permissions, e.g. ENCLAVE__LOAD, ENCLAVE__INIT,
ENCLAVE__EXECUTE, ENCLAVE__EXECMEM, ENCLAVE__EXECMOD, if we want to
distinguish these operations from regular file mmap/mprotect operations.
>
> The userspace changes are fairly minimal:
>
> - For SGX1, use PROT_NONE for the initial mmap() and refactor ADD_PAGE
> to ADD_REGION.
>
> - For SGX2, do an explicit ADD_REGION on the ranges to be EAUG'd, and an
> ACTIVATE_REGION to make a region RX or R (no extra ioctl() required to
> keep RW permissions).
>
> Because ACTIVATE_REGION can only be done once per page, to do *abitrary*
> mprotect() transitions, userspace would need to set the added/activated
> permissions to be a superset of the transitions, e.g. RW -> RX would
> require RWX, but that's a non-issue.
>
> - For SGX1 it's a nop since it's impossible to change the EPCM
> permissions, i.e. the page would need to be RWX regardless.
>
> - For SGX2, userspace can suck it up and request RWX to do completely
> arbitrary transitions (working as intended), or the kernel can support
> trimming (removing) pages from an enclave, which would allow userspace
> to do "arbitrary" transitions by first removing the page.
>
^ permalink raw reply
* Re: [PATCH 4/7] vfs: Add superblock notifications
From: Jann Horn @ 2019-05-29 14:16 UTC (permalink / raw)
To: David Howells
Cc: Al Viro, raven, linux-fsdevel, Linux API, linux-block, keyrings,
linux-security-module, kernel list
In-Reply-To: <24577.1559134719@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 2:58 PM David Howells <dhowells@redhat.com> wrote:
> Jann Horn <jannh@google.com> wrote:
> > It might make sense to require that the path points to the root inode
> > of the superblock? That way you wouldn't be able to do this on a bind
> > mount that exposes part of a shared filesystem to a container.
>
> Why prevent that? It doesn't prevent the container denizen from watching a
> bind mount that exposes the root of a shared filesystem into a container.
Well, yes, but if you expose the root of the shared filesystem to the
container, the container is probably meant to have a higher level of
access than if only a bind mount is exposed? But I don't know.
> It probably makes sense to permit the LSM to rule on whether a watch may be
> emplaced, however.
We should have some sort of reasonable policy outside of LSM code
though - the kernel should still be secure even if no LSMs are built
into it.
> > > + }
> > > + }
> > > + up_write(&s->s_umount);
> > > + if (ret < 0)
> > > + kfree(watch);
> > > + } else if (s->s_watchers) {
> >
> > This should probably have something like a READ_ONCE() for clarity?
>
> Note that I think I'll rearrange this to:
>
> } else {
> ret = -EBADSLT;
> if (s->s_watchers) {
> down_write(&s->s_umount);
> ret = remove_watch_from_object(s->s_watchers, wqueue,
> s->s_unique_id, false);
> up_write(&s->s_umount);
> }
> }
>
> I'm not sure READ_ONCE() is necessary, since s_watchers can only be
> instantiated once and the watch list then persists until the superblock is
> deactivated. Furthermore, by the time deactivate_locked_super() is called, we
> can't be calling sb_notify() on it as it's become inaccessible.
>
> So if we see s->s_watchers as non-NULL, we should not see anything different
> inside the lock. In fact, I should be able to rewrite the above to:
>
> } else {
> ret = -EBADSLT;
> wlist = s->s_watchers;
> if (wlist) {
> down_write(&s->s_umount);
> ret = remove_watch_from_object(wlist, wqueue,
> s->s_unique_id, false);
> up_write(&s->s_umount);
> }
> }
I'm extremely twitchy when it comes to code like this because AFAIK
gcc at least used to sometimes turn code that read a value from memory
and then used it multiple times into something with multiple memory
reads, leading to critical security vulnerabilities; see e.g. slide 36
of <https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf>.
I am not aware of any spec that requires the compiler to only perform
one read from the memory location in code like this.
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Jan Kara @ 2019-05-29 14:25 UTC (permalink / raw)
To: Amir Goldstein
Cc: David Howells, Al Viro, Ian Kent, linux-fsdevel, linux-api,
linux-block, keyrings, LSM List, linux-kernel, Jan Kara
In-Reply-To: <CAOQ4uxjC1M7jwjd9zSaSa6UW2dbEjc+ZbFSo7j9F1YHAQxQ8LQ@mail.gmail.com>
On Wed 29-05-19 09:33:35, Amir Goldstein wrote:
> On Tue, May 28, 2019 at 7:03 PM David Howells <dhowells@redhat.com> wrote:
> >
> >
> > Hi Al,
> >
> > Here's a set of patches to add a general variable-length notification queue
> > concept and to add sources of events for:
> >
> > (1) Mount topology events, such as mounting, unmounting, mount expiry,
> > mount reconfiguration.
> >
> > (2) Superblock events, such as R/W<->R/O changes, quota overrun and I/O
> > errors (not complete yet).
> >
> > (3) Block layer events, such as I/O errors.
> >
> > (4) Key/keyring events, such as creating, linking and removal of keys.
> >
> > One of the reasons for this is so that we can remove the issue of processes
> > having to repeatedly and regularly scan /proc/mounts, which has proven to
> > be a system performance problem. To further aid this, the fsinfo() syscall
> > on which this patch series depends, provides a way to access superblock and
> > mount information in binary form without the need to parse /proc/mounts.
> >
> >
> > Design decisions:
> >
> > (1) A misc chardev is used to create and open a ring buffer:
> >
> > fd = open("/dev/watch_queue", O_RDWR);
> >
> > which is then configured and mmap'd into userspace:
> >
> > ioctl(fd, IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
> > ioctl(fd, IOC_WATCH_QUEUE_SET_FILTER, &filter);
> > buf = mmap(NULL, BUF_SIZE * page_size, PROT_READ | PROT_WRITE,
> > MAP_SHARED, fd, 0);
> >
> > The fd cannot be read or written (though there is a facility to use
> > write to inject records for debugging) and userspace just pulls data
> > directly out of the buffer.
> >
> > (2) The ring index pointers are stored inside the ring and are thus
> > accessible to userspace. Userspace should only update the tail
> > pointer and never the head pointer or risk breaking the buffer. The
> > kernel checks that the pointers appear valid before trying to use
> > them. A 'skip' record is maintained around the pointers.
> >
> > (3) poll() can be used to wait for data to appear in the buffer.
> >
> > (4) Records in the buffer are binary, typed and have a length so that they
> > can be of varying size.
> >
> > This means that multiple heterogeneous sources can share a common
> > buffer. Tags may be specified when a watchpoint is created to help
> > distinguish the sources.
> >
> > (5) The queue is reusable as there are 16 million types available, of
> > which I've used 4, so there is scope for others to be used.
> >
> > (6) Records are filterable as types have up to 256 subtypes that can be
> > individually filtered. Other filtration is also available.
> >
> > (7) Each time the buffer is opened, a new buffer is created - this means
> > that there's no interference between watchers.
> >
> > (8) When recording a notification, the kernel will not sleep, but will
> > rather mark a queue as overrun if there's insufficient space, thereby
> > avoiding userspace causing the kernel to hang.
> >
> > (9) The 'watchpoint' should be specific where possible, meaning that you
> > specify the object that you want to watch.
> >
> > (10) The buffer is created and then watchpoints are attached to it, using
> > one of:
> >
> > keyctl_watch_key(KEY_SPEC_SESSION_KEYRING, fd, 0x01);
> > mount_notify(AT_FDCWD, "/", 0, fd, 0x02);
> > sb_notify(AT_FDCWD, "/mnt", 0, fd, 0x03);
> >
> > where in all three cases, fd indicates the queue and the number after
> > is a tag between 0 and 255.
> >
> > (11) The watch must be removed if either the watch buffer is destroyed or
> > the watched object is destroyed.
> >
> >
> > Things I want to avoid:
> >
> > (1) Introducing features that make the core VFS dependent on the network
> > stack or networking namespaces (ie. usage of netlink).
> >
> > (2) Dumping all this stuff into dmesg and having a daemon that sits there
> > parsing the output and distributing it as this then puts the
> > responsibility for security into userspace and makes handling
> > namespaces tricky. Further, dmesg might not exist or might be
> > inaccessible inside a container.
> >
> > (3) Letting users see events they shouldn't be able to see.
> >
> >
> > Further things that could be considered:
> >
> > (1) Adding a keyctl call to allow a watch on a keyring to be extended to
> > "children" of that keyring, such that the watch is removed from the
> > child if it is unlinked from the keyring.
> >
> > (2) Adding global superblock event queue.
> >
> > (3) Propagating watches to child superblock over automounts.
> >
>
> David,
>
> I am interested to know how you envision filesystem notifications would
> look with this interface.
>
> fanotify can certainly benefit from providing a ring buffer interface to read
> events.
>
> From what I have seen, a common practice of users is to monitor mounts
> (somehow) and place FAN_MARK_MOUNT fanotify watches dynamically.
> It'd be good if those users can use a single watch mechanism/API for
> watching the mount namespace and filesystem events within mounts.
>
> A similar usability concern is with sb_notify and FAN_MARK_FILESYSTEM.
> It provides users with two complete different mechanisms to watch error
> and filesystem events. That is generally not a good thing to have.
>
> I am not asking that you implement fs_notify() before merging sb_notify()
> and I understand that you have a use case for sb_notify().
> I am asking that you show me the path towards a unified API (how a
> typical program would look like), so that we know before merging your
> new API that it could be extended to accommodate fsnotify events
> where the final result will look wholesome to users.
Are you sure we want to combine notification about file changes etc. with
administrator-type notifications about the filesystem? To me these two
sound like rather different (although sometimes related) things.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Greg KH @ 2019-05-29 15:10 UTC (permalink / raw)
To: Jan Kara
Cc: Amir Goldstein, David Howells, Al Viro, Ian Kent, linux-fsdevel,
linux-api, linux-block, keyrings, LSM List, linux-kernel
In-Reply-To: <20190529142504.GC32147@quack2.suse.cz>
On Wed, May 29, 2019 at 04:25:04PM +0200, Jan Kara wrote:
> > I am not asking that you implement fs_notify() before merging sb_notify()
> > and I understand that you have a use case for sb_notify().
> > I am asking that you show me the path towards a unified API (how a
> > typical program would look like), so that we know before merging your
> > new API that it could be extended to accommodate fsnotify events
> > where the final result will look wholesome to users.
>
> Are you sure we want to combine notification about file changes etc. with
> administrator-type notifications about the filesystem? To me these two
> sound like rather different (although sometimes related) things.
This patchset is looking to create a "generic" kernel notification
system, so I think the question is valid. It's up to the requestor to
ask for the specific type of notification.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Casey Schaufler @ 2019-05-29 15:41 UTC (permalink / raw)
To: David Howells, Greg KH
Cc: viro, raven, linux-fsdevel, linux-api, linux-block, keyrings,
linux-security-module, linux-kernel, casey
In-Reply-To: <31751.1559120984@warthog.procyon.org.uk>
On 5/29/2019 2:09 AM, David Howells wrote:
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
>>> (3) Letting users see events they shouldn't be able to see.
>> How are you handling namespaces then? Are they determined by the
>> namespace of the process that opened the original device handle, or the
>> namespace that made the new syscall for the events to "start flowing"?
> So far I haven't had to deal directly with namespaces.
>
> mount_notify() requires you to have access to the mountpoint you want to watch
> - and the entire tree rooted there is in one namespace, so your event sources
> are restricted to that namespace. Further, mount objects don't themselves
> have any other namespaces, not even a user_ns.
>
> sb_notify() requires you to have access to the superblock you want to watch.
> superblocks aren't directly namespaced as a class, though individual
> superblocks may participate in particular namespaces (ipc, net, etc.). I'm
> thinking some of these should be marked unwatchable (all pseudo superblocks,
> kernfs-class, proc, for example).
>
> Superblocks, however, do each have a user_ns - but you were allowed to access
> the superblock by pathwalk, so you must have some access to the user_ns - I
> think.
>
> KEYCTL_NOTIFY requires you to have View access on the key you're watching.
> Currently, keys have no real namespace restrictions, though I have patches to
> include a namespace tag in the lookup criteria.
>
> block_notify() doesn't require any direct access since you're watching a
> global queue and there is no blockdev namespacing. LSMs are given the option
> to filter events, though. The thought here is that if you can access dmesg,
> you should be able to watch for blockdev events.
>
>
> Actually, thinking further on this, restricting access to events is trickier
> than I thought and than perhaps Casey was suggesting.
>
> Say you're watching a mount object and someone in a different user_ns
> namespace or with a different security label mounts on it. What governs
> whether you are allowed to see the event?
Conceptually it should be simple, but we have a variety of different
policies in the core OS, never mind what goes on inside the LSMs.
If you want to treat a notification like a signal you would only deliver
it if the process that performed the action that triggered the event
has the same UID as the process receiving the notification. Should you
decide to treat it like an IP packet only the LSMs would filter delivery.
If there are mode bits on the thing being watched shouldn't you respect
them?
> You're watching the object for changes - and it *has* changed. Further, you
> might be able to see the result of this change by other means (/proc/mounts,
> for instance).
>
> Should you be denied the event based on the security model?
From a subject/object model view there are two objects and one
subject involved. The subject (active entity) is the process that
changes the first object, triggering an event. The watching process
(that will receive the notification) is the second object, because
its state will change (be written to) when the notification is
delivered. For the watching process to receive the notification
the changing process needs write access to the watching process.
The indirection of the notification mechanism isn't relevant.
If the changing process couldn't directly notify the watching process
it shouldn't be able to do it indirectly, either.
> On the other hand, if you're watching a tree of mount objects, it could be
> argued that you should be denied access to events on any mount object you
> can't reach by pathwalk.
>
> On the third hand, if you can see it in /proc/mounts or by fsinfo(), you
> should get an event for it.
Right. We've done a pretty good job of muddling the security
landscape by adding spiffy features to make life easier for
particular use cases. /proc is chuck full of examples. Objects
that can be viewed in many different ways make for confusing
security models. Try explaining /proc/234/fd/2 to a security
theory student.
>> How are you handling namespaces then?
> So to go back to the original question. At the moment they haven't impinged
> directly and I haven't had to deal with them directly. There are indirect
> namespace restrictions that I get for free just due to pathwalk, for instance.
>
> David
^ permalink raw reply
* Re: [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications
From: Amir Goldstein @ 2019-05-29 15:53 UTC (permalink / raw)
To: Jan Kara
Cc: David Howells, Al Viro, Ian Kent, linux-fsdevel, linux-api,
linux-block, keyrings, LSM List, linux-kernel
In-Reply-To: <20190529142504.GC32147@quack2.suse.cz>
> > David,
> >
> > I am interested to know how you envision filesystem notifications would
> > look with this interface.
> >
> > fanotify can certainly benefit from providing a ring buffer interface to read
> > events.
> >
> > From what I have seen, a common practice of users is to monitor mounts
> > (somehow) and place FAN_MARK_MOUNT fanotify watches dynamically.
> > It'd be good if those users can use a single watch mechanism/API for
> > watching the mount namespace and filesystem events within mounts.
> >
> > A similar usability concern is with sb_notify and FAN_MARK_FILESYSTEM.
> > It provides users with two complete different mechanisms to watch error
> > and filesystem events. That is generally not a good thing to have.
> >
> > I am not asking that you implement fs_notify() before merging sb_notify()
> > and I understand that you have a use case for sb_notify().
> > I am asking that you show me the path towards a unified API (how a
> > typical program would look like), so that we know before merging your
> > new API that it could be extended to accommodate fsnotify events
> > where the final result will look wholesome to users.
>
> Are you sure we want to combine notification about file changes etc. with
> administrator-type notifications about the filesystem? To me these two
> sound like rather different (although sometimes related) things.
>
Well I am sure that ring buffer for fanotify events would be useful, so
seeing that David is proposing a generic notification mechanism, I wanted
to know how that mechanism could best share infrastructure with fsnotify.
But apart from that I foresee the questions from users about why the
mount notification API and filesystem events API do not have better
integration.
The way I see it, the notification queue can serve several classes
of notifications and fsnotify could be one of those classes
(at least FAN_CLASS_NOTIF fits nicely to the model).
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Casey Schaufler @ 2019-05-29 15:53 UTC (permalink / raw)
To: David Howells, Jann Horn
Cc: Al Viro, raven, linux-fsdevel, Linux API, linux-block, keyrings,
linux-security-module, kernel list
In-Reply-To: <14347.1559127657@warthog.procyon.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 1386 bytes --]
On 5/29/2019 4:00 AM, David Howells wrote:
> Jann Horn <jannh@google.com> wrote:
>
>>> +void post_mount_notification(struct mount *changed,
>>> + struct mount_notification *notify)
>>> +{
>>> + const struct cred *cred = current_cred();
>> This current_cred() looks bogus to me. Can't mount topology changes
>> come from all sorts of places? For example, umount_mnt() from
>> umount_tree() from dissolve_on_fput() from __fput(), which could
>> happen pretty much anywhere depending on where the last reference gets
>> dropped?
> IIRC, that's what Casey argued is the right thing to do from a security PoV.
> Casey?
You need to identify the credential of the subject that triggered
the event. If it isn't current_cred(), the cred needs to be passed
in to post_mount_notification(), or derived by some other means.
> Maybe I should pass in NULL creds in the case that an event is being generated
> because an object is being destroyed due to the last usage[*] being removed.
You should pass the cred of the process that removed the
last usage. If the last usage was removed by something like
the power being turned off on a disk drive a system cred
should be used. Someone or something caused the event. It can
be important who it was.
> [*] Usage, not ref - Superblocks are a bit weird in their accounting.
>
> David
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: David Howells @ 2019-05-29 16:06 UTC (permalink / raw)
To: Greg KH
Cc: dhowells, viro, raven, linux-fsdevel, linux-api, linux-block,
keyrings, linux-security-module, linux-kernel
In-Reply-To: <20190528231218.GA28384@kroah.com>
Greg KH <gregkh@linuxfoundation.org> wrote:
> > kref_put() could potentially add an unnecessary extra stack frame and would
> > seem to be best avoided, though an optimising compiler ought to be able to
> > inline if it can.
>
> If kref_put() is on your fast path, you have worse problems (kfree isn't
> fast, right?)
>
> Anyway, it's an inline function, how can it add an extra stack frame?
The call to the function pointer. Hopefully the compiler will optimise that
away for an inlineable function.
> > Are you now on the convert all refcounts to krefs path?
>
> "now"? Remember, I wrote kref all those years ago,
Yes - and I thought it wasn't a good idea at the time. But this is the first
time you've mentioned it to me, let alone pushed to change to it, that I
recall.
> everyone should use
> it. It saves us having to audit the same pattern over and over again.
> And, even nicer, it uses a refcount now, and as you are trying to
> reference count an object, it is exactly what this was written for.
>
> So yes, I do think it should be used here, unless it is deemed to not
> fit the pattern/usage model.
kref_put() enforces a very specific destructor signature. I know of places
where that doesn't work because the destructor takes more than one argument
(granted that this is not the case here). So why does kref_put() exist at
all? Why not kref_dec_and_test()?
Why doesn't refcount_t get merged into kref, or vice versa? Having both would
seem redundant.
Mind you, I've been gradually reverting atomic_t-to-refcount_t conversions
because it seems I'm not allowed refcount_inc/dec_return() and I want to get
at the point refcount for tracing purposes.
> > > > +module_exit(watch_queue_exit);
> > >
> > > module_misc_device()?
> >
> > warthog>git grep module_misc_device -- Documentation/
> > warthog1>
>
> Do I have to document all helper macros?
If you add an API, documenting it is your privilege ;-) It's an important
test of the API - if you can't describe it, it's probably wrong.
Now I will grant that you didn't add that function...
> Anyway, it saves you boilerplate code, but if built in, it's at the module
> init level, not the fs init level, like you are asking for here. So that
> might not work, it's your call.
Actually, I probably shouldn't have a module exit function. It can't be a
module as it's called by core code. I'll switch to builtin_misc_device().
> And how does the tracing and perf ring buffers do this without needing
> volatile? Why not use the same type of interface they provide, as it's
> always good to share code that has already had all of the nasty corner
> cases worked out.
I've no idea how trace does it - or even where - or even if. As far as I can
see, grepping for mmap in kernel/trace/*, there's no mmap support.
Reading Documentation/trace/ring-buffer-design.txt the trace subsystem has
some sort of transient page fifo which is a lot more complicated than what I
want and doesn't look like it'll be mmap'able.
Looking at the perf ring buffer, there appears to be a missing barrier in
perf_aux_output_end():
rb->user_page->aux_head = rb->aux_head;
should be:
smp_store_release(&rb->user_page->aux_head, rb->aux_head);
It should also be using smp_load_acquire(). See
Documentation/core-api/circular-buffers.rst
And a (partial) patch has been proposed: https://lkml.org/lkml/2018/5/10/249
David
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Jann Horn @ 2019-05-29 16:12 UTC (permalink / raw)
To: Casey Schaufler
Cc: David Howells, Al Viro, raven, linux-fsdevel, Linux API,
linux-block, keyrings, linux-security-module, kernel list,
Andy Lutomirski
In-Reply-To: <312a138c-e5b2-4bfb-b50b-40c82c55773f@schaufler-ca.com>
On Wed, May 29, 2019 at 5:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/29/2019 4:00 AM, David Howells wrote:
> > Jann Horn <jannh@google.com> wrote:
> >
> >>> +void post_mount_notification(struct mount *changed,
> >>> + struct mount_notification *notify)
> >>> +{
> >>> + const struct cred *cred = current_cred();
> >> This current_cred() looks bogus to me. Can't mount topology changes
> >> come from all sorts of places? For example, umount_mnt() from
> >> umount_tree() from dissolve_on_fput() from __fput(), which could
> >> happen pretty much anywhere depending on where the last reference gets
> >> dropped?
> > IIRC, that's what Casey argued is the right thing to do from a security PoV.
> > Casey?
>
> You need to identify the credential of the subject that triggered
> the event. If it isn't current_cred(), the cred needs to be passed
> in to post_mount_notification(), or derived by some other means.
>
> > Maybe I should pass in NULL creds in the case that an event is being generated
> > because an object is being destroyed due to the last usage[*] being removed.
>
> You should pass the cred of the process that removed the
> last usage. If the last usage was removed by something like
> the power being turned off on a disk drive a system cred
> should be used. Someone or something caused the event. It can
> be important who it was.
The kernel's normal security model means that you should be able to
e.g. accept FDs that random processes send you and perform
read()/write() calls on them without acting as a subject in any
security checks; let alone close(). If you send a file descriptor over
a unix domain socket and the unix domain socket is garbage collected,
for example, I think the close() will just come from some random,
completely unrelated task that happens to trigger the garbage
collector?
Also, I think if someone does I/O via io_uring, I think the caller's
credentials for read/write operations will probably just be normal
kernel creds?
Here the checks probably aren't all that important, but in other
places, when people try to use an LSM as the primary line of defense,
checks that don't align with the kernel's normal security model might
lead to a bunch of problems.
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Casey Schaufler @ 2019-05-29 17:04 UTC (permalink / raw)
To: Jann Horn
Cc: David Howells, Al Viro, raven, linux-fsdevel, Linux API,
linux-block, keyrings, linux-security-module, kernel list,
Andy Lutomirski, casey
In-Reply-To: <CAG48ez2KMrTBFzO9p8GvduXruz+FNLPyhc2YivHePsgViEoT1g@mail.gmail.com>
On 5/29/2019 9:12 AM, Jann Horn wrote:
> On Wed, May 29, 2019 at 5:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 5/29/2019 4:00 AM, David Howells wrote:
>>> Jann Horn <jannh@google.com> wrote:
>>>
>>>>> +void post_mount_notification(struct mount *changed,
>>>>> + struct mount_notification *notify)
>>>>> +{
>>>>> + const struct cred *cred = current_cred();
>>>> This current_cred() looks bogus to me. Can't mount topology changes
>>>> come from all sorts of places? For example, umount_mnt() from
>>>> umount_tree() from dissolve_on_fput() from __fput(), which could
>>>> happen pretty much anywhere depending on where the last reference gets
>>>> dropped?
>>> IIRC, that's what Casey argued is the right thing to do from a security PoV.
>>> Casey?
>> You need to identify the credential of the subject that triggered
>> the event. If it isn't current_cred(), the cred needs to be passed
>> in to post_mount_notification(), or derived by some other means.
>>
>>> Maybe I should pass in NULL creds in the case that an event is being generated
>>> because an object is being destroyed due to the last usage[*] being removed.
>> You should pass the cred of the process that removed the
>> last usage. If the last usage was removed by something like
>> the power being turned off on a disk drive a system cred
>> should be used. Someone or something caused the event. It can
>> be important who it was.
> The kernel's normal security model means that you should be able to
> e.g. accept FDs that random processes send you and perform
> read()/write() calls on them without acting as a subject in any
> security checks; let alone close().
Passed file descriptors are an anomaly in the security model
that (in this developer's opinion) should have never been
included. More than one of the "B" level UNIX systems disabled
them outright.
> If you send a file descriptor over
> a unix domain socket and the unix domain socket is garbage collected,
> for example, I think the close() will just come from some random,
> completely unrelated task that happens to trigger the garbage
> collector?
I never said this was going to be easy or pleasant.
Who destroyed the UDS? It didn't just spontaneously become
garbage. Well, not on modern Linux filesystems, anyway.
> Also, I think if someone does I/O via io_uring, I think the caller's
> credentials for read/write operations will probably just be normal
> kernel creds?
>
> Here the checks probably aren't all that important, but in other
> places, when people try to use an LSM as the primary line of defense,
> checks that don't align with the kernel's normal security model might
> lead to a bunch of problems.
The kernel does not have a "normal security model". It has a
collection of disparate and almost but not quite contradictory
models for the various objects and mechanisms it implements.
It already has a bunch of problems, we're just used to them.
I can only send a signal to a process with the same UID. Why doesn't
a process have mode bits so that I could get signals from my group?
Why do IPC object have creator bits, while files don't?
Why can I send a file descriptor over a UDS, but not a message queue?
Why can't I set the mode bits on a symlink?
What can go wrong if I don't map groups into a user namespace?
LSMs (SELinux and Smack, which are classic mandatory access control
systems in particular) are more consistent, but still have to deal with
some of these differences. A symlink gets a Smack label, for example.
The point being that it's very easy to add new mechanisms that do
wonderful things but that introduce unforeseen ways to bypass one
or more of the existing protections.
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Andy Lutomirski @ 2019-05-29 17:13 UTC (permalink / raw)
To: Casey Schaufler
Cc: David Howells, Jann Horn, Al Viro, raven, linux-fsdevel,
Linux API, linux-block, keyrings, linux-security-module,
kernel list
In-Reply-To: <312a138c-e5b2-4bfb-b50b-40c82c55773f@schaufler-ca.com>
> On May 29, 2019, at 8:53 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> On 5/29/2019 4:00 AM, David Howells wrote:
>> Jann Horn <jannh@google.com> wrote:
>>
>>>> +void post_mount_notification(struct mount *changed,
>>>> + struct mount_notification *notify)
>>>> +{
>>>> + const struct cred *cred = current_cred();
>>> This current_cred() looks bogus to me. Can't mount topology changes
>>> come from all sorts of places? For example, umount_mnt() from
>>> umount_tree() from dissolve_on_fput() from __fput(), which could
>>> happen pretty much anywhere depending on where the last reference gets
>>> dropped?
>> IIRC, that's what Casey argued is the right thing to do from a security PoV.
>> Casey?
>
> You need to identify the credential of the subject that triggered
> the event. If it isn't current_cred(), the cred needs to be passed
> in to post_mount_notification(), or derived by some other means.
Taking a step back, why do we care who triggered the event? It seems to me that we should care whether the event happened and whether the *receiver* is permitted to know that.
(And receiver means whoever subscribed, presumably, not whoever called read() or mmap().)
^ permalink raw reply
* [PATCH] ima: use struct_size() in kzalloc()
From: Gustavo A. R. Silva @ 2019-05-29 16:53 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E. Hallyn
Cc: linux-integrity, linux-security-module, linux-kernel,
Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct foo {
int stuff;
struct boo entry[];
};
instance = kzalloc(sizeof(struct foo) + count * sizeof(struct boo), GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
security/integrity/ima/ima_template.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index b631b8bc7624..b945dff2ed14 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -281,9 +281,8 @@ static int ima_restore_template_data(struct ima_template_desc *template_desc,
int ret = 0;
int i;
- *entry = kzalloc(sizeof(**entry) +
- template_desc->num_fields * sizeof(struct ima_field_data),
- GFP_NOFS);
+ *entry = kzalloc(struct_size(*entry, template_data,
+ template_desc->num_fields), GFP_NOFS);
if (!*entry)
return -ENOMEM;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Casey Schaufler @ 2019-05-29 17:46 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Jann Horn, Al Viro, raven, linux-fsdevel,
Linux API, linux-block, keyrings, linux-security-module,
kernel list, casey
In-Reply-To: <4552118F-BE9B-4905-BF0F-A53DC13D5A82@amacapital.net>
On 5/29/2019 10:13 AM, Andy Lutomirski wrote:
>
>> On May 29, 2019, at 8:53 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>> On 5/29/2019 4:00 AM, David Howells wrote:
>>> Jann Horn <jannh@google.com> wrote:
>>>
>>>>> +void post_mount_notification(struct mount *changed,
>>>>> + struct mount_notification *notify)
>>>>> +{
>>>>> + const struct cred *cred = current_cred();
>>>> This current_cred() looks bogus to me. Can't mount topology changes
>>>> come from all sorts of places? For example, umount_mnt() from
>>>> umount_tree() from dissolve_on_fput() from __fput(), which could
>>>> happen pretty much anywhere depending on where the last reference gets
>>>> dropped?
>>> IIRC, that's what Casey argued is the right thing to do from a security PoV.
>>> Casey?
>> You need to identify the credential of the subject that triggered
>> the event. If it isn't current_cred(), the cred needs to be passed
>> in to post_mount_notification(), or derived by some other means.
> Taking a step back, why do we care who triggered the event? It seems to me that we should care whether the event happened and whether the *receiver* is permitted to know that.
There are two filesystems, "dot" and "dash". I am not allowed
to communicate with Fred on the system, and all precautions have
been taken to ensure I cannot. Fred asks for notifications on
all mount activity. I perform actions that result in notifications
on "dot" and "dash". Fred receives notifications and interprets
them using Morse code. This is not OK. If Wilma, who *is* allowed
to communicate with Fred, does the same actions, he should be
allowed to get the messages via Morse.
The event is information. The information is generated as a
result of my or Wilma's action. Fred is passive in this access.
Fred is not "reading" the event. The event is being written to
Fred. My process is the subject, and Fred's the object.
Other security modelers may disagree. The models they produce
are going to be *very* complicated and will introduce agents and
intermediate objects to justify Fred's reception of an event as
a read operation.
> (And receiver means whoever subscribed, presumably, not whoever called read() or mmap().)
The receiver is the process that gets the event. There may
be more than one receiver, and the receivers may have different
credentials. Each needs to be checked separately.
Isn't this starting to sound like the discussions on kdbus?
I'm not sure if that deserves a :) or a :( but probably one of the two.
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: Jann Horn @ 2019-05-29 17:46 UTC (permalink / raw)
To: David Howells
Cc: Greg KH, Al Viro, raven, linux-fsdevel, Linux API, linux-block,
keyrings, linux-security-module, kernel list, Kees Cook,
Kernel Hardening
In-Reply-To: <31936.1559146000@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 6:07 PM David Howells <dhowells@redhat.com> wrote:
> Greg KH <gregkh@linuxfoundation.org> wrote:
> > everyone should use
> > it. It saves us having to audit the same pattern over and over again.
> > And, even nicer, it uses a refcount now, and as you are trying to
> > reference count an object, it is exactly what this was written for.
> >
> > So yes, I do think it should be used here, unless it is deemed to not
> > fit the pattern/usage model.
>
> kref_put() enforces a very specific destructor signature. I know of places
> where that doesn't work because the destructor takes more than one argument
> (granted that this is not the case here). So why does kref_put() exist at
> all? Why not kref_dec_and_test()?
>
> Why doesn't refcount_t get merged into kref, or vice versa? Having both would
> seem redundant.
>
> Mind you, I've been gradually reverting atomic_t-to-refcount_t conversions
> because it seems I'm not allowed refcount_inc/dec_return() and I want to get
> at the point refcount for tracing purposes.
Yeeech, that's horrible, please don't do that.
Does this mean that refcount_read() isn't sufficient for what you want
to do with tracing (because for some reason you actually need to know
the values atomically at the time of increment/decrement)?
^ 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