Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230103075603.12294-1-Kuan-Ying.Lee@mediatek.com>
In-Reply-To: <20230103075603.12294-1-Kuan-Ying.Lee@mediatek.com>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Mon, 9 Jan 2023 07:51:04 +0100
Message-ID: <CACT4Y+b5hbCod=Gj6oGxFrq5CaFPbz5T9A0nomzhWooiXQy5aA@mail.gmail.com>
Subject: Re: [PATCH] kasan: infer the requested size by scanning shadow memory
To:     Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>
Cc:     Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        chinwen.chang@mediatek.com, qun-wei.lin@mediatek.com,
        kasan-dev@googlegroups.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 3 Jan 2023 at 08:56, 'Kuan-Ying Lee' via kasan-dev
<kasan-dev@googlegroups.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".
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216457 [1]
>
> Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>
> ---
>  mm/kasan/kasan.h          |  5 +++++
>  mm/kasan/report.c         |  3 ++-
>  mm/kasan/report_generic.c | 18 ++++++++++++++++++
>  3 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 32413f22aa82..7bb627d21580 100644
> --- 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;
> +}
>  #endif
>
>  bool kasan_report(unsigned long addr, size_t size,
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 1d02757e90a3..6de454bb2cad 100644
> --- 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);
>         const char *rel_type;
>         int rel_bytes;
>
>         pr_err("The buggy address belongs to the object at %px\n"
>                " which belongs to the cache %s of size %d\n",
> -               object, cache->name, cache->object_size);
> +               object, cache->name, real_size);
>
>         if (access_addr < object_addr) {
>                 rel_type = "to the left";
> diff --git a/mm/kasan/report_generic.c b/mm/kasan/report_generic.c
> index 043c94b04605..01b38e459352 100644
> --- a/mm/kasan/report_generic.c
> +++ b/mm/kasan/report_generic.c
> @@ -43,6 +43,24 @@ void *kasan_find_first_bad_addr(void *addr, size_t size)
>         return p;
>  }
>
> +int kasan_get_alloc_size(void *addr, struct kmem_cache *cache)
> +{
> +       int size = 0;
> +       u8 *shadow = (u8 *)kasan_mem_to_shadow(addr);
> +
> +       while (size < cache->object_size) {
> +               if (*shadow == 0)
> +                       size += KASAN_GRANULE_SIZE;
> +               else if (*shadow >= 1 && *shadow <= KASAN_GRANULE_SIZE - 1)
> +                       size += *shadow;
> +               else
> +                       return size;
> +               shadow++;

This only works for out-of-bounds reports, but I don't see any checks
for report type. Won't this break reporting for all other report
types?

I would also print the cache name anyway. Sometimes reports are
perplexing and/or this logic may return a wrong result for some
reason. The total object size may be useful to understand harder
cases.

> +       }
> +
> +       return cache->object_size;
> +}
> +
>  static const char *get_shadow_bug_type(struct kasan_report_info *info)
>  {
>         const char *bug_type = "unknown-crash";