* 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
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Jann Horn @ 2019-05-29 18:11 UTC (permalink / raw)
To: Casey Schaufler
Cc: Andy Lutomirski, David Howells, Al Viro, raven, linux-fsdevel,
Linux API, linux-block, keyrings, linux-security-module,
kernel list
In-Reply-To: <058f227c-71ab-a6f4-00bf-b8782b3b2956@schaufler-ca.com>
On Wed, May 29, 2019 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> 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.
In other words, a classic covert channel. You can't really prevent two
cooperating processes from communicating through a covert channel on a
modern computer. You can transmit information through the scheduler,
through hyperthread resource sharing, through CPU data caches, through
disk contention, through page cache state, through RAM contention, and
probably dozens of other ways that I can't think of right now. There
have been plenty of papers that demonstrated things like an SSH
connection between two virtual machines without network access running
on the same physical host (<https://gruss.cc/files/hello.pdf>),
communication between a VM and a browser running on the host system,
and so on.
^ permalink raw reply
* Re: [PATCH v5 1/3] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Kees Cook @ 2019-05-29 18:41 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, 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-2-glider@google.com>
On Wed, May 29, 2019 at 02:38:10PM +0200, Alexander Potapenko wrote:
> 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>
I'm really looking forward to having this available! :)
Acked-by: Kees Cook <keescook@chromium.org>
> 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
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v5 2/3] mm: init: report memory auto-initialization features at boot time
From: Kees Cook @ 2019-05-29 18:43 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, 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-3-glider@google.com>
On Wed, May 29, 2019 at 02:38:11PM +0200, Alexander Potapenko wrote:
> 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>
Looks good to me!
Acked-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
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v5 3/3] lib: introduce test_meminit module
From: Kees Cook @ 2019-05-29 18:47 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, 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-4-glider@google.com>
On Wed, May 29, 2019 at 02:38:12PM +0200, Alexander Potapenko wrote:
> 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>
Yay tests :)
Acked-by: Kees Cook <keescook@chromium.org>
After this series lands, we can start looking at methods to provide
better tuning interfaces.
One idea I had for the page allocator (i.e. based on the prior identified
skb improvements) would be to give a "page filler" callback to the page
allocator. If the page filler was NULL, it would just zero it. This
might provide the best way to hook these things up.
For the slab, I think just having zeroing stats visible (as done in
grsecurity) combined with a list of slab names to be excluded read from
a boot param would get us full flexibility on the slab side of things.
And if a "commonly recognized list of excluded slabs" were identified,
we can just use that list by default.
-Kees
> 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
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Casey Schaufler @ 2019-05-29 19:28 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, David Howells, Al Viro, raven, linux-fsdevel,
Linux API, linux-block, keyrings, linux-security-module,
kernel list, casey
In-Reply-To: <CAG48ez2S+i2wxpWXVGpEAprgY9gtjxyejLfbZtrqu5YOkQ81Nw@mail.gmail.com>
On 5/29/2019 11:11 AM, Jann Horn wrote:
> On Wed, May 29, 2019 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> 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.
> In other words, a classic covert channel. You can't really prevent two
> cooperating processes from communicating through a covert channel on a
> modern computer.
That doesn't give you permission to design them in.
Plus, the LSMs that implement mandatory access controls
are going to want to intervene. No unclassified user
should see notifications caused by Top Secret users.
> You can transmit information through the scheduler,
> through hyperthread resource sharing, through CPU data caches, through
> disk contention, through page cache state, through RAM contention, and
> probably dozens of other ways that I can't think of right now.
Yeah, and there's been a lot of activity to reduce those,
which are hard to exploit, as opposed to this, which would
be trivial and obvious.
> There
> have been plenty of papers that demonstrated things like an SSH
> connection between two virtual machines without network access running
> on the same physical host (<https://gruss.cc/files/hello.pdf>),
> communication between a VM and a browser running on the host system,
> and so on.
So you're saying we shouldn't have mode bits on files because
spectre/meltdown makes them pointless?
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Jann Horn @ 2019-05-29 19:47 UTC (permalink / raw)
To: Casey Schaufler
Cc: Andy Lutomirski, David Howells, Al Viro, raven, linux-fsdevel,
Linux API, linux-block, keyrings, linux-security-module,
kernel list
In-Reply-To: <0cd823ca-4733-19ef-c13e-ed5ac8c63a0f@schaufler-ca.com>
On Wed, May 29, 2019 at 9:28 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/29/2019 11:11 AM, Jann Horn wrote:
> > On Wed, May 29, 2019 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> 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.
> > In other words, a classic covert channel. You can't really prevent two
> > cooperating processes from communicating through a covert channel on a
> > modern computer.
>
> That doesn't give you permission to design them in.
> Plus, the LSMs that implement mandatory access controls
> are going to want to intervene. No unclassified user
> should see notifications caused by Top Secret users.
But that's probably because they're worried about *side* channels, not
covert channels?
Talking about this in the context of (small) side channels: The
notification types introduced in this patch are mostly things that a
user would be able to observe anyway if they polled /proc/self/mounts,
right? It might make sense to align access controls based on that - if
you don't want it to be possible to observe events happening on some
mount points through this API, you should probably lock down
/proc/*/mounts equivalently, by introducing an LSM hook for "is @cred
allowed to see @mnt" or something like that - and if you want to
compare two cred structures, you could record the cred structure that
is responsible for the creation of the mount point, or something like
that.
For some of the other patches, I guess things get more tricky because
the notification exposes new information that wasn't really available
before.
> > You can transmit information through the scheduler,
> > through hyperthread resource sharing, through CPU data caches, through
> > disk contention, through page cache state, through RAM contention, and
> > probably dozens of other ways that I can't think of right now.
>
> Yeah, and there's been a lot of activity to reduce those,
> which are hard to exploit, as opposed to this, which would
> be trivial and obvious.
>
> > There
> > have been plenty of papers that demonstrated things like an SSH
> > connection between two virtual machines without network access running
> > on the same physical host (<https://gruss.cc/files/hello.pdf>),
> > communication between a VM and a browser running on the host system,
> > and so on.
>
> So you're saying we shouldn't have mode bits on files because
> spectre/meltdown makes them pointless?
spectre/meltdown are vulnerabilities that are being mitigated.
Microarchitectural covert channels are an accepted fact and I haven't
heard of anyone seriously considering trying to get rid of them all.
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Casey Schaufler @ 2019-05-29 20:50 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, David Howells, Al Viro, raven, linux-fsdevel,
Linux API, linux-block, keyrings, linux-security-module,
kernel list, casey
In-Reply-To: <CAG48ez0X7rKw-qfZm9i+8OLq7YccBRtV3aF-7hkQsfWaiTbuXg@mail.gmail.com>
On 5/29/2019 12:47 PM, Jann Horn wrote:
> On Wed, May 29, 2019 at 9:28 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 5/29/2019 11:11 AM, Jann Horn wrote:
>>> On Wed, May 29, 2019 at 7:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> 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.
>>> In other words, a classic covert channel. You can't really prevent two
>>> cooperating processes from communicating through a covert channel on a
>>> modern computer.
>> That doesn't give you permission to design them in.
>> Plus, the LSMs that implement mandatory access controls
>> are going to want to intervene. No unclassified user
>> should see notifications caused by Top Secret users.
> But that's probably because they're worried about *side* channels, not
> covert channels?
The security evaluators from the 1990's considered any channel
with greater than 1 bit/second bandwidth a show-stopper. That was
true for covert and side channels. Further, if you knew that a
mechanism had a channel, as this one does, and you didn't fix it,
you didn't get your certificate. If you know about a problem
during the design/implementation phase it's really inexcusable not
to fix it before "completing" the code.
> Talking about this in the context of (small) side channels: The
> notification types introduced in this patch are mostly things that a
> user would be able to observe anyway if they polled /proc/self/mounts,
> right?
It's supposed to be a general mechanism. Of course it would
be simpler if is was restricted to things you can get at via
/proc/self.
> It might make sense to align access controls based on that - if
> you don't want it to be possible to observe events happening on some
> mount points through this API, you should probably lock down
> /proc/*/mounts equivalently, by introducing an LSM hook for "is @cred
> allowed to see @mnt" or something like that - and if you want to
> compare two cred structures, you could record the cred structure that
> is responsible for the creation of the mount point, or something like
> that.
I'm not going to argue against that.
> For some of the other patches, I guess things get more tricky because
> the notification exposes new information that wasn't really available
> before.
We have to look not just at the information being available,
but the mechanism used. Being able to look at information about
a process in /proc doesn't mean I should be able to look at it
using ptrace(). Access control isn't done on data, it's done on
objects. That I can get information by looking in one object provides
no assurance that I can get it through a different object containing
the same information. This happens in /dev all over the place. A
file with hard links may be accessible by one path but not another.
>
>>> You can transmit information through the scheduler,
>>> through hyperthread resource sharing, through CPU data caches, through
>>> disk contention, through page cache state, through RAM contention, and
>>> probably dozens of other ways that I can't think of right now.
>> Yeah, and there's been a lot of activity to reduce those,
>> which are hard to exploit, as opposed to this, which would
>> be trivial and obvious.
>>
>>> There
>>> have been plenty of papers that demonstrated things like an SSH
>>> connection between two virtual machines without network access running
>>> on the same physical host (<https://gruss.cc/files/hello.pdf>),
>>> communication between a VM and a browser running on the host system,
>>> and so on.
>> So you're saying we shouldn't have mode bits on files because
>> spectre/meltdown makes them pointless?
> spectre/meltdown are vulnerabilities that are being mitigated.
> Microarchitectural covert channels are an accepted fact and I haven't
> heard of anyone seriously considering trying to get rid of them all.
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: David Howells @ 2019-05-29 21:02 UTC (permalink / raw)
To: Jann Horn
Cc: dhowells, Greg KH, Al Viro, raven, linux-fsdevel, Linux API,
linux-block, keyrings, linux-security-module, kernel list,
Kees Cook, Kernel Hardening
In-Reply-To: <CAG48ez0R-R3Xs+3Xg9T9qcV3Xv6r4pnx1Z2y=Ltx7RGOayte_w@mail.gmail.com>
Jann Horn <jannh@google.com> wrote:
> 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)?
Correct. There's a gap and if an interrupt or something occurs, it's
sufficiently big for the refcount trace to go weird.
I've seen it in afs/rxrpc where the incoming network packets that are part of
the rxrpc call flow disrupt the refcounts noted in trace lines.
David
^ permalink raw reply
* Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings
From: David Howells @ 2019-05-29 21:34 UTC (permalink / raw)
To: James Morris; +Cc: dhowells, keyrings, linux-security-module, linux-kernel
In-Reply-To: <alpine.LRH.2.21.1905290646010.31297@namei.org>
James Morris <jmorris@namei.org> wrote:
> > +
> > + if (flags & ~KEYCTL_MOVE_EXCL)
> > + return -EINVAL;
> > +
> > + key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE, KEY_NEED_LINK);
> > + if (IS_ERR(key_ref)) {
> > + ret = PTR_ERR(key_ref);
> > + goto error;
> > + }
>
> This could probably be a simple return, as there is no cleanup.
Changed.
David
^ permalink raw reply
* [GIT PULL] keys: Miscellany
From: David Howells @ 2019-05-29 22:01 UTC (permalink / raw)
To: James Morris; +Cc: dhowells, keyrings, linux-security-module, linux-kernel
Hi James,
Here are some miscellaneous keyrings fixes and improvements intended for
the next merge window, if you could pull them please.
(1) Fix a bunch of warnings from sparse, including missing RCU bits and
kdoc-function argument mismatches
(2) Implement a keyctl to allow a key to be moved from one keyring to
another, with the option of prohibiting key replacement in the
destination keyring.
(3) Grant Link permission to possessors of request_key_auth tokens so that
upcall servicing daemons can more easily arrange things such that only
the necessary auth key is passed to the actual service program, and
not all the auth keys a daemon might possesss.
Changes: I made the change you suggested on patch 6 and added the
reviewed-by's to patches 1-5 and 7.
David
---
The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:
Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-misc-20190529
for you to fetch changes up to 39de363a48ae151d766512c8e73a5564a3096c82:
keys: Grant Link permission to possessers of request_key auth keys (2019-05-29 22:34:52 +0100)
----------------------------------------------------------------
Miscellaneous keyrings development
----------------------------------------------------------------
David Howells (7):
keys: sparse: Fix key_fs[ug]id_changed()
keys: sparse: Fix incorrect RCU accesses
keys: sparse: Fix kdoc mismatches
keys: Break bits out of key_unlink()
keys: Make __key_link_begin() handle lockdep nesting
keys: Add a keyctl to move a key between keyrings
keys: Grant Link permission to possessers of request_key auth keys
Documentation/security/keys/core.rst | 21 +++++
include/linux/key.h | 13 ++-
include/uapi/linux/keyctl.h | 3 +
kernel/cred.c | 4 +-
security/keys/compat.c | 3 +
security/keys/internal.h | 3 +-
security/keys/key.c | 6 +-
security/keys/keyctl.c | 56 ++++++++++-
security/keys/keyring.c | 178 ++++++++++++++++++++++++++++++-----
security/keys/process_keys.c | 22 ++---
security/keys/request_key.c | 4 +-
security/keys/request_key_auth.c | 4 +-
12 files changed, 269 insertions(+), 48 deletions(-)
^ permalink raw reply
* Re: [GIT PULL] keys: Miscellany
From: Eric Biggers @ 2019-05-29 22:11 UTC (permalink / raw)
To: David Howells; +Cc: James Morris, keyrings, linux-security-module, linux-kernel
In-Reply-To: <18289.1559167295@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 11:01:35PM +0100, David Howells wrote:
>
> (2) Implement a keyctl to allow a key to be moved from one keyring to
> another, with the option of prohibiting key replacement in the
> destination keyring.
>
What is the use case, and where are the tests and documentation for this?
- Eric
^ permalink raw reply
* [PATCH] Allow to exclude specific file types in LoadPin
From: Ke Wu @ 2019-05-29 22:43 UTC (permalink / raw)
To: Kees Cook, Jonathan Corbet, James Morris, Serge E. Hallyn
Cc: linux-doc, linux-kernel, linux-security-module, Ke Wu
Linux kernel already provide MODULE_SIG and KEXEC_VERIFY_SIG to
make sure loaded kernel module and kernel image are trusted. This
patch adds a kernel command line option "loadpin.exclude" which
allows to exclude specific file types from LoadPin. This is useful
when people want to use different mechanisms to verify module and
kernel image while still use LoadPin to protect the integrity of
other files kernel loads.
Signed-off-by: Ke Wu <mikewu@google.com>
---
Documentation/admin-guide/LSM/LoadPin.rst | 10 ++++++
security/loadpin/loadpin.c | 37 +++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/Documentation/admin-guide/LSM/LoadPin.rst b/Documentation/admin-guide/LSM/LoadPin.rst
index 32070762d24c..716ad9b23c9a 100644
--- a/Documentation/admin-guide/LSM/LoadPin.rst
+++ b/Documentation/admin-guide/LSM/LoadPin.rst
@@ -19,3 +19,13 @@ block device backing the filesystem is not read-only, a sysctl is
created to toggle pinning: ``/proc/sys/kernel/loadpin/enabled``. (Having
a mutable filesystem means pinning is mutable too, but having the
sysctl allows for easy testing on systems with a mutable filesystem.)
+
+It's also possible to exclude specific file types from LoadPin using kernel
+command line option "``loadpin.exclude``". By default, all files are
+included, but they can be excluded using kernel command line option such
+as "``loadpin.exclude=kernel-module,kexec-image``". This allows to use
+different mechanisms such as ``CONFIG_MODULE_SIG`` and
+``CONFIG_KEXEC_VERIFY_SIG`` to verify kernel module and kernel image while
+still use LoadPin to protect the integrity of other files kernel loads. The
+full list of valid file types can be found in ``kernel_read_file_str``
+defined in ``include/linux/fs.h``.
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 055fb0a64169..8ee0c58fea40 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -45,6 +45,8 @@ static void report_load(const char *origin, struct file *file, char *operation)
}
static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
+static char *exclude_read_files[READING_MAX_ID];
+static int ignore_read_file_id[READING_MAX_ID];
static struct super_block *pinned_root;
static DEFINE_SPINLOCK(pinned_root_spinlock);
@@ -129,6 +131,12 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
struct super_block *load_root;
const char *origin = kernel_read_file_id_str(id);
+ /* If the file id is excluded, ignore the pinning. */
+ if ((unsigned int)id < READING_MAX_ID && ignore_read_file_id[id]) {
+ report_load(origin, file, "pinning-excluded");
+ return 0;
+ }
+
/* This handles the older init_module API that has a NULL file. */
if (!file) {
if (!enforce) {
@@ -187,10 +195,37 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
};
+static void parse_exclude(void)
+{
+ int i, j;
+ char *cur;
+
+ for (i = 0; i < ARRAY_SIZE(exclude_read_files); i++) {
+ cur = exclude_read_files[i];
+ if (!cur)
+ break;
+ if (*cur == '\0')
+ continue;
+
+ for (j = 0; j < ARRAY_SIZE(kernel_read_file_str); j++) {
+ if (strcmp(cur, kernel_read_file_str[j]) == 0) {
+ pr_info("excluding: %s\n",
+ kernel_read_file_str[j]);
+ ignore_read_file_id[j] = 1;
+ /*
+ * Can not break, because one read_file_str
+ * may map to more than on read_file_id.
+ */
+ }
+ }
+ }
+}
+
static int __init loadpin_init(void)
{
pr_info("ready to pin (currently %senforcing)\n",
enforce ? "" : "not ");
+ parse_exclude();
security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
return 0;
}
@@ -203,3 +238,5 @@ DEFINE_LSM(loadpin) = {
/* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
module_param(enforce, int, 0);
MODULE_PARM_DESC(enforce, "Enforce module/firmware pinning");
+module_param_array_named(exclude, exclude_read_files, charp, NULL, 0);
+MODULE_PARM_DESC(exclude, "Exclude pinning specific read file types");
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* Re: [PATCH] Allow to exclude specific file types in LoadPin
From: Kees Cook @ 2019-05-29 23:07 UTC (permalink / raw)
To: Ke Wu
Cc: Jonathan Corbet, James Morris, Serge E. Hallyn, linux-doc,
linux-kernel, linux-security-module
In-Reply-To: <20190529224350.6460-1-mikewu@google.com>
On Wed, May 29, 2019 at 03:43:50PM -0700, Ke Wu wrote:
> Linux kernel already provide MODULE_SIG and KEXEC_VERIFY_SIG to
> make sure loaded kernel module and kernel image are trusted. This
> patch adds a kernel command line option "loadpin.exclude" which
> allows to exclude specific file types from LoadPin. This is useful
> when people want to use different mechanisms to verify module and
> kernel image while still use LoadPin to protect the integrity of
> other files kernel loads.
Cool; I like this. A few thoughts below...
>
> Signed-off-by: Ke Wu <mikewu@google.com>
> ---
> Documentation/admin-guide/LSM/LoadPin.rst | 10 ++++++
> security/loadpin/loadpin.c | 37 +++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/Documentation/admin-guide/LSM/LoadPin.rst b/Documentation/admin-guide/LSM/LoadPin.rst
> index 32070762d24c..716ad9b23c9a 100644
> --- a/Documentation/admin-guide/LSM/LoadPin.rst
> +++ b/Documentation/admin-guide/LSM/LoadPin.rst
> @@ -19,3 +19,13 @@ block device backing the filesystem is not read-only, a sysctl is
> created to toggle pinning: ``/proc/sys/kernel/loadpin/enabled``. (Having
> a mutable filesystem means pinning is mutable too, but having the
> sysctl allows for easy testing on systems with a mutable filesystem.)
> +
> +It's also possible to exclude specific file types from LoadPin using kernel
> +command line option "``loadpin.exclude``". By default, all files are
> +included, but they can be excluded using kernel command line option such
> +as "``loadpin.exclude=kernel-module,kexec-image``". This allows to use
> +different mechanisms such as ``CONFIG_MODULE_SIG`` and
> +``CONFIG_KEXEC_VERIFY_SIG`` to verify kernel module and kernel image while
> +still use LoadPin to protect the integrity of other files kernel loads. The
> +full list of valid file types can be found in ``kernel_read_file_str``
> +defined in ``include/linux/fs.h``.
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index 055fb0a64169..8ee0c58fea40 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -45,6 +45,8 @@ static void report_load(const char *origin, struct file *file, char *operation)
> }
>
> static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
> +static char *exclude_read_files[READING_MAX_ID];
> +static int ignore_read_file_id[READING_MAX_ID];
Since this is set up at init, let's mark ignore_read_file_id with
__ro_after_init.
> static struct super_block *pinned_root;
> static DEFINE_SPINLOCK(pinned_root_spinlock);
>
> @@ -129,6 +131,12 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
> struct super_block *load_root;
> const char *origin = kernel_read_file_id_str(id);
>
> + /* If the file id is excluded, ignore the pinning. */
> + if ((unsigned int)id < READING_MAX_ID && ignore_read_file_id[id]) {
Can you use ARRAY_SIZE(ignore_read_file_id) here instead of
READING_MAX_ID?
> + report_load(origin, file, "pinning-excluded");
> + return 0;
> + }
> +
> /* This handles the older init_module API that has a NULL file. */
> if (!file) {
> if (!enforce) {
> @@ -187,10 +195,37 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
> };
>
> +static void parse_exclude(void)
Please mark this __init (since it's called from another __init
function).
> +{
> + int i, j;
> + char *cur;
> +
> + for (i = 0; i < ARRAY_SIZE(exclude_read_files); i++) {
> + cur = exclude_read_files[i];
> + if (!cur)
> + break;
> + if (*cur == '\0')
> + continue;
> +
> + for (j = 0; j < ARRAY_SIZE(kernel_read_file_str); j++) {
> + if (strcmp(cur, kernel_read_file_str[j]) == 0) {
> + pr_info("excluding: %s\n",
> + kernel_read_file_str[j]);
> + ignore_read_file_id[j] = 1;
> + /*
> + * Can not break, because one read_file_str
> + * may map to more than on read_file_id.
> + */
> + }
> + }
> + }
> +}
> +
> static int __init loadpin_init(void)
> {
> pr_info("ready to pin (currently %senforcing)\n",
> enforce ? "" : "not ");
> + parse_exclude();
> security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
> return 0;
> }
> @@ -203,3 +238,5 @@ DEFINE_LSM(loadpin) = {
> /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
> module_param(enforce, int, 0);
> MODULE_PARM_DESC(enforce, "Enforce module/firmware pinning");
> +module_param_array_named(exclude, exclude_read_files, charp, NULL, 0);
> +MODULE_PARM_DESC(exclude, "Exclude pinning specific read file types");
> --
> 2.22.0.rc1.257.g3120a18244-goog
>
Everything else looks good; thanks!
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: Greg KH @ 2019-05-29 23:09 UTC (permalink / raw)
To: David Howells
Cc: viro, raven, linux-fsdevel, linux-api, linux-block, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <31936.1559146000@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 05:06:40PM +0100, David Howells wrote:
> 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.
The function pointer only gets called for the last "put", and then kfree
will be called so you should not have to worry about speed/stack frames
at that point in time.
> > > 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.
I bring up using a kref any time I see a usage that could use it as it
makes it easier for people to understand and "know" you are doing your
reference counting for your object "correctly". It's an abstraction
that is used to make it easier for us developers to understand.
Otherwise you have to hand-roll the same logic here. Yes, refcounts
have made it easier to do it in your own (which was their goal), but you
still don't have to do it "on your own".
Anyway, I'll not push the issue here, if you want to stick to a
refcount_t, that's enough for now. We can worry about changing this
later after you have debugged all the corner conditions :)
> > 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()?
The destructor only takes one object pointer as you are finally freeing
that object. What more do you need/want to "know" at that point in
time?
What would kref_dec_and_test() be needed for?
> Why doesn't refcount_t get merged into kref, or vice versa? Having both would
> seem redundant.
kref uses refcount_t and provides a different functionality on top of
it. Not all uses of a refcount in the kernel is for object lifecycle
reference counting, as you know :)
> 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.
That's not good, we should address that independently as you are loosing
functionality/protection when doing that.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: Greg KH @ 2019-05-29 23:11 UTC (permalink / raw)
To: David Howells
Cc: viro, raven, linux-fsdevel, linux-api, linux-block, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <31936.1559146000@warthog.procyon.org.uk>
On Wed, May 29, 2019 at 05:06:40PM +0100, David Howells wrote:
> Greg KH <gregkh@linuxfoundation.org> wrote:
> > 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
So, if that's all that needs to be fixed, can you use the same
buffer/code if that patch is merged?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Andy Lutomirski @ 2019-05-29 23:12 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: <058f227c-71ab-a6f4-00bf-b8782b3b2956@schaufler-ca.com>
> On May 29, 2019, at 10:46 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> 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.
Under this scenario, Fred should not be allowed to enable these watches. If you give yourself and Fred unconstrained access to the same FS, then can communicate.
>
> 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.
I disagree. They’ll model the watch as something to prevent if they want to restrict communication.
>
>> (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.
I think it’s a bit crazy to have the same event queue with two readers who read different things.
^ permalink raw reply
* [PATCH 00/22] Some documentation fixes
From: Mauro Carvalho Chehab @ 2019-05-29 23:23 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, xen-devel, linux-kselftest, linux-amlogic,
linux-gpio, linux-pm, devel, keyrings, dri-devel, linux-integrity,
linux-mtd, patches, devicetree, netdev, alsa-devel, devel,
virtualization, linux-mm, linux-pci, linux-acpi, linuxppc-dev,
linux-security-module, linux-i2c, kvm, bpf, x86
Fix several warnings and broken links.
This series was generated against linux-next, but was rebased to be applied at
docs-next. It should apply cleanly on either tree.
There's a git tree with all of them applied on the top of docs/docs-next
at:
https://git.linuxtv.org/mchehab/experimental.git/log/?h=fix_doc_links_v2
Mauro Carvalho Chehab (21):
ABI: sysfs-devices-system-cpu: point to the right docs
isdn: mISDN: remove a bogus reference to a non-existing doc
dt: fix broken references to nand.txt
docs: zh_CN: get rid of basic_profiling.txt
doc: it_IT: fix reference to magic-number.rst
docs: mm: numaperf.rst: get rid of a build warning
docs: bpf: get rid of two warnings
docs: mark orphan documents as such
docs: amd-memory-encryption.rst get rid of warnings
gpu: amdgpu: fix broken amdgpu_dma_buf.c references
gpu: i915.rst: Fix references to renamed files
docs: zh_CN: avoid duplicate citation references
docs: vm: hmm.rst: fix some warnings
docs: it: license-rules.rst: get rid of warnings
docs: gpio: driver.rst: fix a bad tag
docs: soundwire: locking: fix tags for a code-block
docs: security: trusted-encrypted.rst: fix code-block tag
docs: security: core.rst: Fix several warnings
docs: net: dpio-driver.rst: fix two codeblock warnings
docs: net: sja1105.rst: fix table format
docs: fix broken documentation links
Otto Sabart (1):
mfd: madera: Fix bad reference to pinctrl.txt file
.../ABI/testing/sysfs-devices-system-cpu | 3 +-
Documentation/accelerators/ocxl.rst | 2 +
Documentation/acpi/dsd/leds.txt | 2 +-
.../admin-guide/kernel-parameters.rst | 6 +-
.../admin-guide/kernel-parameters.txt | 16 ++---
Documentation/admin-guide/mm/numaperf.rst | 5 +-
Documentation/admin-guide/ras.rst | 2 +-
Documentation/arm/stm32/overview.rst | 2 +
.../arm/stm32/stm32f429-overview.rst | 2 +
.../arm/stm32/stm32f746-overview.rst | 2 +
.../arm/stm32/stm32f769-overview.rst | 2 +
.../arm/stm32/stm32h743-overview.rst | 2 +
.../arm/stm32/stm32mp157-overview.rst | 2 +
Documentation/bpf/btf.rst | 2 +
.../bindings/mtd/amlogic,meson-nand.txt | 2 +-
.../devicetree/bindings/mtd/gpmc-nand.txt | 2 +-
.../devicetree/bindings/mtd/marvell-nand.txt | 2 +-
.../devicetree/bindings/mtd/tango-nand.txt | 2 +-
.../devicetree/bindings/net/fsl-enetc.txt | 7 +-
.../bindings/pci/amlogic,meson-pcie.txt | 2 +-
.../regulator/qcom,rpmh-regulator.txt | 2 +-
.../devicetree/booting-without-of.txt | 2 +-
Documentation/driver-api/gpio/board.rst | 2 +-
Documentation/driver-api/gpio/consumer.rst | 2 +-
Documentation/driver-api/gpio/driver.rst | 2 +-
.../driver-api/soundwire/locking.rst | 4 +-
.../firmware-guide/acpi/enumeration.rst | 2 +-
.../firmware-guide/acpi/method-tracing.rst | 2 +-
Documentation/gpu/amdgpu.rst | 4 +-
Documentation/gpu/i915.rst | 6 +-
Documentation/gpu/msm-crash-dump.rst | 2 +
Documentation/i2c/instantiating-devices | 2 +-
Documentation/interconnect/interconnect.rst | 2 +
Documentation/laptops/lg-laptop.rst | 2 +
.../freescale/dpaa2/dpio-driver.rst | 4 +-
Documentation/networking/dsa/sja1105.rst | 6 +-
Documentation/powerpc/isa-versions.rst | 2 +
Documentation/security/keys/core.rst | 16 +++--
.../security/keys/trusted-encrypted.rst | 4 +-
Documentation/sysctl/kernel.txt | 4 +-
.../translations/it_IT/process/howto.rst | 2 +-
.../it_IT/process/license-rules.rst | 28 ++++----
.../it_IT/process/magic-number.rst | 2 +-
.../it_IT/process/stable-kernel-rules.rst | 4 +-
.../translations/zh_CN/basic_profiling.txt | 71 -------------------
.../translations/zh_CN/process/4.Coding.rst | 2 +-
.../zh_CN/process/management-style.rst | 4 +-
.../zh_CN/process/programming-language.rst | 28 ++++----
.../virtual/kvm/amd-memory-encryption.rst | 5 ++
Documentation/virtual/kvm/vcpu-requests.rst | 2 +
Documentation/vm/hmm.rst | 9 ++-
Documentation/x86/x86_64/5level-paging.rst | 2 +-
Documentation/x86/x86_64/boot-options.rst | 4 +-
.../x86/x86_64/fake-numa-for-cpusets.rst | 2 +-
MAINTAINERS | 6 +-
arch/arm/Kconfig | 2 +-
arch/arm64/kernel/kexec_image.c | 2 +-
arch/powerpc/Kconfig | 2 +-
arch/x86/Kconfig | 16 ++---
arch/x86/Kconfig.debug | 2 +-
arch/x86/boot/header.S | 2 +-
arch/x86/entry/entry_64.S | 2 +-
arch/x86/include/asm/bootparam_utils.h | 2 +-
arch/x86/include/asm/page_64_types.h | 2 +-
arch/x86/include/asm/pgtable_64_types.h | 2 +-
arch/x86/kernel/cpu/microcode/amd.c | 2 +-
arch/x86/kernel/kexec-bzimage64.c | 2 +-
arch/x86/kernel/pci-dma.c | 2 +-
arch/x86/mm/tlb.c | 2 +-
arch/x86/platform/pvh/enlighten.c | 2 +-
drivers/acpi/Kconfig | 10 +--
drivers/isdn/mISDN/dsp_core.c | 2 -
drivers/net/ethernet/faraday/ftgmac100.c | 2 +-
.../fieldbus/Documentation/fieldbus_dev.txt | 4 +-
drivers/vhost/vhost.c | 2 +-
include/acpi/acpi_drivers.h | 2 +-
include/linux/fs_context.h | 2 +-
include/linux/lsm_hooks.h | 2 +-
include/linux/mfd/madera/pdata.h | 3 +-
mm/Kconfig | 2 +-
security/Kconfig | 2 +-
tools/include/linux/err.h | 2 +-
.../Documentation/stack-validation.txt | 4 +-
tools/testing/selftests/x86/protection_keys.c | 2 +-
84 files changed, 183 insertions(+), 212 deletions(-)
delete mode 100644 Documentation/translations/zh_CN/basic_profiling.txt
--
2.21.0
^ permalink raw reply
* Re: [PATCH 6/7] keys: Add a keyctl to move a key between keyrings
From: Eric Biggers @ 2019-05-29 23:25 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <155856412507.10428.15987388402707639951.stgit@warthog.procyon.org.uk>
On Wed, May 22, 2019 at 11:28:45PM +0100, David Howells wrote:
> Add a keyctl to atomically move a link to a key from one keyring to
> another. The key must exist in "from" keyring and a flag can be given to
> cause the operation to fail if there's a matching key already in the "to"
> keyring.
>
> This can be done with:
>
> keyctl(KEYCTL_MOVE,
> key_serial_t key,
> key_serial_t from_keyring,
> key_serial_t to_keyring,
> unsigned int flags);
>
> The key being moved must grant Link permission and both keyrings must grant
> Write permission.
>
> flags should be 0 or KEYCTL_MOVE_EXCL, with the latter preventing
> displacement of a matching key from the "to" keyring.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
This shows up after a few seconds of syzkaller fuzzing with a description of
KEYCTL_MOVE added:
WARNING: possible circular locking dependency detected
5.2.0-rc1 #5 Not tainted
------------------------------------------------------
syz-executor.28/27700 is trying to acquire lock:
00000000049888d8 (keyring_serialise_link_sem){+.+.}, at: __key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231
but task is already holding lock:
00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&type->lock_class/1){+.+.}:
lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
down_write_nested+0x3c/0xa0 kernel/locking/rwsem.c:177
__key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398
key_move+0x3ad/0x470 security/keys/keyring.c:1538
keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
__do_sys_keyctl security/keys/keyctl.c:1823 [inline]
__se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
__x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #0 (keyring_serialise_link_sem){+.+.}:
check_prevs_add kernel/locking/lockdep.c:2417 [inline]
validate_chain kernel/locking/lockdep.c:2799 [inline]
__lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792
lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
down_write+0x38/0xa0 kernel/locking/rwsem.c:66
__key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231
key_move+0xf0/0x470 security/keys/keyring.c:1529
keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
__do_sys_keyctl security/keys/keyctl.c:1823 [inline]
__se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
__x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&type->lock_class/1);
lock(keyring_serialise_link_sem);
lock(&type->lock_class/1);
lock(keyring_serialise_link_sem);
*** DEADLOCK ***
2 locks held by syz-executor.28/27700:
#0: 000000002a03f208 (&type->lock_class){++++}, at: __key_unlink_begin+0x6c/0x110 security/keys/keyring.c:1398
#1: 00000000b171310c (&type->lock_class/1){+.+.}, at: __key_link_begin+0xa4/0x2d0 security/keys/keyring.c:1222
stack backtrace:
CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xb1/0x118 lib/dump_stack.c:113
print_circular_bug+0x4a4/0x4b5 kernel/locking/lockdep.c:1564
check_prev_add+0xd1f/0x1af7 kernel/locking/lockdep.c:2309
check_prevs_add kernel/locking/lockdep.c:2417 [inline]
validate_chain kernel/locking/lockdep.c:2799 [inline]
__lock_acquire+0x38a4/0x3c30 kernel/locking/lockdep.c:3792
lock_acquire+0x106/0x330 kernel/locking/lockdep.c:4302
down_write+0x38/0xa0 kernel/locking/rwsem.c:66
__key_link_begin+0x1c2/0x2d0 security/keys/keyring.c:1231
key_move+0xf0/0x470 security/keys/keyring.c:1529
keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
__do_sys_keyctl security/keys/keyctl.c:1823 [inline]
__se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
__x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458a09
Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa
RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09
RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e
RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000
R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4
R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003
FAULT_INJECTION: forcing a failure.
name failslab, interval 1, probability 0, space 0, times 0
CPU: 8 PID: 27700 Comm: syz-executor.28 Not tainted 5.2.0-rc1 #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xb1/0x118 lib/dump_stack.c:113
fail_dump lib/fault-inject.c:51 [inline]
should_fail+0x61e/0x720 lib/fault-inject.c:143
__should_failslab+0xec/0x120 mm/failslab.c:32
should_failslab+0x9/0x14 mm/slab_common.c:1610
slab_pre_alloc_hook mm/slab.h:420 [inline]
slab_alloc mm/slab.c:3312 [inline]
kmem_cache_alloc_trace+0x146/0x2a0 mm/slab.c:3553
kmalloc include/linux/slab.h:547 [inline]
kzalloc include/linux/slab.h:742 [inline]
assoc_array_insert+0xcc/0x440 lib/assoc_array.c:985
__key_link_begin+0x120/0x2d0 security/keys/keyring.c:1236
key_move+0xf0/0x470 security/keys/keyring.c:1529
keyctl_keyring_move+0xb6/0x120 security/keys/keyctl.c:610
__do_sys_keyctl security/keys/keyctl.c:1823 [inline]
__se_sys_keyctl+0x8e/0x2c0 security/keys/keyctl.c:1685
__x64_sys_keyctl+0xbe/0x150 security/keys/keyctl.c:1685
do_syscall_64+0x9e/0x4b0 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x458a09
Code: dd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f9d53755c88 EFLAGS: 00000246 ORIG_RAX: 00000000000000fa
RAX: ffffffffffffffda RBX: 000000000071bf00 RCX: 0000000000458a09
RDX: 000000001f62d16e RSI: 000000002490e642 RDI: 000000000000001e
RBP: 00007f9d53755ca0 R08: 0000000000000000 R09: 0000000000000000
R10: 000000001afbc80a R11: 0000000000000246 R12: 00007f9d537566d4
R13: 00000000004ac12c R14: 00000000006ebd68 R15: 0000000000000003
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: Casey Schaufler @ 2019-05-29 23:56 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: <2FF92095-E5B1-4811-A7F8-B7D4C32F86DD@amacapital.net>
On 5/29/2019 4:12 PM, Andy Lutomirski wrote:
>
>> On May 29, 2019, at 10:46 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>> 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.
> Under this scenario, Fred should not be allowed to enable these watches. If you give yourself and Fred unconstrained access to the same FS, then can communicate.
How are you going to determine at the time Fred tries to enable the watches
that I am going to do something that will trigger them? I'm not saying it isn't
possible, I'm curious how you would propose doing it. If you deny Fred the ability
to set watches because it is possible for me to trigger them, he can't use them
to get information from Wilma, either.
>
>> 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.
> I disagree. They’ll model the watch as something to prevent if they want to restrict communication.
Sorry, but that isn't sufficiently detailed to be meaningful.
>>> (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.
> I think it’s a bit crazy to have the same event queue with two readers who read different things.
Look at killpg(3).
The process that creates the event has to be involved in the
access decision. Otherwise you have an uncontrolled data channel.
When the receiver reads the event queue it knows nothing about the
sender, and hence cannot make the decision unless the credential of
the sender is kept with the event message, and used when the
receiver tries to access it. I don't think that wold work well with
the mechanism as designed.
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-30 5:38 UTC (permalink / raw)
To: Christopherson, Sean J, Andy Lutomirski
Cc: Stephen Smalley, 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: <20190528214107.GD13158@linux.intel.com>
> From: Christopherson, Sean J
> Sent: Tuesday, May 28, 2019 2:41 PM
>
> On Tue, May 28, 2019 at 01:48:02PM -0700, Andy Lutomirski wrote:
> > On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > > 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.
> >
> > You lost me here. What operation triggers this callback? And
> > wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO
> > or just some fresh executable zero bytes?
>
> An explicit ioctl() after EACCEPTCOPY to update the allowed permissions.
> For all intents and purposes, the EAUG'd page must start RW. Maybe a better way to phrase
> it is that at some point the page must be writable to have any value whatsover.
> EACCEPTCOPY explicitly requires the page to be at least RW. EACCEPT technically doesn't
> require RW, but a RO or RX zero page is useless. Userspace could still EACCEPT with RO or
> RX, but SGX would assume a minimum of RW for the purposes of the LSM check.
Why is an explicit ioctl() necessary after EACCEPTCOPY? Or why is mprotect() not sufficient?
I tend to agree on Andy's MAXPERM model where MAXPERM never changes once established.
>
> > > 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);
> >
> > In the maxperm model, this mprotect() will fail unless MAXPERM
> > contains RX, which could only happen if MAXPERM=RWX. So, regardless
> > of how it's actually mapped to SELinux policy, MAXPERM=RWX is
> > functionally like EXECMOD and actual RWX PTEs are functionally like
> > EXECMEM.
>
> Yep, same idea, except in the proposed flow ACTIVATE_REGION.
>
> > > ...
> > >
> > > 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)
> >
> > Sure it can! You just have a hypervisor that maps a PA bit to EPT
> > no-read. Then you can use that PA bit to suppress read. Also, Linux
> > already abuses PKRU to simulate XO, although that won't work for
> > enclaves.
>
> Heh, I intentionally said "x86 paging" to rule out EPT :-) I'm pretty sure it's a moot
> point though, I have a hard time believing an LSM will allow RW->X and not RW->RX.
>
> > > 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)
I'm not sure on what security_enclave_load()'s decision would be based.
> > >
> > > - <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.
> > >
> > > 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.
> > >
> >
> > I may be misunderstanding or just be biased to my own proposal, but
> > this seems potentially more complicated and less flexible than the
> > MAXPERM model. One of the main things that made me come up with
> > MAXPERM is that I wanted to avoid any complicated PTE/VMA modification
> > or runtime changes. So, with MAXPERM, we still need to track the
> > MAXPERM bits per page, but we don't ever need to *change* them or to
> > worry about what is or is not mapped anywhere at any given time. With
> > ACTIVATE_REGION, don't we need to make sure that we don't have a
> > second VMA pointing at the same pages? Or am I just confused?
>
> In theory, it's still your MAXPERM model, but with the unnecessary states removed and the
> others enforced/handled by the natural SGX transitions instead of explictly in ioctls.
> Underneath the hood the SGX driver would still need to track the MAXPERM.
What are the "unnecessary states" removed?
I'm not sure understand the proposal fully. The whole thing looks to me like the driver is undertaking things that should/would otherwise be done by mmap()/mprotect() syscalls. It also imposes unnecessary restrictions on user mode code, such as mmap(PROT_NONE), ACTIVATE_REGION can be called only once, etc. What'd happen if ACTIVATE_REGION is called with a range spanning multiple/partial VMAs? What'd happen if an enclave was unmapped than mapped again? I'd say the proposal is unintuitive at least.
In theory, if the driver can keep track of MAXPERM for all pages within an enclave, then it could fail mmap() if the requested prot conflicts with any page's MAXPERM within that range. Otherwise, MAXPERM could be copied into VM_MAY* flags then mprotect() will just follow through. Wouldn't that be a much simpler and more intuitive approach?
>
> With SGX1, SECINFO == MAXPERM. With SGX2, ACTIVATE_REGION == MAXPERM, with the
> implication that the previous state is always RW.
>
> > > - 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.
> >
> > I may still be missing something, but, for SGX1, it's possible at
> > least in principle for the enclave to request, via ocall or similar,
> > that the untrusted runtime do mprotect(). It's not even such a bad
> > idea. Honestly, enclaves *shouldn't* have anything actually writable
> > and executable at once because the enclaves don't want to be easily
> > exploited.
>
> Yes, but the *EPCM* permissions are immutable. So if an enclave wants to do RW->RX it has
> to intialize its pages to RWX. And because the untrusted runtime is, ahem, untrusted, the
> enclave cannot rely on userspace to never map its pages RWX. In other words, from a
> enclave security perspective, an SGX1 enclave+runtime that uses RW->RX is no different
> than an enclave that uses RWX. Using your earlier terminology, an SGX1 enclave *should*
> get a dirty looks if maps a page RWX in the EPCM, even if it only intends RW->RX behavior.
>
> > > - 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: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-30 6:12 UTC (permalink / raw)
To: Stephen Smalley, Christopherson, Sean J, 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: <285f279f-b500-27f0-ab42-fb1dbcc5ab18@tycho.nsa.gov>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-owner@vger.kernel.org] On Behalf
> Of Stephen Smalley
> Sent: Wednesday, May 29, 2019 7:08 AM
>
> 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.
I'm confused.
In the case of EADD (non-NULL vma), are we passing both vma and sigstruct file? If so, which file dictates allowed permissions, vma->vm_file or sigstruct, or both???
In the case of EAUG (NULL vma), all other parameters are constant for any given enclave. Then why do we call this same hook for every region added, assuming the hook will return the same value everytime anyway?
And it looks like ACTIVATE_REGION is needed only because the proposed security_enclave_load() would base its decision on the sigstruct file. An alternative is to base that decision on /dev/sgx/enclave. Of course the former has finer granularity but is that really necessary? From security perspective, only the weakest link matters. FILE__EXECMOD on a regular shared object could allow exploits of all bugs throughout the host process because code within that shared object is modifiable by not only itself but also any code within that same process. In contrast, FILE__EXECMOD on /dev/sgx/enclave only allows enclaves to modify themselves. They cannot modify each other, neither can "untrusted" code outside of enclaves modify any of them. So it doesn't look like a weaker link to me. Moreover, requiring FILE__EXECMOD on sigstruct means it could be used as a target buffer for code injection attacks. IMHO that *lowers* the security of the whole process.
>
> 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()?
I think security_enclave_init() will always be useful, as it offers a way for LSM to implement whitelisting/blacklisting. Of course an LSM module like SELinux can look into the backing inode too. I think the hook should have a signature like:
int security_enclave_init(struct sgx_sigstruct __user *sigstruct);
An LSM that cares about the backing file could look into vm_file of the VMA covering the buffer, while an LSM that cares the sigstruct itself (e.g. signing key) could just look into the buffer.
>
> 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.
I'm not sure if these ENCLAVE__* flags are an overkill, unless we want to enforce an enclave file cannot be loaded as a regular shared object or vice versa.
>
> >
> > 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: [GIT PULL] keys: Miscellany
From: James Morris @ 2019-05-30 6:53 UTC (permalink / raw)
To: David Howells; +Cc: keyrings, linux-security-module, linux-kernel
In-Reply-To: <18289.1559167295@warthog.procyon.org.uk>
On Wed, 29 May 2019, David Howells wrote:
> Hi James,
>
> Here are some miscellaneous keyrings fixes and improvements intended for
> the next merge window, if you could pull them please.
>
Linus has asked for security subsystem PRs to go directly to him.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer
From: Andrea Parri @ 2019-05-30 9:50 UTC (permalink / raw)
To: Greg KH
Cc: David Howells, viro, raven, linux-fsdevel, linux-api, linux-block,
keyrings, linux-security-module, linux-kernel, Peter Zijlstra,
Will Deacon, Paul E. McKenney, Mark Rutland
In-Reply-To: <20190529231112.GB3164@kroah.com>
> > 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
>
> So, if that's all that needs to be fixed, can you use the same
> buffer/code if that patch is merged?
That's about one year old...: let me add the usual suspects in Cc: ;-)
since I'm not sure what the plan was (or if I'm missing something) ...
Speaking of ring buffer implementations (and relatively "old" patches),
here's another quite interesting:
https://lkml.kernel.org/r/20181211034032.32338-1-yuleixzhang@tencent.com
Thanks,
Andrea
^ permalink raw reply
* Re: sleep in selinux_audit_rule_init
From: Janne Karhunen @ 2019-05-30 10:39 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module
In-Reply-To: <1432f617-424e-044c-4f78-47f1100262ae@tycho.nsa.gov>
On Wed, May 22, 2019 at 6:27 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > Ok. The question is then how should IMA handle missing domains/types.
> > Just dropping IMA policy rules doesn't sound safe, nor does skipping
> > rules in case the domains/types are restored.
>
> You can just do what audit_dupe_lsm_field() does. It effectively
> disables the rule upon the invalidation (which makes sense, since it can
> no longer match anything since nothing can have that domain/type) but
> retains the string value so it can later re-activate the rule if the
> domain/type becomes valid again later.
Finally got a moment to look into this. It looks to me there is
already a notifier? Could something like this work?
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..2203451862d4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
int ima_init_template(void);
void ima_init_template_list(void);
int __init ima_init_digests(void);
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data);
/*
* used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 5749ec92516f..449502f5c3dc 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -52,6 +52,10 @@ int ima_hash_algo = HASH_ALGO_SHA1;
static int hash_setup_done;
static struct workqueue_struct *ima_update_wq;
+static struct notifier_block ima_lsm_policy_notifier = {
+ .notifier_call = ima_lsm_policy_change,
+};
+
static int __init hash_setup(char *str)
{
struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -691,6 +695,10 @@ static int __init init_ima(void)
error = ima_init();
}
+ error = register_lsm_notifier(&ima_lsm_policy_notifier);
+ if (error)
+ pr_warn("Couldn't register LSM notifier, error %d\n", error);
+
if (!error)
ima_update_policy_flag();
else
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index e0cc323f948f..c3983d24279a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -252,8 +252,8 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
/*
* The LSM policy can be reloaded, leaving the IMA LSM based rules referring
* to the old, stale LSM policy. Update the IMA LSM based rules to reflect
- * the reloaded LSM policy. We assume the rules still exist; and BUG_ON() if
- * they don't.
+ * the reloaded LSM policy. Keep currently invalid fields around in case
+ * they become valid after a policy reload.
*/
static void ima_lsm_update_rules(void)
{
@@ -269,11 +269,23 @@ static void ima_lsm_update_rules(void)
Audit_equal,
entry->lsm[i].args_p,
&entry->lsm[i].rule);
- BUG_ON(!entry->lsm[i].rule);
+ if (result == -EINVAL)
+ pr_warn("ima: rule for LSM \'%d\' is invalid\n",
+ entry->lsm[i].type);
}
}
}
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+ void *lsm_data)
+{
+ if (event != LSM_POLICY_CHANGE)
+ return NOTIFY_DONE;
+
+ ima_lsm_update_rules();
+ return NOTIFY_DONE;
+}
+
/**
* ima_match_rules - determine whether an inode matches the measure rule.
* @rule: a pointer to a rule
@@ -327,11 +339,10 @@ static bool ima_match_rules(struct
ima_rule_entry *rule, struct inode *inode,
for (i = 0; i < MAX_LSM_RULES; i++) {
int rc = 0;
u32 osid;
- int retried = 0;
if (!rule->lsm[i].rule)
continue;
-retry:
+
switch (i) {
case LSM_OBJ_USER:
case LSM_OBJ_ROLE:
@@ -352,11 +363,6 @@ static bool ima_match_rules(struct ima_rule_entry
*rule, struct inode *inode,
default:
break;
}
- if ((rc < 0) && (!retried)) {
- retried = 1;
- ima_lsm_update_rules();
- goto retry;
- }
if (!rc)
return false;
}
--
Janne
^ permalink raw reply related
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