| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANpmjNPQQid6UirgZkBov-WhpyRR_5tqazvfS5f_K3PoAF3WYw@mail.gmail.com>
Date: Sat, 26 Oct 2024 21:05:14 +0200
From: Marco Elver <elver@...gle.com>
To: Nihar Chaithanya <niharchaithanya@...il.com>
Cc: ryabinin.a.a@...il.com, andreyknvl@...il.com, dvyukov@...gle.com,
glider@...gle.com, skhan@...uxfoundation.org, kasan-dev@...glegroups.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] kasan: report: filter out kasan related stack entries
On Sat, 26 Oct 2024 at 18:17, Nihar Chaithanya
<niharchaithanya@...il.com> wrote:
>
> 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 which employs pattern based stack
> filtering is included.
> - The replace_stack_entry() function which uses ip value based
> stack filtering is included.
>
> 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.
>
> Changes in v3:
> - Included an additional criteria for pattern based filtering
> in get_stack_skipnr().
> - Included ip value based stack filtering with the functions
> sanitize_stack_entries() and replace_stack_entry().
> - Corrected the comments and name of the newly added functions
> kasan_dump_stack() and kasan_stack_depot_print().
>
> mm/kasan/report.c | 111 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 107 insertions(+), 4 deletions(-)
>
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 3e48668c3e40..648a89fea3e7 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -261,6 +261,110 @@ 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[64];
> + int len, skip;
> +
> + for (skip = 0; skip < num_entries; ++skip) {
> + len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skip]);
> +
> + /* Never show kasan_* or __kasan_* functions. */
> + if ((strnstr(buf, "kasan_", len) == buf) ||
> + (strnstr(buf, "__kasan_", len) == buf))
> + continue;
> + /*
> + * No match for runtime functions -- @skip entries to skip to
> + * get to first frame of interest.
> + */
> + break;
> + }
> +
> + return skip;
> +}
> +
> +/*
> + * Skips to the first entry that matches the function of @ip, and then replaces
> + * that entry with @ip, returning the entries to skip with @replaced containing
> + * the replaced entry.
> + */
> +static int
> +replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned long ip,
> + unsigned long *replaced)
> +{
> + unsigned long symbolsize, offset;
> + unsigned long target_func;
> + int skip;
> +
> + if (kallsyms_lookup_size_offset(ip, &symbolsize, &offset))
> + target_func = ip - offset;
> + else
> + goto fallback;
> +
> + for (skip = 0; skip < num_entries; ++skip) {
> + unsigned long func = stack_entries[skip];
> +
> + if (!kallsyms_lookup_size_offset(func, &symbolsize, &offset))
> + goto fallback;
> + func -= offset;
> +
> + if (func == target_func) {
> + *replaced = stack_entries[skip];
All this replaced logic is not needed for KASAN.
> + stack_entries[skip] = ip;
> + return skip;
> + }
> + }
> +
> +fallback:
> + /* Should not happen; the resulting stack trace is likely misleading. */
> + WARN_ONCE(1, "Cannot find frame for %pS in stack trace", (void *)ip);
> + return get_stack_skipnr(stack_entries, num_entries);
> +}
> +
> +static int
> +sanitize_stack_entries(unsigned long stack_entries[], int num_entries, unsigned long ip,
> + unsigned long *replaced)
> +{
> + return ip ? replace_stack_entry(stack_entries, num_entries, ip, replaced) :
> + get_stack_skipnr(stack_entries, num_entries);
> +}
> +
> +/*
> + * Use in place of dump_stack() to filter out KASAN-related frames in
> + * the stack trace.
> + */
> +static void kasan_dump_stack(unsigned long ip)
> +{
> + unsigned long reordered_to = 0;
Do you understand what this code is doing? The whole "reordered_to"
logic (along with the "replaced" logic in replace_stack_entry()) is
very specific to KCSAN and not at all needed for KASAN. See the commit
that introduced it:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/kernel/kcsan/report.c?id=be3f6967ec5947dd7b2f23bf9d42bb2729889618
You've copied and pasted the code from KCSAN, in the hopes it more or
less does what we want. However, the code as-is is more complex than
needed.
Please try to really understand what the most optimal way to do this
might be. As additional inspiration, please look at how KFENCE does
it: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/kfence/report.c#n49
In the end what I'd like to see is the simplest possible way to do
this for KASAN.
Powered by blists - more mailing lists