linux-kernel - Re: [PATCH] kasan: infer the requested size by scanning shadow memory

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: <dbaeb044c547ddb908bffdce4d2dfa0936805ef7.camel@mediatek.com>
Date:   Mon, 9 Jan 2023 05:02:38 +0000
From:   Kuan-Ying Lee (李冠穎) 
        <Kuan-Ying.Lee@...iatek.com>
To:     "andreyknvl@...il.com" <andreyknvl@...il.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        Qun-wei Lin (林群崴) 
        <Qun-wei.Lin@...iatek.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Chinwen Chang (張錦文) 
        <chinwen.chang@...iatek.com>,
        "dvyukov@...gle.com" <dvyukov@...gle.com>,
        "kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "ryabinin.a.a@...il.com" <ryabinin.a.a@...il.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "vincenzo.frascino@....com" <vincenzo.frascino@....com>,
        "glider@...gle.com" <glider@...gle.com>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>
Subject: Re: [PATCH] kasan: infer the requested size by scanning shadow memory

On Wed, 2023-01-04 at 03:00 +0100, Andrey Konovalov wrote:
> On Tue, Jan 3, 2023 at 8:56 AM Kuan-Ying Lee <
> Kuan-Ying.Lee@...iatek.com> wrote:
> > 
> > We scan the shadow memory to infer the requested size instead of
> > printing cache->object_size directly.
> > 
> > This patch will fix the confusing generic kasan report like below.
> > [1]
> > Report shows "cache kmalloc-192 of size 192", but user
> > actually kmalloc(184).
> > 
> > ==================================================================
> > BUG: KASAN: slab-out-of-bounds in _find_next_bit+0x143/0x160
> > lib/find_bit.c:109
> > Read of size 8 at addr ffff8880175766b8 by task kworker/1:1/26
> > ...
> > The buggy address belongs to the object at ffff888017576600
> >  which belongs to the cache kmalloc-192 of size 192
> > The buggy address is located 184 bytes inside of
> >  192-byte region [ffff888017576600, ffff8880175766c0)
> > ...
> > Memory state around the buggy address:
> >  ffff888017576580: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> >  ffff888017576600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > ffff888017576680: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
> > 
> >                                         ^
> >  ffff888017576700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >  ffff888017576780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ==================================================================
> > 
> > After this patch, report will show "cache kmalloc-192 of size 184".
> 
> I think this introduces more confusion. kmalloc-192 cache doesn't
> have
> the size of 184.
> 
> Let's leave the first two lines as is, and instead change the second
> two lines to:
> 
> The buggy address is located 0 bytes to the right of
>  requested 184-byte region [ffff888017576600, ffff8880175766c0)

Did you mean region [ffff888017576600, ffff8880175766b8)?

> 
> This specifically points out an out-of-bounds access.
> 
> Note the added "requested". Alternatively, we could say "allocated".
> 
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -340,8 +340,13 @@ static inline void
> > kasan_print_address_stack_frame(const void *addr) { }
> > 
> >  #ifdef CONFIG_KASAN_GENERIC
> >  void kasan_print_aux_stacks(struct kmem_cache *cache, const void
> > *object);
> > +int kasan_get_alloc_size(void *object_addr, struct kmem_cache
> > *cache);
> >  #else
> >  static inline void kasan_print_aux_stacks(struct kmem_cache
> > *cache, const void *object) { }
> > +static inline int kasan_get_alloc_size(void *object_addr, struct
> > kmem_cache *cache)
> > +{
> > +       return cache->object_size;
> 
> Please implement similar shadow/tag walking for the tag-based modes.
> Even though we can only deduce the requested size with the
> granularity
> of 16 bytes, it still makes sense.

Will do in v2.

> 
> It makes sense to also use the word "allocated" instead of
> "requested"
> for these modes, as the size is not deduced precisely.
> 
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -236,12 +236,13 @@ static void describe_object_addr(const void
> > *addr, struct kmem_cache *cache,
> >  {
> >         unsigned long access_addr = (unsigned long)addr;
> >         unsigned long object_addr = (unsigned long)object;
> > +       int real_size = kasan_get_alloc_size((void *)object_addr,
> > cache);
> 
> Please add another field to the mode-specific section of the
> kasan_report_info structure, fill it in complete_report_info, and use
> it here. See kasan_find_first_bad_addr as a reference.

Got it. Will do in v2.

> 
> Thanks for working on this!