Re: [PATCH v2] kasan:report: filter out kasan related stack entries
From: Nihar Chaithanya
Date: Thu Oct 24 2024 - 22:48:38 EST
On 23/10/24 19:30, Andrey Konovalov wrote:
On Mon, Oct 21, 2024 at 9:58 PM Nihar Chaithanya
<niharchaithanya@xxxxxxxxx> 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@xxxxxxxxx>
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!