Re: [PATCH RFC v1 18/26] kmsan: mm: call KMSAN hooks from SLUB code

[Qian Cai <cai@lca.pw>]

On Fri, 2019-10-18 at 17:30 +0200, Alexander Potapenko wrote:
> On Fri, Oct 18, 2019 at 5:13 PM Qian Cai <cai@lca.pw> wrote:
> > 
> > On Fri, 2019-10-18 at 16:54 +0200, Alexander Potapenko wrote:
> > > On Fri, Oct 18, 2019 at 4:42 PM Qian Cai <cai@lca.pw> wrote:
> > > > 
> > > > On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote:
> > > > > On Fri, Oct 18, 2019 at 3:42 PM Qian Cai <cai@lca.pw> wrote:
> > > > > > 
> > > > > > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> > > > > > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > > > > > > > 
> > > > > > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > > > > > > > In order to report uninitialized memory coming from heap allocations
> > > > > > > > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > > > > > > > 
> > > > > > > > > It's handy that we need KMSAN hooks in the places where
> > > > > > > > > init_on_alloc/init_on_free initialization is performed.
> > > > > > > > 
> > > > > > > > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > > > > > > > value of this patch?
> > > > > > > 
> > > > > > > Sorry, are you talking about the whole patchset or just this patch?
> > > > > > 
> > > > > > Just this patch.
> > > > > > 
> > > > > > > Note that SLUB debug is unable to detect uninitialized values with
> > > > > > > bit-to-bit precision, neither have I heard of anyone using it for
> > > > > > > detecting uses of uninitialized memory in the kernel at all.
> > > > > > > The purpose of SLUB debug is to detect corruptions of freed memory.
> > > > > > 
> > > > > > The point is if developers have SLUB debug enabled, all the free objects will be
> > > > > > poisoned, so what's the point of checking uninitialized memory there?
> > > > > 
> > > > > You are right, SLUB debug has to be handled separately. If I'm
> > > > > understanding correctly, right now KMSAN poisons freed memory before
> > > > > SLUB debug wipes it, therefore the memory will count as initialized.
> > > > > On the other hand, newly allocated memory is still marked as
> > > > > uninitialized, so a lot of bugs still remain detectable.
> > > > 
> > > > Yes, but newly allocated slub objects will be poisoned as well.
> > > 
> > > As far as I can tell, KMSAN hook marking newly allocated objects as
> > > uninitialized is called after slub poisoning.
> > > Therefore these allocations will be treated by KMSAN as uninitialized.
> > > > > TBH, I haven't tested KMSAN with SLUB debug good enough. Note that
> > > > > it's anyway a separate build that requires Clang, so right now it
> > > > > doesn't make much sense to combine KMSAN and SLUB debug together.
> > > > 
> > > > Can't you just build a debug kernel with SLUB debug enabled but dropping this
> > > > patch? If there is an uninitialized memory here leading to data corruption, SLUB
> > > > debug should be detected as well as this patch. If not, it needs to understand
> > > > why.
> > > 
> > > Sorry, there might be some misunderstanding here.
> > > KMSAN keeps the state of heap objects separately by keeping exactly
> > > the same amount of initialized/uninitialized bits for every
> > > allocation.
> > > A call to kmsan_slab_alloc()/kmsan_slab_free() will mark an allocation
> > > as uninitialized for KMSAN. Not doing so will result in false reports.
> > > A call to memset(object, POISON_FREE, object_size) performed by SLAB
> > > debug will actually mark this allocation as initialized from KMSAN
> > > point of view, because we're memsetting a range with initialized data.
> > > Note that use of uninitialized data doesn't necessarily lead to
> > > immediate data corruption, so there might be nothing to detect for
> > > SLUB debug.
> > 
> > Well, SLUB debug would mark any access of uninitialized memory as data
> > corruption with the help of poisoning and red zoning.
> > 
> > It is true that KMSAN might be doing a bit more than SLUB debug here, but the
> > question is if it is worth the maintenance burden? Do you have any existing bugs
> > to show that KMSAN would find by this patch but SLUB debug can't?
> 
> Consider the following case:
> 
>   int *a = kmalloc(sizeof(int), GFP_KERNEL);
>   if (*a) {
>     // do something, e.g. become root
>   }
> 
> IIUC SLUB poisoning will result in the branch being taken, but as long
> as there are no invalid accesses the kernel won't crash and no errors
> will be reported.
> KMSAN however will report an error, because the contents of the buffer
> pointed to by |a| are poisoned by kmsan_slab_alloc().
> The compiler instrumentation will retrieve the shadow value for *a,
> compare it to zero and report a use of uninitialized memory.
> 
> Another example would be copying the contents of newly allocated
> buffer to the userspace.

Make sense. BTW, it might worth testing this patchset with this MM debug config
based on linux-next to edge out some corner cases if you could,

https://raw.githubusercontent.com/cailca/linux-mm/master/x86.config

> 
> > > > 
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > > > > > To: Alexander Potapenko <glider@google.com>
> > > > > > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > > > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > > > > > Cc: linux-mm@kvack.org
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > > > > > > > ---
> > > > > > > > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > > > > > > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > > > > > > index 3d63ae320d31..3d6d4c63446e 100644
> > > > > > > > > --- a/mm/slub.c
> > > > > > > > > +++ b/mm/slub.c
> > > > > > > > > @@ -21,6 +21,8 @@
> > > > > > > > >  #include <linux/proc_fs.h>
> > > > > > > > >  #include <linux/seq_file.h>
> > > > > > > > >  #include <linux/kasan.h>
> > > > > > > > > +#include <linux/kmsan.h>
> > > > > > > > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > > > > > > > >  #include <linux/cpu.h>
> > > > > > > > >  #include <linux/cpuset.h>
> > > > > > > > >  #include <linux/mempolicy.h>
> > > > > > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > > > > > > > >       prefetch(object + s->offset);
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > +/*
> > > > > > > > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > > > > > > > + * pointer value in the case the current thread loses the race for the next
> > > > > > > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > > > > > > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > > > > > > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > > > > > > > + * handling of inline assembly.
> > > > > > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > > > > > > > + * return value of get_freepointer_safe().
> > > > > > > > > + */
> > > > > > > > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > > > > > > > >  {
> > > > > > > > >       unsigned long freepointer_addr;
> > > > > > > > >       void *p;
> > > > > > > > > 
> > > > > > > > >       if (!debug_pagealloc_enabled())
> > > > > > > > > -             return get_freepointer(s, object);
> > > > > > > > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > > > > > > > 
> > > > > > > > >       freepointer_addr = (unsigned long)object + s->offset;
> > > > > > > > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > > > > > > > -     return freelist_ptr(s, p, freepointer_addr);
> > > > > > > > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > > > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > > > > > > > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > > > > > > > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > > > > > > > >       kmemleak_alloc(ptr, size, 1, flags);
> > > > > > > > > +     kmsan_kmalloc_large(ptr, size, flags);
> > > > > > > > >       return ptr;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > > > > > > > >  {
> > > > > > > > >       kmemleak_free(x);
> > > > > > > > >       kasan_kfree_large(x, _RET_IP_);
> > > > > > > > > +     kmsan_kfree_large(x);
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > > > > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > > > > > > > >               } while (object != old_tail);
> > > > > > > > >       }
> > > > > > > > > 
> > > > > > > > > +     do {
> > > > > > > > > +             object = next;
> > > > > > > > > +             next = get_freepointer(s, object);
> > > > > > > > > +             kmsan_slab_free(s, object);
> > > > > > > > > +     } 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.
> > > > > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > > > > > > > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > > > > > > > >               memset(object, 0, s->object_size);
> > > > > > > > > 
> > > > > > > > > +     kmsan_slab_alloc(s, object, gfpflags);
> > > > > > > > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > > > > > > > 
> > > > > > > > >       return object;
> > > > > > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > > > > > > > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > > > > > > > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > > > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > > > +
> > > > > > > > >       return ret;
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > > > > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > > > > > > > 
> > > > > > > > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > > > > > > > >                                   s->object_size, s->size, gfpflags, node);
> > > > > > > > > -
> > > > > > > > >       return ret;
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > > > > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > > > > > > > >                          size, s->size, gfpflags, node);
> > > > > > > > > 
> > > > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > > > +
> > > > > > > > >       return ret;
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > > > > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > > > >                         void **p)
> > > > > > > > >  {
> > > > > > > > >       struct kmem_cache_cpu *c;
> > > > > > > > > -     int i;
> > > > > > > > > +     int i, j;
> > > > > > > > > 
> > > > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > > > >       s = slab_pre_alloc_hook(s, flags);
> > > > > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > > > > 
> > > > > > > > >       /* Clear memory outside IRQ disabled fastpath loop */
> > > > > > > > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > > > > > > > -             int j;
> > > > > > > > > -
> > > > > > > > >               for (j = 0; j < i; j++)
> > > > > > > > >                       memset(p[j], 0, s->object_size);
> > > > > > > > >       }
> > > > > > > > > +     for (j = 0; j < i; j++)
> > > > > > > > > +             kmsan_slab_alloc(s, p[j], flags);
> > > > > > > > > 
> > > > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > > > >       slab_post_alloc_hook(s, flags, size, p);
> > > > > > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > > > > > > > 
> > > > > > > > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > > > > > > > 
> > > > > > > > > +__no_sanitize_memory
> > > > > > > > >  void *__kmalloc(size_t size, gfp_t flags)
> > > > > > > > >  {
> > > > > > > > >       struct kmem_cache *s;
> > > > > > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > > > > > > > >       p += sprintf(p, "%07u", s->size);
> > > > > > > > > 
> > > > > > > > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > > > > > > > +     kmsan_unpoison_shadow(name, p - name);
> > > > > > > > >       return name;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > > > > > > > >       al->name = name;
> > > > > > > > >       al->next = alias_list;
> > > > > > > > >       alias_list = al;
> > > > > > > > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > > > > > > > >       return 0;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > > 
> 
> 
>