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

From: Kuan-Ying Lee (李冠穎)
Date: Fri Jan 13 2023 - 04:34:33 EST

Next message: Dan Carpenter: "Re: [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()"
Previous message: Bard Liao: "[PATCH 5/5] soundwire: cadence: further simplify low-level xfer_msg_defer() callback"
In reply to: Dmitry Vyukov: "Re: [PATCH] kasan: infer the requested size by scanning shadow memory"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 2023-01-13 at 09:01 +0100, Dmitry Vyukov wrote:
> On Fri, 13 Jan 2023 at 08:59, 'Kuan-Ying Lee (李冠穎)' via kasan-dev
> <kasan-dev@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, 2023-01-09 at 07:51 +0100, Dmitry Vyukov wrote:
> > > On Tue, 3 Jan 2023 at 08:56, 'Kuan-Ying Lee' via kasan-dev
> > > <kasan-dev@xxxxxxxxxxxxxxxx> 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://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=216457__;!!CTRNKA9wMg0ARbw!mLNcuZ83c39d0Xkut-WMY3CcvZcAYDuLCmv4mu7IAldw4_n4i6XvX8GORBfjOadWxOa6d-ODQdx6ZCSvB2g13Q$
> > > > $ [1]
> > > >
> > > > Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@xxxxxxxxxxxx>
> > > > ---
> > > > 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 think it won't break reporting for other report types.
> > This function is only called by slab OOB and UAF.
>
> I meant specifically UAF reports.
> During UAF there are no 0s in the object shadow.
>

Ok.
I will check the report type in v2.

> > > 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.
> > >
> >
> > Ok. I will keep the cache name and the total object_size.
> >
> > > > + }
> > > > +
> > > > + return cache->object_size;
> > > > +}
> > > > +
> > > > static const char *get_shadow_bug_type(struct
> > > > kasan_report_info
> > > > *info)
> > > > {
> > > > const char *bug_type = "unknown-crash";
> >
> > --
> > You received this message because you are subscribed to the Google
> > Groups "kasan-dev" group.
> > To unsubscribe from this group and stop receiving emails from it,
> > send an email to kasan-dev+unsubscribe@xxxxxxxxxxxxxxxx.
> > To view this discussion on the web visit
> > https://urldefense.com/v3/__https://groups.google.com/d/msgid/kasan-dev/edbcce8a1e9e772e3a3fd032cd4600bd5677c877.camel*40mediatek.com__;JQ!!CTRNKA9wMg0ARbw!nLk2eBIc9qAXEy50sxxXRS2IRZKY8WSfVt_T3VtaMDrIrRHx31xOy5cTmqZa1py5ifu9UiHoqrKmxtnVKcWfJQ$
> > Q$ .

Next message: Dan Carpenter: "Re: [PATCH] f2fs: use iostat_lat_type directly as a parameter in the iostat_update_and_unbind_ctx()"
Previous message: Bard Liao: "[PATCH 5/5] soundwire: cadence: further simplify low-level xfer_msg_defer() callback"
In reply to: Dmitry Vyukov: "Re: [PATCH] kasan: infer the requested size by scanning shadow memory"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]