Re: [PATCH v2] kasan:report: filter out kasan related stack entries

[Nihar Chaithanya <niharchaithanya@gmail.com>]

On 23/10/24 19:30, Andrey Konovalov wrote:
> On Mon, Oct 21, 2024 at 9:58 PM Nihar Chaithanya
> <niharchaithanya@gmail.com> wrote:
> Let's change the patch name prefix to "kasan: report:" (i.e. add an
> extra space between "kasan:" and "report:").
>
>> The reports of KASAN include KASAN related stack frames which are not
>> the point of interest in the stack-trace. KCSAN report filters out such
>> internal frames providing relevant stack trace. Currently, KASAN reports
>> are generated by dump_stack_lvl() which prints the entire stack.
>>
>> Add functionality to KASAN reports to save the stack entries and filter
>> out the kasan related stack frames in place of dump_stack_lvl() and
>> stack_depot_print().
>>
>> Within this new functionality:
>>          - A function kasan_dump_stack_lvl() in place of dump_stack_lvl() is
>>            created which contains functionality for saving, filtering and
>>            printing the stack-trace.
>>          - A function kasan_stack_depot_print() in place of
>>            stack_depot_print() is created which contains functionality for
>>            filtering and printing the stack-trace.
>>          - The get_stack_skipnr() function is included to get the number of
>>            stack entries to be skipped for filtering the stack-trace.
>>
>> Signed-off-by: Nihar Chaithanya <niharchaithanya@gmail.com>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=215756
>> ---
>> Changes in v2:
>>          - Changed the function name from save_stack_lvl_kasan() to
>>            kasan_dump_stack_lvl().
>>          - Added filtering of stack frames for print_track() with
>>            kasan_stack_depot_print().
>>          - Removed redundant print_stack_trace(), and instead using
>>            stack_trace_print() directly.
>>          - Removed sanitize_stack_entries() and replace_stack_entry()
>>            functions.
>>          - Increased the buffer size in get_stack_skipnr to 128.
>>
>> Note:
>> When using sanitize_stack_entries() the output was innacurate for free and
>> alloc tracks, because of the missing ip value in print_track().
>> The buffer size in get_stack_skipnr() is increase as it was too small when
>> testing with some KASAN uaf bugs which included free and alloc tracks.
>>
>>   mm/kasan/report.c | 62 ++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 56 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index b48c768acc84..e00cf764693c 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -261,6 +261,59 @@ static void print_error_description(struct kasan_report_info *info)
>>                          info->access_addr, current->comm, task_pid_nr(current));
>>   }
>>
>> +/* Helper to skip KASAN-related functions in stack-trace. */
>> +static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries)
>> +{
>> +       char buf[128];
>> +       int len, skip;
>> +
>> +       for (skip = 0; skip < num_entries; ++skip) {
>> +               len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skip]);
>> +
>> +               /* Never show  kasan_* functions. */
>> +               if (strnstr(buf, "kasan_", len) == buf)
>> +                       continue;
> Also check for "__kasan_" prefix: Right now, for the very first KASAN
> test, we get this alloc stack trace:
>
> [    1.799579] Allocated by task 63:
> [    1.799935]  __kasan_kmalloc+0x8b/0x90
> [    1.800353]  kmalloc_oob_right+0x95/0x6c0
> [    1.800801]  kunit_try_run_case+0x16e/0x280
> [    1.801267]  kunit_generic_run_threadfn_adapter+0x77/0xe0
> [    1.801863]  kthread+0x296/0x350
> [    1.802224]  ret_from_fork+0x2b/0x70
> [    1.802652]  ret_from_fork_asm+0x1a/0x30
>
> The __kasan_kmalloc frame is a part of KASAN internals and we want to
> skip that. kmalloc_oob_right is the function where the allocation
> happened, and that should be the first stack trace frame.
>
> (I suspect we'll have to adapt more of these from KFENCE, but let's do
> that after resolving the other issues.)
>
>> +               /*
>> +                * No match for runtime functions -- @skip entries to skip to
>> +                * get to first frame of interest.
>> +                */
>> +               break;
>> +       }
>> +
>> +       return skip;
>> +}
>> +
>> +/*
>> + * Use in place of stack_dump_lvl to filter KASAN related functions in
>> + * stack_trace.
> "Use in place of dump_stack() to filter out KASAN-related frames in
> the stack trace."
>
>> + */
>> +static void kasan_dump_stack_lvl(void)
> No need for the "_lvl" suffix - you removed the lvl argument.
>
>> +{
>> +       unsigned long stack_entries[KASAN_STACK_DEPTH] = { 0 };
>> +       int num_stack_entries = stack_trace_save(stack_entries, KASAN_STACK_DEPTH, 1);
>> +       int skipnr = get_stack_skipnr(stack_entries, num_stack_entries);
> For printing the access stack trace, we still want to keep the
> ip-based skipping (done via sanitize_stack_entries() in v1) - it's
> more precise than pattern-based matching in get_stack_skipnr(). But
> for alloc/free stack traces, we can only use get_stack_skipnr().
>
> However, I realized I don't fully get the point of replacing a stack
> trace entry when doind the ip-based skipping. Marco, is this something
> KCSAN-specific? I see that this is used for reodered_to thing.
When I included ip-based skipping for filtering access stack trace the 
output was
inconsistent where the Freed track was not fully printed and it also 
triggered
the following warning a few times:

[    6.467470][ T4653] Freed by task 511183648:
[    6.467792][ T4653] ------------[ cut here ]------------
[    6.468194][ T4653] pool index 100479 out of bounds (466) for stack 
id ffff8880
[    6.468862][ T4653] WARNING: CPU: 1 PID: 4653 at lib/stackdepot.c:452 
depot_fetch_stack+0x86/0xb0

This was not present when using pattern based skipping. Does modifying 
access
stack trace when using sanitize_stack_entries() modify the free and 
alloc tracks
as well? In that case shall we just use pattern based skipping.
>> +
>> +       dump_stack_print_info(KERN_ERR);
>> +       stack_trace_print(stack_entries + skipnr, num_stack_entries - skipnr, 0);
>> +       pr_err("\n");
>> +}
>> +
>> +/*
>> + * Use in place of stack_depot_print to filter KASAN related functions in
>> + * stack_trace.
> "Use in place of stack_depot_print() to filter out KASAN-related
> frames in the stack trace."
>
>> + */
>> +static void kasan_stack_depot_print(depot_stack_handle_t stack)
>> +{
>> +       unsigned long *entries;
>> +       unsigned int nr_entries;
>> +
>> +       nr_entries = stack_depot_fetch(stack, &entries);
>> +       int skipnr = get_stack_skipnr(entries, nr_entries);
>> +
>> +       if (nr_entries > 0)
>> +               stack_trace_print(entries + skipnr, nr_entries - skipnr, 0);
>> +}
>> +
>>   static void print_track(struct kasan_track *track, const char *prefix)
>>   {
>>   #ifdef CONFIG_KASAN_EXTRA_INFO
>> @@ -277,7 +330,7 @@ static void print_track(struct kasan_track *track, const char *prefix)
>>          pr_err("%s by task %u:\n", prefix, track->pid);
>>   #endif /* CONFIG_KASAN_EXTRA_INFO */
>>          if (track->stack)
>> -               stack_depot_print(track->stack);
>> +               kasan_stack_depot_print(track->stack);
>>          else
>>                  pr_err("(stack is not available)\n");
>>   }
>> @@ -374,9 +427,6 @@ static void print_address_description(void *addr, u8 tag,
>>   {
>>          struct page *page = addr_to_page(addr);
>>
>> -       dump_stack_lvl(KERN_ERR);
>> -       pr_err("\n");
> This new line we want to keep.
>
>> -
>>          if (info->cache && info->object) {
>>                  describe_object(addr, info);
>>                  pr_err("\n");
>> @@ -484,11 +534,11 @@ static void print_report(struct kasan_report_info *info)
>>                  kasan_print_tags(tag, info->first_bad_addr);
>>          pr_err("\n");
>>
>> +       kasan_dump_stack_lvl();
>> +
>>          if (addr_has_metadata(addr)) {
>>                  print_address_description(addr, tag, info);
>>                  print_memory_metadata(info->first_bad_addr);
>> -       } else {
>> -               dump_stack_lvl(KERN_ERR);
>>          }
>>   }
>>
>> --
>> 2.34.1
>>
> Thank you!