linux-kernel - Re: [PATCH 1/5] mm/kasan: Don't store metadata inside kmalloc object when slub_debug_orig

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zt+sUs46cZc0vh/K@feng-clx.sh.intel.com>
Date: Tue, 10 Sep 2024 10:17:54 +0800
From: Feng Tang <feng.tang@...el.com>
To: Andrey Konovalov <andreyknvl@...il.com>
CC: Vlastimil Babka <vbabka@...e.cz>, Andrew Morton
	<akpm@...ux-foundation.org>, Christoph Lameter <cl@...ux.com>, Pekka Enberg
	<penberg@...nel.org>, David Rientjes <rientjes@...gle.com>, Joonsoo Kim
	<iamjoonsoo.kim@....com>, Roman Gushchin <roman.gushchin@...ux.dev>,
	Hyeonggon Yoo <42.hyeyoo@...il.com>, Marco Elver <elver@...gle.com>, "Shuah
 Khan" <skhan@...uxfoundation.org>, David Gow <davidgow@...gle.com>, "Danilo
 Krummrich" <dakr@...nel.org>, <linux-mm@...ck.org>,
	<kasan-dev@...glegroups.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] mm/kasan: Don't store metadata inside kmalloc object
 when slub_debug_orig_size is on

On Mon, Sep 09, 2024 at 06:24:21PM +0200, Andrey Konovalov wrote:
> On Mon, Sep 9, 2024 at 3:30 AM Feng Tang <feng.tang@...el.com> wrote:
> >
> > For a kmalloc object, when both kasan and slub redzone sanity check
> > are enabled, they could both manipulate its data space like storing
> > kasan free meta data and setting up kmalloc redzone, and may affect
> > accuracy of that object's 'orig_size'.
> >
> > As an accurate 'orig_size' will be needed by some function like
> > krealloc() soon, save kasan's free meta data in slub's metadata area
> > instead of inside object when 'orig_size' is enabled.
> >
> > This will make it easier to maintain/understand the code. Size wise,
> > when these two options are both enabled, the slub meta data space is
> > already huge, and this just slightly increase the overall size.
> >
> > Signed-off-by: Feng Tang <feng.tang@...el.com>
> > ---
> >  mm/kasan/generic.c |  5 ++++-
> >  mm/slab.h          |  6 ++++++
> >  mm/slub.c          | 17 -----------------
> >  3 files changed, 10 insertions(+), 18 deletions(-)
> >
> > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > index 6310a180278b..cad376199d47 100644
> > --- a/mm/kasan/generic.c
> > +++ b/mm/kasan/generic.c
> > @@ -393,8 +393,11 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
> >          *    be touched after it was freed, or
> >          * 2. Object has a constructor, which means it's expected to
> >          *    retain its content until the next allocation.
> 
> Nit: ", or" above.

Aha, yes, I missed that.

Hi Vlastimil,

Could you help to change it when taking the patches, or you prefer me
to send a new version? thanks!

> 
> > +        * 3. It is from a kmalloc cache which enables the debug option
> > +        *    to store original size.
> >          */
> > -       if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor) {
> > +       if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor ||
> > +            slub_debug_orig_size(cache)) {
> >                 cache->kasan_info.free_meta_offset = *size;
> >                 *size += sizeof(struct kasan_free_meta);
> >                 goto free_meta_added;
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 90f95bda4571..7a0e9b34ba2a 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -689,6 +689,12 @@ void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab)
> >  void __check_heap_object(const void *ptr, unsigned long n,
> >                          const struct slab *slab, bool to_user);
> >
> > +static inline bool slub_debug_orig_size(struct kmem_cache *s)
> > +{
> > +       return (kmem_cache_debug_flags(s, SLAB_STORE_USER) &&
> > +                       (s->flags & SLAB_KMALLOC));
> > +}
> > +
> >  #ifdef CONFIG_SLUB_DEBUG
> >  void skip_orig_size_check(struct kmem_cache *s, const void *object);
> >  #endif
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 23761533329d..996a72fa6f62 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -230,12 +230,6 @@ static inline bool kmem_cache_debug(struct kmem_cache *s)
> >         return kmem_cache_debug_flags(s, SLAB_DEBUG_FLAGS);
> >  }
> >
> > -static inline bool slub_debug_orig_size(struct kmem_cache *s)
> > -{
> > -       return (kmem_cache_debug_flags(s, SLAB_STORE_USER) &&
> > -                       (s->flags & SLAB_KMALLOC));
> > -}
> > -
> >  void *fixup_red_left(struct kmem_cache *s, void *p)
> >  {
> >         if (kmem_cache_debug_flags(s, SLAB_RED_ZONE))
> > @@ -760,21 +754,10 @@ static inline void set_orig_size(struct kmem_cache *s,
> >                                 void *object, unsigned int orig_size)
> >  {
> >         void *p = kasan_reset_tag(object);
> > -       unsigned int kasan_meta_size;
> >
> >         if (!slub_debug_orig_size(s))
> >                 return;
> >
> > -       /*
> > -        * KASAN can save its free meta data inside of the object at offset 0.
> > -        * If this meta data size is larger than 'orig_size', it will overlap
> > -        * the data redzone in [orig_size+1, object_size]. Thus, we adjust
> > -        * 'orig_size' to be as at least as big as KASAN's meta data.
> > -        */
> > -       kasan_meta_size = kasan_metadata_size(s, true);
> > -       if (kasan_meta_size > orig_size)
> > -               orig_size = kasan_meta_size;
> > -
> >         p += get_info_end(s);
> >         p += sizeof(struct track) * 2;
> >
> > --
> > 2.34.1
> >
> 
> Acked-by: Andrey Konovalov <andreyknvl@...il.com>

Thank you!

- Feng