Re: [PATCH mm] slub, kasan: improve interaction of KASAN and slub_debug poisoning

[Andrey Konovalov]

On Thu, Nov 23, 2023 at 7:35 AM Feng Tang <feng.tang@xxxxxxxxx> 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!