diff mbox series

kasan: infer the requested size by scanning shadow memory

Message ID 20230103075603.12294-1-Kuan-Ying.Lee@mediatek.com (mailing list archive)
State New
Headers show
Series kasan: infer the requested size by scanning shadow memory | expand

Commit Message

Kuan-Ying Lee (李冠穎) Jan. 3, 2023, 7:55 a.m. UTC
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(-)

Comments

Andrey Konovalov Jan. 4, 2023, 2 a.m. UTC | #1
On Tue, Jan 3, 2023 at 8:56 AM Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.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)

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.

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.

Thanks for working on this!
kernel test robot Jan. 5, 2023, 8:09 p.m. UTC | #2
Hi Kuan-Ying,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master v6.2-rc2 next-20230105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuan-Ying-Lee/kasan-infer-the-requested-size-by-scanning-shadow-memory/20230103-155641
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20230103075603.12294-1-Kuan-Ying.Lee%40mediatek.com
patch subject: [PATCH] kasan: infer the requested size by scanning shadow memory
config: arm64-randconfig-c004-20230105
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2e7537415684a55e473e98515beeef6d03e09c8f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kuan-Ying-Lee/kasan-infer-the-requested-size-by-scanning-shadow-memory/20230103-155641
        git checkout 2e7537415684a55e473e98515beeef6d03e09c8f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash mm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from mm/kasan/common.c:30:
   mm/kasan/kasan.h: In function 'kasan_get_alloc_size':
>> mm/kasan/kasan.h:348:21: error: invalid use of undefined type 'struct kmem_cache'
     348 |         return cache->object_size;
         |                     ^~
--
   In file included from mm/kasan/report.c:34:
   mm/kasan/kasan.h: In function 'kasan_get_alloc_size':
>> mm/kasan/kasan.h:348:21: error: invalid use of undefined type 'struct kmem_cache'
     348 |         return cache->object_size;
         |                     ^~
   mm/kasan/kasan.h:349:1: error: control reaches end of non-void function [-Werror=return-type]
     349 | }
         | ^
   cc1: some warnings being treated as errors
--
   In file included from mm/kasan/sw_tags.c:33:
   mm/kasan/kasan.h: In function 'kasan_get_alloc_size':
>> mm/kasan/kasan.h:348:21: error: invalid use of undefined type 'struct kmem_cache'
     348 |         return cache->object_size;
         |                     ^~
   mm/kasan/sw_tags.c: At top level:
   mm/kasan/sw_tags.c:173:6: warning: no previous prototype for 'kasan_tag_mismatch' [-Wmissing-prototypes]
     173 | void kasan_tag_mismatch(unsigned long addr, unsigned long access_info,
         |      ^~~~~~~~~~~~~~~~~~


vim +348 mm/kasan/kasan.h

   340	
   341	#ifdef CONFIG_KASAN_GENERIC
   342	void kasan_print_aux_stacks(struct kmem_cache *cache, const void *object);
   343	int kasan_get_alloc_size(void *object_addr, struct kmem_cache *cache);
   344	#else
   345	static inline void kasan_print_aux_stacks(struct kmem_cache *cache, const void *object) { }
   346	static inline int kasan_get_alloc_size(void *object_addr, struct kmem_cache *cache)
   347	{
 > 348		return cache->object_size;
   349	}
   350	#endif
   351
Kuan-Ying Lee (李冠穎) Jan. 9, 2023, 5:02 a.m. UTC | #3
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@mediatek.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!
Dmitry Vyukov Jan. 9, 2023, 6:51 a.m. UTC | #4
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";
Andrey Konovalov Jan. 9, 2023, 8:55 p.m. UTC | #5
On Mon, Jan 9, 2023 at 6:02 AM Kuan-Ying Lee (李冠穎)
<Kuan-Ying.Lee@mediatek.com> wrote:
>
> > 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)?

Yes! Forgot to change the range. The idea is to refer to the requested
size in these two lines of the report.
Kuan-Ying Lee (李冠穎) Jan. 13, 2023, 7:59 a.m. UTC | #6
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@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://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@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 think it won't break reporting for other report types.
This function is only called by slab OOB and UAF.

> 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";
Dmitry Vyukov Jan. 13, 2023, 8:01 a.m. UTC | #7
On Fri, 13 Jan 2023 at 08:59, 'Kuan-Ying Lee (李冠穎)' via kasan-dev
<kasan-dev@googlegroups.com> 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@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://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@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 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.

> > 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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/edbcce8a1e9e772e3a3fd032cd4600bd5677c877.camel%40mediatek.com.
Kuan-Ying Lee (李冠穎) Jan. 13, 2023, 9:28 a.m. UTC | #8
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@googlegroups.com> 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@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://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@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 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@googlegroups.com.
> > 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$  .
diff mbox series

Patch

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