linux-kernel - Re: [PATCH mm] slub, kasan: improve interaction of KASAN and slub

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: <CA+fCnZcAnZh7H901SZFsaU=-XrpUeeJwUeThMpduDd1-Wt0gsA@mail.gmail.com>
Date:   Thu, 23 Nov 2023 17:12:08 +0100
From:   Andrey Konovalov <andreyknvl@...il.com>
To:     Feng Tang <feng.tang@...el.com>
Cc:     "andrey.konovalov@...ux.dev" <andrey.konovalov@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Marco Elver <elver@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
        Evgenii Stepanov <eugenis@...gle.com>,
        Oscar Salvador <osalvador@...e.de>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrey Konovalov <andreyknvl@...gle.com>
Subject: Re: [PATCH mm] slub, kasan: improve interaction of KASAN and
 slub_debug poisoning

On Thu, Nov 23, 2023 at 7:35 AM Feng Tang <feng.tang@...el.com> wrote:
>

Hi Feng,

> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -870,20 +870,20 @@ 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;
> >
> > -#ifdef CONFIG_KASAN_GENERIC
> >       /*
> > -      * KASAN could save its free meta data in object's data area at
> > -      * offset 0, if the size is larger than 'orig_size', it will
> > -      * overlap the data redzone in [orig_size+1, object_size], and
> > -      * the check should be skipped.
> > +      * 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.
> >        */
> > -     if (kasan_metadata_size(s, true) > orig_size)
> > -             orig_size = s->object_size;
> > -#endif
> > +     kasan_meta_size = kasan_metadata_size(s, true);
> > +     if (kasan_meta_size > orig_size)
> > +             orig_size = kasan_meta_size;
>
> 'orig_size' is to save the orignal request size for kmalloc object,
> and its main purpose is to detect the memory wastage of kmalloc
> objects, see commit 6edf2576a6cc "mm/slub: enable debugging memory
> wasting of kmalloc"
>
> Setting "orig_size = s->object_size" was to skip the wastage check
> and the redzone sanity check for this 'wasted space'.

Yes, I get that.

The point of my change was to allow slub_debug detecting overwrites in
the [kasan_meta_size, object_size) range when KASAN stores its free
meta in the [0, kasan_meta_size) range. If orig_size is set to
object_size, writes to that area will not be detected. I also thought
that using kasan_meta_size instead of object_size for orig_size might
give the reader better understanding of the memory layout.

> So it's better not to set 'kasan_meta_size' to orig_size.

I don't have a strong preference here: slub_debug and KASAN are not
really meant to be used together anyway. So if you prefer, I can
revert this change and keep using object_size as before.

> And from the below code, IIUC, the orig_size is not used in fixing
> the boot problem found by Hyeonggon?

No, this is a just a partially-related clean up. It just seemed
natural to include it into the fix, as it also touches the code around
a kasan_metadata_size call.

Thanks!