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;
+		/*
+		 * 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.
+ */
+static void kasan_dump_stack_lvl(void)
+{
+	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);
+
+	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.
+ */
+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");
-
 	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

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.

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

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!

On Fri, Oct 25, 2024 at 4:48 AM Nihar Chaithanya
<niharchaithanya@gmail.com> wrote:
>
> 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.

To clarify once again: we only want the ip-based filtering for the
access stack trace (the one printed directly from print_report()). For
Allocated/Freed stack traces, we want to use the pattern-based
filtering.