Re: [PATCH v5 4/4] mm/slub: extend redzone check to extra allocated kmalloc space than requested

[Feng Tang <feng.tang@intel.com>]

On Sun, Sep 11, 2022 at 07:12:05AM +0800, Andrey Konovalov wrote:
> On Wed, Sep 7, 2022 at 9:11 AM Feng Tang <feng.tang@intel.com> wrote:
> >
> > kmalloc will round up the request size to a fixed size (mostly power
> > of 2), so there could be a extra space than what is requested, whose
> > size is the actual buffer size minus original request size.
> >
> > To better detect out of bound access or abuse of this space, add
> > redzone sanity check for it.
> >
> > And in current kernel, some kmalloc user already knows the existence
> > of the space and utilizes it after calling 'ksize()' to know the real
> > size of the allocated buffer. So we skip the sanity check for objects
> > which have been called with ksize(), as treating them as legitimate
> > users.
> >
> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
> >  mm/slab.h        |  4 ++++
> >  mm/slab_common.c |  4 ++++
> >  mm/slub.c        | 57 +++++++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 62 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 20f9e2a9814f..0bc91b30b031 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -885,4 +885,8 @@ void __check_heap_object(const void *ptr, unsigned long n,
> >  }
> >  #endif
> >
> > +#ifdef CONFIG_SLUB_DEBUG
> > +void skip_orig_size_check(struct kmem_cache *s, const void *object);
> > +#endif
> > +
> >  #endif /* MM_SLAB_H */
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 8e13e3aac53f..5106667d6adb 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -1001,6 +1001,10 @@ size_t __ksize(const void *object)
> >                 return folio_size(folio);
> >         }
> >
> > +#ifdef CONFIG_SLUB_DEBUG
> > +       skip_orig_size_check(folio_slab(folio)->slab_cache, object);
> > +#endif
> > +
> >         return slab_ksize(folio_slab(folio)->slab_cache);
> >  }
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index f523601d3fcf..2f0302136604 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -812,12 +812,27 @@ static inline void set_orig_size(struct kmem_cache *s,
> >         if (!slub_debug_orig_size(s))
> >                 return;
> >
> > +#ifdef CONFIG_KASAN_GENERIC
> > +       /*
> > +        * KASAN could save its free meta data in the start part of object
> > +        * area, so skip the redzone check if kasan's meta data size is
> > +        * bigger enough to possibly overlap with kmalloc redzone
> > +        */
> > +       if (s->kasan_info.free_meta_size_in_object * 2 >= s->object_size)
> 
> Why is free_meta_size_in_object multiplied by 2? Looks cryptic,
> probably needs a comment.
 
OK, will change, I didn't make it clear. 

The basic idea is kasan's free-meta could be saved in object's data
area at offset 0, and it could overlap the kmalloc's in-object
redzone, which can only be in the second half part of the data
area. And as long as kasan's free meta sits in the first half,
then it's fine.

Maybe I can change the check to

  if (s->kasan_info.free_meta_size_in_object > orig_size)
	...

Thanks,
Feng

> Thanks!
>