| Message ID | 20230118093832.1945-1-Kuan-Ying.Lee@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kasan: infer the requested size by scanning shadow memory | expand |
On Wed, Jan 18, 2023 at 10:39 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 kasan slab-out-of-bounds > 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, slab-out-of-bounds report will show as below. > ================================================================== > ... > The buggy address belongs to the object at ffff888017576600 > which belongs to the cache kmalloc-192 of size 192 > The buggy address is located 0 bytes right of > allocated 184-byte region [ffff888017576600, ffff8880175766b8) > ... > ================================================================== > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216457 [1] > > Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> > --- > V1 -> V2: > - Implement getting allocated size of object for tag-based kasan. > - Refine the kasan report. > - Check if it is slab-out-of-bounds report type. > - Thanks for Andrey and Dmitry suggestion. Hi Kuan-Ying, I came up with a few more things to fix while testing your patch and decided to address them myself. Please check the v3 here: https://github.com/xairy/linux/commit/012a584a9f11ba08a6051b075f7fd0a0eb54c719 The significant changes are to print "freed" for a slab-use-after-free and only print the region state for the Generic mode (printing it for Tag-Based modes doesn't work properly atm, see the comment in the code). The rest is clean-ups and a few added comments. See the full list of changes in the commit message. Please check whether this v3 looks good to you, and then feel free to submit it. Thank you!
On Mon, 2023-01-23 at 22:46 +0100, Andrey Konovalov wrote: > On Wed, Jan 18, 2023 at 10:39 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 kasan slab-out-of-bounds > > 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, slab-out-of-bounds report will show as below. > > ================================================================== > > ... > > The buggy address belongs to the object at ffff888017576600 > > which belongs to the cache kmalloc-192 of size 192 > > The buggy address is located 0 bytes right of > > allocated 184-byte region [ffff888017576600, ffff8880175766b8) > > ... > > ================================================================== > > > > Link: > > https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=216457__;!!CTRNKA9wMg0ARbw!iEOOICl7DzhvfYobmQ8MsNFAWmbqicXdjd0LYWw9uBOqwj8lai7oEODVdRJyWUEXr11A3-m7wbIX2cdpxLwiW6Tm$ > > $ [1] > > > > Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> > > --- > > V1 -> V2: > > - Implement getting allocated size of object for tag-based kasan. > > - Refine the kasan report. > > - Check if it is slab-out-of-bounds report type. > > - Thanks for Andrey and Dmitry suggestion. > > Hi Kuan-Ying, > > I came up with a few more things to fix while testing your patch and > decided to address them myself. Please check the v3 here: > > https://urldefense.com/v3/__https://github.com/xairy/linux/commit/012a584a9f11ba08a6051b075f7fd0a0eb54c719__;!!CTRNKA9wMg0ARbw!iEOOICl7DzhvfYobmQ8MsNFAWmbqicXdjd0LYWw9uBOqwj8lai7oEODVdRJyWUEXr11A3-m7wbIX2cdpxNwCtfpJ$ > > > The significant changes are to print "freed" for a slab-use-after- > free > and only print the region state for the Generic mode (printing it for > Tag-Based modes doesn't work properly atm, see the comment in the > code). The rest is clean-ups and a few added comments. See the full > list of changes in the commit message. > > Please check whether this v3 looks good to you, and then feel free to > submit it. It looks good to me. I will send the v3. Thank you. > Thank you!
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h index abbcc1b0eec5..15ffd46fec6a 100644 --- a/mm/kasan/kasan.h +++ b/mm/kasan/kasan.h @@ -185,6 +185,7 @@ struct kasan_report_info { const char *bug_type; struct kasan_track alloc_track; struct kasan_track free_track; + int obj_size; }; /* Do not change the struct layout: compiler ABI. */ @@ -306,6 +307,7 @@ static inline bool addr_has_metadata(const void *addr) void *kasan_find_first_bad_addr(void *addr, size_t size); void kasan_complete_mode_report_info(struct kasan_report_info *info); void kasan_metadata_fetch_row(char *buffer, void *row); +int kasan_get_alloc_size(void *object_addr, struct kmem_cache *cache); #if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS) void kasan_print_tags(u8 addr_tag, const void *addr); diff --git a/mm/kasan/report.c b/mm/kasan/report.c index df3602062bfd..dae0d4ae8fe9 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -210,12 +210,13 @@ static inline struct page *addr_to_page(const void *addr) } static void describe_object_addr(const void *addr, struct kmem_cache *cache, - void *object) + void *object, int obj_size, const char *bug_type) { unsigned long access_addr = (unsigned long)addr; unsigned long object_addr = (unsigned long)object; const char *rel_type; int rel_bytes; + bool slab_oob = false; pr_err("The buggy address belongs to the object at %px\n" " which belongs to the cache %s of size %d\n", @@ -224,18 +225,22 @@ static void describe_object_addr(const void *addr, struct kmem_cache *cache, if (access_addr < object_addr) { rel_type = "to the left"; rel_bytes = object_addr - access_addr; - } else if (access_addr >= object_addr + cache->object_size) { + } else if (access_addr >= object_addr + obj_size) { rel_type = "to the right"; - rel_bytes = access_addr - (object_addr + cache->object_size); + rel_bytes = access_addr - (object_addr + obj_size); } else { rel_type = "inside"; rel_bytes = access_addr - object_addr; } + if (strcmp(bug_type, "slab-out-of-bounds") == 0) + slab_oob = true; + pr_err("The buggy address is located %d bytes %s of\n" - " %d-byte region [%px, %px)\n", - rel_bytes, rel_type, cache->object_size, (void *)object_addr, - (void *)(object_addr + cache->object_size)); + " %s%d-byte region [%px, %px)\n", + rel_bytes, rel_type, slab_oob ? "allocated " : "", + obj_size, (void *)object_addr, + (void *)(object_addr + obj_size)); } static void describe_object_stacks(struct kasan_report_info *info) @@ -257,7 +262,8 @@ static void describe_object(const void *addr, struct kasan_report_info *info) { if (kasan_stack_collection_enabled()) describe_object_stacks(info); - describe_object_addr(addr, info->cache, info->object); + describe_object_addr(addr, info->cache, info->object, info->obj_size, + info->bug_type); } static inline bool kernel_or_module_addr(const void *addr) diff --git a/mm/kasan/report_generic.c b/mm/kasan/report_generic.c index 043c94b04605..7b4bec9e6d1a 100644 --- a/mm/kasan/report_generic.c +++ b/mm/kasan/report_generic.c @@ -43,6 +43,25 @@ 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; + + 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"; @@ -149,6 +168,11 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info) memcpy(&info->free_track, &free_meta->free_track, sizeof(info->free_track)); } + + if (strcmp(info->bug_type, "slab-out-of-bounds") == 0) + info->obj_size = kasan_get_alloc_size(info->object, info->cache); + else + info->obj_size = info->cache->object_size; } void kasan_metadata_fetch_row(char *buffer, void *row) diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c index f3d3be614e4b..e462dd750fe2 100644 --- a/mm/kasan/report_hw_tags.c +++ b/mm/kasan/report_hw_tags.c @@ -21,6 +21,24 @@ void *kasan_find_first_bad_addr(void *addr, size_t size) return kasan_reset_tag(addr); } +int kasan_get_alloc_size(void *addr, struct kmem_cache *cache) +{ + int size = 0, i = 0; + u8 memory_tag; + + while (size < cache->object_size) { + memory_tag = hw_get_mem_tag(addr + i * KASAN_GRANULE_SIZE); + + if (memory_tag != KASAN_TAG_INVALID) + size += KASAN_GRANULE_SIZE; + else + return size; + i++; + } + + return cache->object_size; +} + void kasan_metadata_fetch_row(char *buffer, void *row) { int i; diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c index 7a26397297ed..d50caefd7fd5 100644 --- a/mm/kasan/report_sw_tags.c +++ b/mm/kasan/report_sw_tags.c @@ -45,6 +45,23 @@ 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; + + shadow = (u8 *)kasan_mem_to_shadow(addr); + while (size < cache->object_size) { + if (*shadow != KASAN_TAG_INVALID) + size += KASAN_GRANULE_SIZE; + else + return size; + shadow++; + } + + return cache->object_size; +} + void kasan_metadata_fetch_row(char *buffer, void *row) { memcpy(buffer, kasan_mem_to_shadow(row), META_BYTES_PER_ROW); diff --git a/mm/kasan/report_tags.c b/mm/kasan/report_tags.c index ecede06ef374..b349a0ae1b83 100644 --- a/mm/kasan/report_tags.c +++ b/mm/kasan/report_tags.c @@ -7,6 +7,7 @@ #include <linux/atomic.h> #include "kasan.h" +#include "../slab.h" extern struct kasan_stack_ring stack_ring; @@ -113,4 +114,11 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info) /* Assign the common bug type if no entries were found. */ if (!info->bug_type) info->bug_type = get_common_bug_type(info); + + if (info->object && info->cache) { + if (strcmp(info->bug_type, "slab-out-of-bounds") == 0) + info->obj_size = kasan_get_alloc_size(info->object, info->cache); + else + info->obj_size = info->cache->object_size; + } }