All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] mm/kasan: Print frame description for stack bugs
@ 2019-05-20 15:47 ` Marco Elver
  0 siblings, 0 replies; 8+ messages in thread
From: Marco Elver @ 2019-05-20 15:47 UTC (permalink / raw)
  To: aryabinin, dvyukov, glider, andreyknvl, akpm
  Cc: linux-kernel, linux-mm, kasan-dev, Marco Elver

This adds support for printing stack frame description on invalid stack
accesses. The frame description is embedded by the compiler, which is
parsed and then pretty-printed.

Currently, we can only print the stack frame info for accesses to the
task's own stack, but not accesses to other tasks' stacks.

Example of what it looks like:

[   17.924050] page dumped because: kasan: bad access detected
[   17.924908]
[   17.925153] addr ffff8880673ef98a is located in stack of task insmod/2008 at offset 106 in frame:
[   17.926542]  kasan_stack_oob+0x0/0xf5 [test_kasan]
[   17.927932]
[   17.928206] this frame has 2 objects:
[   17.928783]  [32, 36) 'i'
[   17.928784]  [96, 106) 'stack_array'
[   17.929216]
[   17.930031] Memory state around the buggy address:

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198435
Signed-off-by: Marco Elver <elver@google.com>
---

Changes since V1:
- Fix types in printf (%zu -> %lu).
- Prefer 'unsigned long', to ensure offsets/addrs are pointer sized, as
  emitted by ASAN instrumentation.

Change-Id: I4836cde103052991ac8871796a45b4c977c9e2e7
---
 mm/kasan/kasan.h  |   5 ++
 mm/kasan/report.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+)

diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 3ce956efa0cb..1979db4763e2 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -43,6 +43,11 @@
 
 #define KASAN_ALLOCA_REDZONE_SIZE	32
 
+/*
+ * Stack frame marker (compiler ABI).
+ */
+#define KASAN_CURRENT_STACK_FRAME_MAGIC 0x41B58AB3
+
 /* Don't break randconfig/all*config builds */
 #ifndef KASAN_ABI_VERSION
 #define KASAN_ABI_VERSION 1
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 03a443579386..36e55956acaf 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -28,6 +28,7 @@
 #include <linux/types.h>
 #include <linux/kasan.h>
 #include <linux/module.h>
+#include <linux/sched/task_stack.h>
 
 #include <asm/sections.h>
 
@@ -181,6 +182,166 @@ static inline bool init_task_stack_addr(const void *addr)
 			sizeof(init_thread_union.stack));
 }
 
+static bool __must_check tokenize_frame_descr(const char **frame_descr,
+					      char *token, size_t max_tok_len,
+					      unsigned long *value)
+{
+	const char *sep = strchr(*frame_descr, ' ');
+
+	if (sep == NULL)
+		sep = *frame_descr + strlen(*frame_descr);
+
+	if (token != NULL) {
+		const size_t tok_len = sep - *frame_descr;
+
+		if (tok_len + 1 > max_tok_len) {
+			pr_err("KASAN internal error: frame description too long: %s\n",
+			       *frame_descr);
+			return false;
+		}
+
+		/* Copy token (+ 1 byte for '\0'). */
+		strlcpy(token, *frame_descr, tok_len + 1);
+	}
+
+	/* Advance frame_descr past separator. */
+	*frame_descr = sep + 1;
+
+	if (value != NULL && kstrtoul(token, 10, value)) {
+		pr_err("KASAN internal error: not a valid number: %s\n", token);
+		return false;
+	}
+
+	return true;
+}
+
+static void print_decoded_frame_descr(const char *frame_descr)
+{
+	/*
+	 * We need to parse the following string:
+	 *    "n alloc_1 alloc_2 ... alloc_n"
+	 * where alloc_i looks like
+	 *    "offset size len name"
+	 * or "offset size len name:line".
+	 */
+
+	char token[64];
+	unsigned long num_objects;
+
+	if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+				  &num_objects))
+		return;
+
+	pr_err("\n");
+	pr_err("this frame has %lu %s:\n", num_objects,
+	       num_objects == 1 ? "object" : "objects");
+
+	while (num_objects--) {
+		unsigned long offset;
+		unsigned long size;
+
+		/* access offset */
+		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+					  &offset))
+			return;
+		/* access size */
+		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+					  &size))
+			return;
+		/* name length (unused) */
+		if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
+			return;
+		/* object name */
+		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+					  NULL))
+			return;
+
+		/* Strip line number, if it exists. */
+		strreplace(token, ':', '\0');
+
+		/* Finally, print object information. */
+		pr_err(" [%lu, %lu) '%s'", offset, offset + size, token);
+	}
+}
+
+static bool __must_check get_address_stack_frame_info(const void *addr,
+						      unsigned long *offset,
+						      const char **frame_descr,
+						      const void **frame_pc)
+{
+	unsigned long aligned_addr;
+	unsigned long mem_ptr;
+	const u8 *shadow_bottom;
+	const u8 *shadow_ptr;
+	const unsigned long *frame;
+
+	/*
+	 * NOTE: We currently only support printing frame information for
+	 * accesses to the task's own stack.
+	 */
+	if (!object_is_on_stack(addr))
+		return false;
+
+	aligned_addr = round_down((unsigned long)addr, sizeof(long));
+	mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
+	shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
+	shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
+
+	while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
+		shadow_ptr--;
+		mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
+	}
+
+	while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
+		shadow_ptr--;
+		mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
+	}
+
+	if (shadow_ptr < shadow_bottom)
+		return false;
+
+	frame = (const unsigned long *)(mem_ptr + KASAN_SHADOW_SCALE_SIZE);
+	if (frame[0] != KASAN_CURRENT_STACK_FRAME_MAGIC) {
+		pr_err("KASAN internal error: frame info validation failed; invalid marker: %lu\n",
+		       frame[0]);
+		return false;
+	}
+
+	*offset = (unsigned long)addr - (unsigned long)frame;
+	*frame_descr = (const char *)frame[1];
+	*frame_pc = (void *)frame[2];
+
+	return true;
+}
+
+static void print_address_stack_frame(const void *addr)
+{
+	unsigned long offset;
+	const char *frame_descr;
+	const void *frame_pc;
+
+	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))
+		return;
+
+	if (!get_address_stack_frame_info(addr, &offset, &frame_descr,
+					  &frame_pc))
+		return;
+
+	/*
+	 * get_address_stack_frame_info only returns true if the given addr is
+	 * on the current task's stack.
+	 */
+	pr_err("\n");
+	pr_err("addr %px is located in stack of task %s/%d at offset %lu in frame:\n",
+	       addr, current->comm, task_pid_nr(current), offset);
+	pr_err(" %pS\n", frame_pc);
+
+	if (!frame_descr)
+		return;
+
+	print_decoded_frame_descr(frame_descr);
+}
+
 static void print_address_description(void *addr)
 {
 	struct page *page = addr_to_page(addr);
@@ -204,6 +365,8 @@ static void print_address_description(void *addr)
 		pr_err("The buggy address belongs to the page:\n");
 		dump_page(page, "kasan: bad access detected");
 	}
+
+	print_address_stack_frame(addr);
 }
 
 static bool row_is_guilty(const void *row, const void *guilty)
-- 
2.21.0.1020.gf2820cf01a-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH v2] mm/kasan: Print frame description for stack bugs
@ 2019-05-20 15:47 ` Marco Elver
  0 siblings, 0 replies; 8+ messages in thread
From: Marco Elver @ 2019-05-20 15:47 UTC (permalink / raw)
  To: aryabinin, dvyukov, glider, andreyknvl, akpm
  Cc: linux-kernel, linux-mm, kasan-dev, Marco Elver

This adds support for printing stack frame description on invalid stack
accesses. The frame description is embedded by the compiler, which is
parsed and then pretty-printed.

Currently, we can only print the stack frame info for accesses to the
task's own stack, but not accesses to other tasks' stacks.

Example of what it looks like:

[   17.924050] page dumped because: kasan: bad access detected
[   17.924908]
[   17.925153] addr ffff8880673ef98a is located in stack of task insmod/2008 at offset 106 in frame:
[   17.926542]  kasan_stack_oob+0x0/0xf5 [test_kasan]
[   17.927932]
[   17.928206] this frame has 2 objects:
[   17.928783]  [32, 36) 'i'
[   17.928784]  [96, 106) 'stack_array'
[   17.929216]
[   17.930031] Memory state around the buggy address:

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198435
Signed-off-by: Marco Elver <elver@google.com>
---

Changes since V1:
- Fix types in printf (%zu -> %lu).
- Prefer 'unsigned long', to ensure offsets/addrs are pointer sized, as
  emitted by ASAN instrumentation.

Change-Id: I4836cde103052991ac8871796a45b4c977c9e2e7
---
 mm/kasan/kasan.h  |   5 ++
 mm/kasan/report.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+)

diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 3ce956efa0cb..1979db4763e2 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -43,6 +43,11 @@
 
 #define KASAN_ALLOCA_REDZONE_SIZE	32
 
+/*
+ * Stack frame marker (compiler ABI).
+ */
+#define KASAN_CURRENT_STACK_FRAME_MAGIC 0x41B58AB3
+
 /* Don't break randconfig/all*config builds */
 #ifndef KASAN_ABI_VERSION
 #define KASAN_ABI_VERSION 1
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 03a443579386..36e55956acaf 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -28,6 +28,7 @@
 #include <linux/types.h>
 #include <linux/kasan.h>
 #include <linux/module.h>
+#include <linux/sched/task_stack.h>
 
 #include <asm/sections.h>
 
@@ -181,6 +182,166 @@ static inline bool init_task_stack_addr(const void *addr)
 			sizeof(init_thread_union.stack));
 }
 
+static bool __must_check tokenize_frame_descr(const char **frame_descr,
+					      char *token, size_t max_tok_len,
+					      unsigned long *value)
+{
+	const char *sep = strchr(*frame_descr, ' ');
+
+	if (sep == NULL)
+		sep = *frame_descr + strlen(*frame_descr);
+
+	if (token != NULL) {
+		const size_t tok_len = sep - *frame_descr;
+
+		if (tok_len + 1 > max_tok_len) {
+			pr_err("KASAN internal error: frame description too long: %s\n",
+			       *frame_descr);
+			return false;
+		}
+
+		/* Copy token (+ 1 byte for '\0'). */
+		strlcpy(token, *frame_descr, tok_len + 1);
+	}
+
+	/* Advance frame_descr past separator. */
+	*frame_descr = sep + 1;
+
+	if (value != NULL && kstrtoul(token, 10, value)) {
+		pr_err("KASAN internal error: not a valid number: %s\n", token);
+		return false;
+	}
+
+	return true;
+}
+
+static void print_decoded_frame_descr(const char *frame_descr)
+{
+	/*
+	 * We need to parse the following string:
+	 *    "n alloc_1 alloc_2 ... alloc_n"
+	 * where alloc_i looks like
+	 *    "offset size len name"
+	 * or "offset size len name:line".
+	 */
+
+	char token[64];
+	unsigned long num_objects;
+
+	if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+				  &num_objects))
+		return;
+
+	pr_err("\n");
+	pr_err("this frame has %lu %s:\n", num_objects,
+	       num_objects == 1 ? "object" : "objects");
+
+	while (num_objects--) {
+		unsigned long offset;
+		unsigned long size;
+
+		/* access offset */
+		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+					  &offset))
+			return;
+		/* access size */
+		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+					  &size))
+			return;
+		/* name length (unused) */
+		if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
+			return;
+		/* object name */
+		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
+					  NULL))
+			return;
+
+		/* Strip line number, if it exists. */
+		strreplace(token, ':', '\0');
+
+		/* Finally, print object information. */
+		pr_err(" [%lu, %lu) '%s'", offset, offset + size, token);
+	}
+}
+
+static bool __must_check get_address_stack_frame_info(const void *addr,
+						      unsigned long *offset,
+						      const char **frame_descr,
+						      const void **frame_pc)
+{
+	unsigned long aligned_addr;
+	unsigned long mem_ptr;
+	const u8 *shadow_bottom;
+	const u8 *shadow_ptr;
+	const unsigned long *frame;
+
+	/*
+	 * NOTE: We currently only support printing frame information for
+	 * accesses to the task's own stack.
+	 */
+	if (!object_is_on_stack(addr))
+		return false;
+
+	aligned_addr = round_down((unsigned long)addr, sizeof(long));
+	mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
+	shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
+	shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
+
+	while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
+		shadow_ptr--;
+		mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
+	}
+
+	while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
+		shadow_ptr--;
+		mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
+	}
+
+	if (shadow_ptr < shadow_bottom)
+		return false;
+
+	frame = (const unsigned long *)(mem_ptr + KASAN_SHADOW_SCALE_SIZE);
+	if (frame[0] != KASAN_CURRENT_STACK_FRAME_MAGIC) {
+		pr_err("KASAN internal error: frame info validation failed; invalid marker: %lu\n",
+		       frame[0]);
+		return false;
+	}
+
+	*offset = (unsigned long)addr - (unsigned long)frame;
+	*frame_descr = (const char *)frame[1];
+	*frame_pc = (void *)frame[2];
+
+	return true;
+}
+
+static void print_address_stack_frame(const void *addr)
+{
+	unsigned long offset;
+	const char *frame_descr;
+	const void *frame_pc;
+
+	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))
+		return;
+
+	if (!get_address_stack_frame_info(addr, &offset, &frame_descr,
+					  &frame_pc))
+		return;
+
+	/*
+	 * get_address_stack_frame_info only returns true if the given addr is
+	 * on the current task's stack.
+	 */
+	pr_err("\n");
+	pr_err("addr %px is located in stack of task %s/%d at offset %lu in frame:\n",
+	       addr, current->comm, task_pid_nr(current), offset);
+	pr_err(" %pS\n", frame_pc);
+
+	if (!frame_descr)
+		return;
+
+	print_decoded_frame_descr(frame_descr);
+}
+
 static void print_address_description(void *addr)
 {
 	struct page *page = addr_to_page(addr);
@@ -204,6 +365,8 @@ static void print_address_description(void *addr)
 		pr_err("The buggy address belongs to the page:\n");
 		dump_page(page, "kasan: bad access detected");
 	}
+
+	print_address_stack_frame(addr);
 }
 
 static bool row_is_guilty(const void *row, const void *guilty)
-- 
2.21.0.1020.gf2820cf01a-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/kasan: Print frame description for stack bugs
  2019-05-20 15:47 ` Marco Elver
  (?)
@ 2019-05-21 15:43 ` Andrey Ryabinin
  2019-05-21 15:52     ` Alexander Potapenko
  -1 siblings, 1 reply; 8+ messages in thread
From: Andrey Ryabinin @ 2019-05-21 15:43 UTC (permalink / raw)
  To: Marco Elver, dvyukov, glider, andreyknvl, akpm
  Cc: linux-kernel, linux-mm, kasan-dev



On 5/20/19 6:47 PM, Marco Elver wrote:

> +static void print_decoded_frame_descr(const char *frame_descr)
> +{
> +	/*
> +	 * We need to parse the following string:
> +	 *    "n alloc_1 alloc_2 ... alloc_n"
> +	 * where alloc_i looks like
> +	 *    "offset size len name"
> +	 * or "offset size len name:line".
> +	 */
> +
> +	char token[64];
> +	unsigned long num_objects;
> +
> +	if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> +				  &num_objects))
> +		return;
> +
> +	pr_err("\n");
> +	pr_err("this frame has %lu %s:\n", num_objects,
> +	       num_objects == 1 ? "object" : "objects");
> +
> +	while (num_objects--) {
> +		unsigned long offset;
> +		unsigned long size;
> +
> +		/* access offset */
> +		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> +					  &offset))
> +			return;
> +		/* access size */
> +		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> +					  &size))
> +			return;
> +		/* name length (unused) */
> +		if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
> +			return;
> +		/* object name */
> +		if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> +					  NULL))
> +			return;
> +
> +		/* Strip line number, if it exists. */

   Why?

> +		strreplace(token, ':', '\0');
> +

...

> +
> +	aligned_addr = round_down((unsigned long)addr, sizeof(long));
> +	mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
> +	shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
> +	shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
> +
> +	while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
> +		shadow_ptr--;
> +		mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> +	}
> +
> +	while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
> +		shadow_ptr--;
> +		mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> +	}
> +

I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch.
But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.



^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/kasan: Print frame description for stack bugs
  2019-05-21 15:43 ` Andrey Ryabinin
@ 2019-05-21 15:52     ` Alexander Potapenko
  0 siblings, 0 replies; 8+ messages in thread
From: Alexander Potapenko @ 2019-05-21 15:52 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Marco Elver, Dmitriy Vyukov, Andrey Konovalov, Andrew Morton,
	LKML, Linux Memory Management List, kasan-dev

On Tue, May 21, 2019 at 5:43 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
>
> On 5/20/19 6:47 PM, Marco Elver wrote:
>
> > +static void print_decoded_frame_descr(const char *frame_descr)
> > +{
> > +     /*
> > +      * We need to parse the following string:
> > +      *    "n alloc_1 alloc_2 ... alloc_n"
> > +      * where alloc_i looks like
> > +      *    "offset size len name"
> > +      * or "offset size len name:line".
> > +      */
> > +
> > +     char token[64];
> > +     unsigned long num_objects;
> > +
> > +     if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                               &num_objects))
> > +             return;
> > +
> > +     pr_err("\n");
> > +     pr_err("this frame has %lu %s:\n", num_objects,
> > +            num_objects == 1 ? "object" : "objects");
> > +
> > +     while (num_objects--) {
> > +             unsigned long offset;
> > +             unsigned long size;
> > +
> > +             /* access offset */
> > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                                       &offset))
> > +                     return;
> > +             /* access size */
> > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                                       &size))
> > +                     return;
> > +             /* name length (unused) */
> > +             if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
> > +                     return;
> > +             /* object name */
> > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                                       NULL))
> > +                     return;
> > +
> > +             /* Strip line number, if it exists. */
>
>    Why?
>
> > +             strreplace(token, ':', '\0');
> > +
>
> ...
>
> > +
> > +     aligned_addr = round_down((unsigned long)addr, sizeof(long));
> > +     mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
> > +     shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
> > +     shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
> > +
> > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
> > +             shadow_ptr--;
> > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > +     }
> > +
> > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
> > +             shadow_ptr--;
> > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > +     }
> > +
>
> I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch.
> But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
Note that KASAN was broken on parisc from day 1 because of other
assumptions on the stack growth direction hardcoded into KASAN
(e.g. __kasan_unpoison_stack() and __asan_allocas_unpoison()).
So maybe this BUILD_BUG_ON can be added in a separate patch as it's
not specific to what Marco is doing here?
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/ebec4325-f91b-b392-55ed-95dbd36bbb8e%40virtuozzo.com.
> For more options, visit https://groups.google.com/d/optout.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/kasan: Print frame description for stack bugs
@ 2019-05-21 15:52     ` Alexander Potapenko
  0 siblings, 0 replies; 8+ messages in thread
From: Alexander Potapenko @ 2019-05-21 15:52 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Marco Elver, Dmitriy Vyukov, Andrey Konovalov, Andrew Morton,
	LKML, Linux Memory Management List, kasan-dev

On Tue, May 21, 2019 at 5:43 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
>
> On 5/20/19 6:47 PM, Marco Elver wrote:
>
> > +static void print_decoded_frame_descr(const char *frame_descr)
> > +{
> > +     /*
> > +      * We need to parse the following string:
> > +      *    "n alloc_1 alloc_2 ... alloc_n"
> > +      * where alloc_i looks like
> > +      *    "offset size len name"
> > +      * or "offset size len name:line".
> > +      */
> > +
> > +     char token[64];
> > +     unsigned long num_objects;
> > +
> > +     if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                               &num_objects))
> > +             return;
> > +
> > +     pr_err("\n");
> > +     pr_err("this frame has %lu %s:\n", num_objects,
> > +            num_objects == 1 ? "object" : "objects");
> > +
> > +     while (num_objects--) {
> > +             unsigned long offset;
> > +             unsigned long size;
> > +
> > +             /* access offset */
> > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                                       &offset))
> > +                     return;
> > +             /* access size */
> > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                                       &size))
> > +                     return;
> > +             /* name length (unused) */
> > +             if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
> > +                     return;
> > +             /* object name */
> > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > +                                       NULL))
> > +                     return;
> > +
> > +             /* Strip line number, if it exists. */
>
>    Why?
>
> > +             strreplace(token, ':', '\0');
> > +
>
> ...
>
> > +
> > +     aligned_addr = round_down((unsigned long)addr, sizeof(long));
> > +     mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
> > +     shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
> > +     shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
> > +
> > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
> > +             shadow_ptr--;
> > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > +     }
> > +
> > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
> > +             shadow_ptr--;
> > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > +     }
> > +
>
> I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch.
> But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
Note that KASAN was broken on parisc from day 1 because of other
assumptions on the stack growth direction hardcoded into KASAN
(e.g. __kasan_unpoison_stack() and __asan_allocas_unpoison()).
So maybe this BUILD_BUG_ON can be added in a separate patch as it's
not specific to what Marco is doing here?
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/ebec4325-f91b-b392-55ed-95dbd36bbb8e%40virtuozzo.com.
> For more options, visit https://groups.google.com/d/optout.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/kasan: Print frame description for stack bugs
  2019-05-21 15:52     ` Alexander Potapenko
@ 2019-05-21 16:07       ` Marco Elver
  -1 siblings, 0 replies; 8+ messages in thread
From: Marco Elver @ 2019-05-21 16:07 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: Andrey Ryabinin, Dmitriy Vyukov, Andrey Konovalov, Andrew Morton,
	LKML, Linux Memory Management List, kasan-dev

On Tue, 21 May 2019 at 17:53, Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, May 21, 2019 at 5:43 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> >
> > On 5/20/19 6:47 PM, Marco Elver wrote:
> >
> > > +static void print_decoded_frame_descr(const char *frame_descr)
> > > +{
> > > +     /*
> > > +      * We need to parse the following string:
> > > +      *    "n alloc_1 alloc_2 ... alloc_n"
> > > +      * where alloc_i looks like
> > > +      *    "offset size len name"
> > > +      * or "offset size len name:line".
> > > +      */
> > > +
> > > +     char token[64];
> > > +     unsigned long num_objects;
> > > +
> > > +     if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                               &num_objects))
> > > +             return;
> > > +
> > > +     pr_err("\n");
> > > +     pr_err("this frame has %lu %s:\n", num_objects,
> > > +            num_objects == 1 ? "object" : "objects");
> > > +
> > > +     while (num_objects--) {
> > > +             unsigned long offset;
> > > +             unsigned long size;
> > > +
> > > +             /* access offset */
> > > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                                       &offset))
> > > +                     return;
> > > +             /* access size */
> > > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                                       &size))
> > > +                     return;
> > > +             /* name length (unused) */
> > > +             if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
> > > +                     return;
> > > +             /* object name */
> > > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                                       NULL))
> > > +                     return;
> > > +
> > > +             /* Strip line number, if it exists. */
> >
> >    Why?

The filename is not included, and I don't think it adds much in terms
of ability to debug; nor is the line number included with all
descriptions. I think, the added complexity of separating the line
number and parsing is not worthwhile here. Alternatively, I could not
pay attention to the line number at all, and leave it as is -- in that
case, some variable names will display as "foo:123".

> >
> > > +             strreplace(token, ':', '\0');
> > > +
> >
> > ...
> >
> > > +
> > > +     aligned_addr = round_down((unsigned long)addr, sizeof(long));
> > > +     mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
> > > +     shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
> > > +     shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
> > > +
> > > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
> > > +             shadow_ptr--;
> > > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > > +     }
> > > +
> > > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
> > > +             shadow_ptr--;
> > > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > > +     }
> > > +
> >
> > I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch.
> > But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
> Note that KASAN was broken on parisc from day 1 because of other
> assumptions on the stack growth direction hardcoded into KASAN
> (e.g. __kasan_unpoison_stack() and __asan_allocas_unpoison()).
> So maybe this BUILD_BUG_ON can be added in a separate patch as it's
> not specific to what Marco is doing here?

Happy to send a follow-up patch, or add here. Let me know what you prefer.

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/kasan: Print frame description for stack bugs
@ 2019-05-21 16:07       ` Marco Elver
  0 siblings, 0 replies; 8+ messages in thread
From: Marco Elver @ 2019-05-21 16:07 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: Andrey Ryabinin, Dmitriy Vyukov, Andrey Konovalov, Andrew Morton,
	LKML, Linux Memory Management List, kasan-dev

On Tue, 21 May 2019 at 17:53, Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, May 21, 2019 at 5:43 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> >
> > On 5/20/19 6:47 PM, Marco Elver wrote:
> >
> > > +static void print_decoded_frame_descr(const char *frame_descr)
> > > +{
> > > +     /*
> > > +      * We need to parse the following string:
> > > +      *    "n alloc_1 alloc_2 ... alloc_n"
> > > +      * where alloc_i looks like
> > > +      *    "offset size len name"
> > > +      * or "offset size len name:line".
> > > +      */
> > > +
> > > +     char token[64];
> > > +     unsigned long num_objects;
> > > +
> > > +     if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                               &num_objects))
> > > +             return;
> > > +
> > > +     pr_err("\n");
> > > +     pr_err("this frame has %lu %s:\n", num_objects,
> > > +            num_objects == 1 ? "object" : "objects");
> > > +
> > > +     while (num_objects--) {
> > > +             unsigned long offset;
> > > +             unsigned long size;
> > > +
> > > +             /* access offset */
> > > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                                       &offset))
> > > +                     return;
> > > +             /* access size */
> > > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                                       &size))
> > > +                     return;
> > > +             /* name length (unused) */
> > > +             if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
> > > +                     return;
> > > +             /* object name */
> > > +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
> > > +                                       NULL))
> > > +                     return;
> > > +
> > > +             /* Strip line number, if it exists. */
> >
> >    Why?

The filename is not included, and I don't think it adds much in terms
of ability to debug; nor is the line number included with all
descriptions. I think, the added complexity of separating the line
number and parsing is not worthwhile here. Alternatively, I could not
pay attention to the line number at all, and leave it as is -- in that
case, some variable names will display as "foo:123".

> >
> > > +             strreplace(token, ':', '\0');
> > > +
> >
> > ...
> >
> > > +
> > > +     aligned_addr = round_down((unsigned long)addr, sizeof(long));
> > > +     mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
> > > +     shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
> > > +     shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
> > > +
> > > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
> > > +             shadow_ptr--;
> > > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > > +     }
> > > +
> > > +     while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
> > > +             shadow_ptr--;
> > > +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
> > > +     }
> > > +
> >
> > I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch.
> > But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
> Note that KASAN was broken on parisc from day 1 because of other
> assumptions on the stack growth direction hardcoded into KASAN
> (e.g. __kasan_unpoison_stack() and __asan_allocas_unpoison()).
> So maybe this BUILD_BUG_ON can be added in a separate patch as it's
> not specific to what Marco is doing here?

Happy to send a follow-up patch, or add here. Let me know what you prefer.

Thanks,
-- Marco


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/kasan: Print frame description for stack bugs
  2019-05-21 16:07       ` Marco Elver
  (?)
@ 2019-05-21 18:07       ` Andrey Ryabinin
  -1 siblings, 0 replies; 8+ messages in thread
From: Andrey Ryabinin @ 2019-05-21 18:07 UTC (permalink / raw)
  To: Marco Elver, Alexander Potapenko
  Cc: Dmitriy Vyukov, Andrey Konovalov, Andrew Morton, LKML,
	Linux Memory Management List, kasan-dev



On 5/21/19 7:07 PM, Marco Elver wrote:
> On Tue, 21 May 2019 at 17:53, Alexander Potapenko <glider@google.com> wrote:
>>
>> On Tue, May 21, 2019 at 5:43 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>>
>>> On 5/20/19 6:47 PM, Marco Elver wrote:
>>>
>>>> +static void print_decoded_frame_descr(const char *frame_descr)
>>>> +{
>>>> +     /*
>>>> +      * We need to parse the following string:
>>>> +      *    "n alloc_1 alloc_2 ... alloc_n"
>>>> +      * where alloc_i looks like
>>>> +      *    "offset size len name"
>>>> +      * or "offset size len name:line".
>>>> +      */
>>>> +
>>>> +     char token[64];
>>>> +     unsigned long num_objects;
>>>> +
>>>> +     if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
>>>> +                               &num_objects))
>>>> +             return;
>>>> +
>>>> +     pr_err("\n");
>>>> +     pr_err("this frame has %lu %s:\n", num_objects,
>>>> +            num_objects == 1 ? "object" : "objects");
>>>> +
>>>> +     while (num_objects--) {
>>>> +             unsigned long offset;
>>>> +             unsigned long size;
>>>> +
>>>> +             /* access offset */
>>>> +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
>>>> +                                       &offset))
>>>> +                     return;
>>>> +             /* access size */
>>>> +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
>>>> +                                       &size))
>>>> +                     return;
>>>> +             /* name length (unused) */
>>>> +             if (!tokenize_frame_descr(&frame_descr, NULL, 0, NULL))
>>>> +                     return;
>>>> +             /* object name */
>>>> +             if (!tokenize_frame_descr(&frame_descr, token, sizeof(token),
>>>> +                                       NULL))
>>>> +                     return;
>>>> +
>>>> +             /* Strip line number, if it exists. */
>>>
>>>    Why?
> 
> The filename is not included, and I don't think it adds much in terms
> of ability to debug; nor is the line number included with all
> descriptions. I think, the added complexity of separating the line
> number and parsing is not worthwhile here. Alternatively, I could not
> pay attention to the line number at all, and leave it as is -- in that
> case, some variable names will display as "foo:123".
> 

Either way is fine by me. But explain why in comment if you decide
to keep current code.  Something like
	 /* Strip line number cause it's not very helpful. */


>>>
>>>> +             strreplace(token, ':', '\0');
>>>> +
>>>
>>> ...
>>>
>>>> +
>>>> +     aligned_addr = round_down((unsigned long)addr, sizeof(long));
>>>> +     mem_ptr = round_down(aligned_addr, KASAN_SHADOW_SCALE_SIZE);
>>>> +     shadow_ptr = kasan_mem_to_shadow((void *)aligned_addr);
>>>> +     shadow_bottom = kasan_mem_to_shadow(end_of_stack(current));
>>>> +
>>>> +     while (shadow_ptr >= shadow_bottom && *shadow_ptr != KASAN_STACK_LEFT) {
>>>> +             shadow_ptr--;
>>>> +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
>>>> +     }
>>>> +
>>>> +     while (shadow_ptr >= shadow_bottom && *shadow_ptr == KASAN_STACK_LEFT) {
>>>> +             shadow_ptr--;
>>>> +             mem_ptr -= KASAN_SHADOW_SCALE_SIZE;
>>>> +     }
>>>> +
>>>
>>> I suppose this won't work if stack grows up, which is fine because it grows up only on parisc arch.
>>> But "BUILD_BUG_ON(IS_ENABLED(CONFIG_STACK_GROUWSUP))" somewhere wouldn't hurt.
>> Note that KASAN was broken on parisc from day 1 because of other
>> assumptions on the stack growth direction hardcoded into KASAN
>> (e.g. __kasan_unpoison_stack() and __asan_allocas_unpoison()).

It's not broken, it doesn't exist.

>> So maybe this BUILD_BUG_ON can be added in a separate patch as it's
>> not specific to what Marco is doing here?
> 

I think it's fine to add it in this patch because BUILD_BUG_ON() is just a hint for developers
that this particular function depends on growing down stack. So it's more a property of the function
rather than KASAN in general.

Other functions you mentioned can be marked with BUILD_BUG_ON()s as well, but not in this patch indeed.

> Happy to send a follow-up patch, or add here. Let me know what you prefer.
> 

Send v3 please.

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-21 18:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-20 15:47 [PATCH v2] mm/kasan: Print frame description for stack bugs Marco Elver
2019-05-20 15:47 ` Marco Elver
2019-05-21 15:43 ` Andrey Ryabinin
2019-05-21 15:52   ` Alexander Potapenko
2019-05-21 15:52     ` Alexander Potapenko
2019-05-21 16:07     ` Marco Elver
2019-05-21 16:07       ` Marco Elver
2019-05-21 18:07       ` Andrey Ryabinin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.