| Message ID | 20230103075603.12294-1-Kuan-Ying.Lee@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: infer the requested size by scanning shadow memory | expand |
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!
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
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!
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";
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.
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";
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.
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 --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";