Message-ID: <CA+fCnZf7sX2-H_jRMcJhiYxYZ=5f5oQ7iO__pQnjEXDLUS+fkg@mail.gmail.com>
Date: Wed, 23 Oct 2024 16:00:56 +0200
From: Andrey Konovalov <andreyknvl@...il.com>
To: Nihar Chaithanya <niharchaithanya@...il.com>, elver@...gle.com
Cc: ryabinin.a.a@...il.com, glider@...gle.com, dvyukov@...gle.com, 
	skhan@...uxfoundation.org, kasan-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kasan:report: filter out kasan related stack entries

On Mon, Oct 21, 2024 at 9:58 PM Nihar Chaithanya
<niharchaithanya@...il.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@...il.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.

> +
> +       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!

Powered by blists - more mailing lists