mm/kasan: Print frame description for stack bugs
Subsystems (2)
PATCH v2 1/1 mm/kasan: Print frame description for stack bugs
This adds support for printing stack frame description on invalid stack accesses. The frame description is embedded by the compiler, which is parsed and then pretty-printed.
Currently, we can only print the stack frame info for accesses to the task's own stack, but not accesses to other tasks' stacks.
Example of what it looks like:
[ 17.924050] page dumped because: kasan: bad access detected [ 17.924908] [ 17.925153] addr ffff8880673ef98a is located in stack of task insmod/2008 at offset 106 in frame: [ 17.926542] kasan_stack_oob+0x0/0xf5 [test_kasan] [ 17.927932] [ 17.928206] this frame has 2 objects: [ 17.928783] [32, 36) 'i' [ 17.928784] [96, 106) 'stack_array' [ 17.929216] [ 17.930031] Memory state around the buggy address:
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198435
---
Changes since V1:
- Fix types in printf (%zu -> %lu).
- Prefer 'unsigned long', to ensure offsets/addrs are pointer sized, as emitted by ASAN instrumentation.
Change-Id: I4836cde103052991ac8871796a45b4c977c9e2e7
--- mm/kasan/kasan.h | 5 ++ mm/kasan/report.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+)
mm/kasan/kasan.h +5 -0
@@ -43,6 +43,11 @@#define KASAN_ALLOCA_REDZONE_SIZE 32 +/* +* Stack frame marker (compiler ABI). +*/ +#define KASAN_CURRENT_STACK_FRAME_MAGIC 0x41B58AB3 +/* Don't break randconfig/all*config builds */ #ifndef KASAN_ABI_VERSION #define KASAN_ABI_VERSION 1
mm/kasan/report.c +163 -0
181 lines not on this page · raw message
Signature · 1 line
2.21.0.1020.gf2820cf01a-goog
Quoted 32 lines · Marco Elver
mm/kasan/report.c +32
-0+static void print_decoded_frame_descr (const char *frame_descr) +{ +/* +* We need to parse the following string: +* "n alloc_1 alloc_2 ... alloc_n" +* where alloc_i looks like +* "offset size len name" +* or "offset size len name:line". +*/ + +char token[64 ]; +unsigned long num_objects; + +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &num_objects)) +return ; + +pr_err ("\n" ); +pr_err ("this frame has %lu %s:\n" , num_objects, + num_objects ==1 ?"object" :"objects" ); + +while (num_objects--) { +unsigned long offset; +unsigned long size; + +/* access offset */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &offset)) +return ; +/* access size */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &size))
mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 259
Quoted 10 lines
mm/kasan/report.c +10
-0+return ; +/* name length (unused) */ +if (!tokenize_frame_descr (&frame_descr,NULL ,0 ,NULL )) +return ; +/* object name */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), +NULL )) +return ; + +/* Strip line number, if it exists. */
Why?
Quoted 2 lines
(quoted diff) +2
-0+ strreplace(token, ':', '\0'); +
...
Quoted 6 lines
mm/kasan/report.c +6
-0+ + aligned_addr =round_down ((unsigned long )addr,sizeof (long )); + mem_ptr =round_down (aligned_addr,KASAN_SHADOW_SCALE_SIZE ); + shadow_ptr =kasan_mem_to_shadow ((void *)aligned_addr); + shadow_bottom =kasan_mem_to_shadow (end_of_stack (current)); +
mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 299
Quoted 10 lines
mm/kasan/report.c +10
-0+while (shadow_ptr >= shadow_bottom && *shadow_ptr !=KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } + +while (shadow_ptr >= shadow_bottom && *shadow_ptr ==KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } +
I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch. But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
Alexander Potapenko
reviewerQuoted 66 lines · Andrey Ryabinin
Quoted 32 lines · Marco Elver
mm/kasan/report.c +32
-0+static void print_decoded_frame_descr (const char *frame_descr) +{ +/* +* We need to parse the following string: +* "n alloc_1 alloc_2 ... alloc_n" +* where alloc_i looks like +* "offset size len name" +* or "offset size len name:line". +*/ + +char token[64 ]; +unsigned long num_objects; + +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &num_objects)) +return ; + +pr_err ("\n" ); +pr_err ("this frame has %lu %s:\n" , num_objects, + num_objects ==1 ?"object" :"objects" ); + +while (num_objects--) { +unsigned long offset; +unsigned long size; + +/* access offset */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &offset)) +return ; +/* access size */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &size))mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 259
Quoted 10 lines
mm/kasan/report.c +10
-0+return ; +/* name length (unused) */ +if (!tokenize_frame_descr (&frame_descr,NULL ,0 ,NULL )) +return ; +/* object name */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), +NULL )) +return ; + +/* Strip line number, if it exists. */ Why?
Quoted 2 lines
(quoted diff) +2
-0+ strreplace(token, ':', '\0'); +...
Quoted 11 lines
mm/kasan/report.c +11
-0+ + aligned_addr =round_down ((unsigned long )addr,sizeof (long )); + mem_ptr =round_down (aligned_addr,KASAN_SHADOW_SCALE_SIZE ); + shadow_ptr =kasan_mem_to_shadow ((void *)aligned_addr); + shadow_bottom =kasan_mem_to_shadow (end_of_stack (current)); + +while (shadow_ptr >= shadow_bottom && *shadow_ptr !=KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } +
Quoted 8 lines
mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 299
Quoted 5 lines
mm/kasan/report.c +5
-0+while (shadow_ptr >= shadow_bottom && *shadow_ptr ==KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } +I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch. But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
Note that KASAN was broken on parisc from day 1 because of other assumptions on the stack growth direction hardcoded into KASAN (e.g. __kasan_unpoison_stack() and __asan_allocas_unpoison()). So maybe this BUILD_BUG_ON can be added in a separate patch as it's not specific to what Marco is doing here?
Quoted 7 lines
--
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 [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/ebec4325-f91b-b392-55ed-95dbd36bbb8e%40virtuozzo.com.
For more options, visit https://groups.google.com/d/optout.
Signature · 10 lines
Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
Quoted 41 lines · Alexander Potapenko
Quoted 39 lines · Andrey Ryabinin
Quoted 36 lines · Marco Elver
mm/kasan/report.c +36
-0+static void print_decoded_frame_descr (const char *frame_descr) +{ +/* +* We need to parse the following string: +* "n alloc_1 alloc_2 ... alloc_n" +* where alloc_i looks like +* "offset size len name" +* or "offset size len name:line". +*/ + +char token[64 ]; +unsigned long num_objects; + +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &num_objects)) +return ; + +pr_err ("\n" ); +pr_err ("this frame has %lu %s:\n" , num_objects, + num_objects ==1 ?"object" :"objects" ); + +while (num_objects--) { +unsigned long offset; +unsigned long size; + +/* access offset */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &offset)) +return ; +/* access size */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &size)) +return ; +/* name length (unused) */ +if (!tokenize_frame_descr (&frame_descr,NULL ,0 ,NULL )) +return ;
Quoted 8 lines
Quoted 8 lines
mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 259
Quoted 6 lines
mm/kasan/report.c +6
-0+/* object name */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), +NULL )) +return ; + +/* Strip line number, if it exists. */ Why?
The filename is not included, and I don't think it adds much in terms of ability to debug; nor is the line number included with all descriptions. I think, the added complexity of separating the line number and parsing is not worthwhile here. Alternatively, I could not pay attention to the line number at all, and leave it as is -- in that case, some variable names will display as "foo:123".
Quoted 25 lines
Quoted 17 lines
Quoted 3 lines
(quoted diff) +2
-0+ strreplace(token, ':', '\0'); +...
Quoted 11 lines
mm/kasan/report.c +11
-0+ + aligned_addr =round_down ((unsigned long )addr,sizeof (long )); + mem_ptr =round_down (aligned_addr,KASAN_SHADOW_SCALE_SIZE ); + shadow_ptr =kasan_mem_to_shadow ((void *)aligned_addr); + shadow_bottom =kasan_mem_to_shadow (end_of_stack (current)); + +while (shadow_ptr >= shadow_bottom && *shadow_ptr !=KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } +Quoted 8 lines
mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 299
Quoted 5 lines
mm/kasan/report.c +5
-0+while (shadow_ptr >= shadow_bottom && *shadow_ptr ==KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } +I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch. But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
Quoted 5 lines
Note that KASAN was broken on parisc from day 1 because of other assumptions on the stack growth direction hardcoded into KASAN (e.g.
__kasan_unpoison_stack()and__asan_allocas_unpoison()). So maybe thisBUILD_BUG_ONcan be added in a separate patch as it's not specific to what Marco is doing here?
Happy to send a follow-up patch, or add here. Let me know what you prefer.
Thanks,
-- Marco
Quoted 51 lines · Marco Elver
Quoted 41 lines · Alexander Potapenko
Quoted 39 lines · Andrey Ryabinin
Quoted 36 lines · Marco Elver
mm/kasan/report.c +36
-0+static void print_decoded_frame_descr (const char *frame_descr) +{ +/* +* We need to parse the following string: +* "n alloc_1 alloc_2 ... alloc_n" +* where alloc_i looks like +* "offset size len name" +* or "offset size len name:line". +*/ + +char token[64 ]; +unsigned long num_objects; + +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &num_objects)) +return ; + +pr_err ("\n" ); +pr_err ("this frame has %lu %s:\n" , num_objects, + num_objects ==1 ?"object" :"objects" ); + +while (num_objects--) { +unsigned long offset; +unsigned long size; + +/* access offset */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &offset)) +return ; +/* access size */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), + &size)) +return ; +/* name length (unused) */ +if (!tokenize_frame_descr (&frame_descr,NULL ,0 ,NULL )) +return ;Quoted 8 lines
Quoted 8 lines
mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 259
Quoted 6 lines
mm/kasan/report.c +6
-0+/* object name */ +if (!tokenize_frame_descr (&frame_descr, token,sizeof (token), +NULL )) +return ; + +/* Strip line number, if it exists. */ Why?
Quoted 7 lines
The filename is not included, and I don't think it adds much in terms of ability to debug; nor is the line number included with all descriptions. I think, the added complexity of separating the line number and parsing is not worthwhile here. Alternatively, I could not pay attention to the line number at all, and leave it as is -- in that case, some variable names will display as "foo:123".
Either way is fine by me. But explain why in comment if you decide to keep current code. Something like
/* Strip line number cause it's not very helpful. */
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
Quoted 3 lines
(quoted diff) +2
-0+ strreplace(token, ':', '\0'); +...
Quoted 6 lines
mm/kasan/report.c +6
-0+ + aligned_addr =round_down ((unsigned long )addr,sizeof (long )); + mem_ptr =round_down (aligned_addr,KASAN_SHADOW_SCALE_SIZE ); + shadow_ptr =kasan_mem_to_shadow ((void *)aligned_addr); + shadow_bottom =kasan_mem_to_shadow (end_of_stack (current)); +mm/kasan/report.c static inline bool init_task_stack_addr(const void *addr) · line 299
Quoted 10 lines
mm/kasan/report.c +10
-0+while (shadow_ptr >= shadow_bottom && *shadow_ptr !=KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } + +while (shadow_ptr >= shadow_bottom && *shadow_ptr ==KASAN_STACK_LEFT ) { + shadow_ptr--; + mem_ptr -=KASAN_SHADOW_SCALE_SIZE ; + } +I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch. But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
Quoted 3 lines
Quoted 3 lines
Note that KASAN was broken on parisc from day 1 because of other assumptions on the stack growth direction hardcoded into KASAN (e.g.
__kasan_unpoison_stack()and__asan_allocas_unpoison()).
It's not broken, it doesn't exist.
Quoted 3 lines
Quoted 2 lines
So maybe this
BUILD_BUG_ONcan be added in a separate patch as it's not specific to what Marco is doing here?
I think it's fine to add it in this patch because BUILD_BUG_ON() is just a hint for developers that this particular function depends on growing down stack. So it's more a property of the function rather than KASAN in general.
Other functions you mentioned can be marked with BUILD_BUG_ON()s as well, but not in this patch indeed.
Quoted 2 lines
Happy to send a follow-up patch, or add here. Let me know what you prefer.
Send v3 please.