All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andrey Konovalov <andreyknvl@google.com>,
	Alexander Potapenko <glider@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	kasan-dev@googlegroups.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kasan: report only the first error by default
Date: Thu, 23 Mar 2017 12:41:54 +0000	[thread overview]
Message-ID: <20170323124154.GE9287@leverpostej> (raw)
In-Reply-To: <20170323114916.29871-1-aryabinin@virtuozzo.com>

On Thu, Mar 23, 2017 at 02:49:16PM +0300, Andrey Ryabinin wrote:
> +	kasan_multi_shot
> +			[KNL] Enforce KASAN (Kernel Address Sanitizer) to print
> +			report on every invalid memory access. Without this
> +			parameter KASAN will print report only for the first
> +			invalid access.
> +

The option looks fine to me.

>  static int __init kmalloc_tests_init(void)
>  {
> +	/* Rise reports limit high enough to see all the following bugs */
> +	atomic_add(100, &kasan_report_count);

> +
> +	/*
> +	 * kasan is unreliable now, disable reports if
> +	 * we are in single shot mode
> +	 */
> +	atomic_sub(100, &kasan_report_count);
>  	return -EAGAIN;
>  }

... but these magic numbers look rather messy.

[...]

> +atomic_t kasan_report_count = ATOMIC_INIT(1);
> +EXPORT_SYMBOL_GPL(kasan_report_count);
> +
> +static int __init kasan_set_multi_shot(char *str)
> +{
> +	atomic_set(&kasan_report_count, 1000000000);
> +	return 1;
> +}
> +__setup("kasan_multi_shot", kasan_set_multi_shot);

... likewise.

Rather than trying to pick an arbitrarily large number, how about we use
separate flags to determine whether we're in multi-shot mode, and
whether a (oneshot) report has been made.

How about the below?

Thanks,
Mark.

---->8----
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index ceb3fe7..291d7b3 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -74,6 +74,9 @@ struct kasan_cache {
 static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
 size_t kasan_metadata_size(struct kmem_cache *cache);
 
+bool kasan_save_enable_multi_shot(void);
+void kasan_restore_multi_shot(bool enabled);
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 0b1d314..ae59dc2 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -12,6 +12,7 @@
 #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
 
 #include <linux/delay.h>
+#include <linux/kasan.h>
 #include <linux/kernel.h>
 #include <linux/mman.h>
 #include <linux/mm.h>
@@ -474,6 +475,12 @@ static noinline void __init use_after_scope_test(void)
 
 static int __init kmalloc_tests_init(void)
 {
+	/*
+	 * Temporarily enable multi-shot mode. Otherwise, we'd only get a
+	 * report for the first case.
+	 */
+	bool multishot = kasan_save_enable_multi_shot();
+
 	kmalloc_oob_right();
 	kmalloc_oob_left();
 	kmalloc_node_oob_right();
@@ -499,6 +506,9 @@ static int __init kmalloc_tests_init(void)
 	ksize_unpoisons_memory();
 	copy_user_test();
 	use_after_scope_test();
+
+	kasan_restore_multi_shot(multishot);
+
 	return -EAGAIN;
 }
 
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 1c260e6..dd2dea8 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -96,11 +96,6 @@ static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
 		<< KASAN_SHADOW_SCALE_SHIFT);
 }
 
-static inline bool kasan_report_enabled(void)
-{
-	return !current->kasan_depth;
-}
-
 void kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 void kasan_report_double_free(struct kmem_cache *cache, void *object,
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index f479365..f1c5892 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -13,6 +13,7 @@
  *
  */
 
+#include <linux/bitops.h>
 #include <linux/ftrace.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
@@ -293,6 +294,40 @@ static void kasan_report_error(struct kasan_access_info *info)
 	kasan_end_report(&flags);
 }
 
+static unsigned long kasan_flags;
+
+#define KASAN_BIT_REPORTED	0
+#define KASAN_BIT_MULTI_SHOT	1
+
+bool kasan_save_enable_multi_shot(void)
+{
+	return test_and_set_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
+}
+EXPORT_SYMBOL_GPL(kasan_save_enable_multi_shot);
+
+void kasan_restore_multi_shot(bool enabled)
+{
+	if (!enabled)
+		clear_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
+}
+EXPORT_SYMBOL_GPL(kasan_restore_multi_shot);
+
+static int __init kasan_set_multi_shot(char *str)
+{
+	set_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
+	return 1;
+}
+__setup("kasan_multi_shot", kasan_set_multi_shot);
+
+static inline bool kasan_report_enabled(void)
+{
+	if (current->kasan_depth)
+		return false;
+	if (test_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags))
+		return true;
+	return !test_and_set_bit(KASAN_BIT_REPORTED, &kasan_flags);
+}
+
 void kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip)
 {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andrey Konovalov <andreyknvl@google.com>,
	Alexander Potapenko <glider@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	kasan-dev@googlegroups.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kasan: report only the first error by default
Date: Thu, 23 Mar 2017 12:41:54 +0000	[thread overview]
Message-ID: <20170323124154.GE9287@leverpostej> (raw)
In-Reply-To: <20170323114916.29871-1-aryabinin@virtuozzo.com>

On Thu, Mar 23, 2017 at 02:49:16PM +0300, Andrey Ryabinin wrote:
> +	kasan_multi_shot
> +			[KNL] Enforce KASAN (Kernel Address Sanitizer) to print
> +			report on every invalid memory access. Without this
> +			parameter KASAN will print report only for the first
> +			invalid access.
> +

The option looks fine to me.

>  static int __init kmalloc_tests_init(void)
>  {
> +	/* Rise reports limit high enough to see all the following bugs */
> +	atomic_add(100, &kasan_report_count);

> +
> +	/*
> +	 * kasan is unreliable now, disable reports if
> +	 * we are in single shot mode
> +	 */
> +	atomic_sub(100, &kasan_report_count);
>  	return -EAGAIN;
>  }

... but these magic numbers look rather messy.

[...]

> +atomic_t kasan_report_count = ATOMIC_INIT(1);
> +EXPORT_SYMBOL_GPL(kasan_report_count);
> +
> +static int __init kasan_set_multi_shot(char *str)
> +{
> +	atomic_set(&kasan_report_count, 1000000000);
> +	return 1;
> +}
> +__setup("kasan_multi_shot", kasan_set_multi_shot);

... likewise.

Rather than trying to pick an arbitrarily large number, how about we use
separate flags to determine whether we're in multi-shot mode, and
whether a (oneshot) report has been made.

How about the below?

Thanks,
Mark.

---->8----
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index ceb3fe7..291d7b3 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -74,6 +74,9 @@ struct kasan_cache {
 static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
 size_t kasan_metadata_size(struct kmem_cache *cache);
 
+bool kasan_save_enable_multi_shot(void);
+void kasan_restore_multi_shot(bool enabled);
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 0b1d314..ae59dc2 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -12,6 +12,7 @@
 #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
 
 #include <linux/delay.h>
+#include <linux/kasan.h>
 #include <linux/kernel.h>
 #include <linux/mman.h>
 #include <linux/mm.h>
@@ -474,6 +475,12 @@ static noinline void __init use_after_scope_test(void)
 
 static int __init kmalloc_tests_init(void)
 {
+	/*
+	 * Temporarily enable multi-shot mode. Otherwise, we'd only get a
+	 * report for the first case.
+	 */
+	bool multishot = kasan_save_enable_multi_shot();
+
 	kmalloc_oob_right();
 	kmalloc_oob_left();
 	kmalloc_node_oob_right();
@@ -499,6 +506,9 @@ static int __init kmalloc_tests_init(void)
 	ksize_unpoisons_memory();
 	copy_user_test();
 	use_after_scope_test();
+
+	kasan_restore_multi_shot(multishot);
+
 	return -EAGAIN;
 }
 
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 1c260e6..dd2dea8 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -96,11 +96,6 @@ static inline const void *kasan_shadow_to_mem(const void *shadow_addr)
 		<< KASAN_SHADOW_SCALE_SHIFT);
 }
 
-static inline bool kasan_report_enabled(void)
-{
-	return !current->kasan_depth;
-}
-
 void kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 void kasan_report_double_free(struct kmem_cache *cache, void *object,
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index f479365..f1c5892 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -13,6 +13,7 @@
  *
  */
 
+#include <linux/bitops.h>
 #include <linux/ftrace.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
@@ -293,6 +294,40 @@ static void kasan_report_error(struct kasan_access_info *info)
 	kasan_end_report(&flags);
 }
 
+static unsigned long kasan_flags;
+
+#define KASAN_BIT_REPORTED	0
+#define KASAN_BIT_MULTI_SHOT	1
+
+bool kasan_save_enable_multi_shot(void)
+{
+	return test_and_set_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
+}
+EXPORT_SYMBOL_GPL(kasan_save_enable_multi_shot);
+
+void kasan_restore_multi_shot(bool enabled)
+{
+	if (!enabled)
+		clear_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
+}
+EXPORT_SYMBOL_GPL(kasan_restore_multi_shot);
+
+static int __init kasan_set_multi_shot(char *str)
+{
+	set_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
+	return 1;
+}
+__setup("kasan_multi_shot", kasan_set_multi_shot);
+
+static inline bool kasan_report_enabled(void)
+{
+	if (current->kasan_depth)
+		return false;
+	if (test_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags))
+		return true;
+	return !test_and_set_bit(KASAN_BIT_REPORTED, &kasan_flags);
+}
+
 void kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip)
 {

  reply	other threads:[~2017-03-23 12:42 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-22 16:06 [PATCH] kasan: report only the first error Andrey Ryabinin
2017-03-22 16:06 ` Andrey Ryabinin
2017-03-22 16:34 ` Andrey Konovalov
2017-03-22 16:34   ` Andrey Konovalov
2017-03-22 16:54   ` Andrey Ryabinin
2017-03-22 16:54     ` Andrey Ryabinin
2017-03-22 17:07     ` Andrey Konovalov
2017-03-22 17:07       ` Andrey Konovalov
2017-03-22 17:33       ` Alexander Potapenko
2017-03-22 17:33         ` Alexander Potapenko
2017-03-22 17:42         ` Dmitry Vyukov
2017-03-22 17:42           ` Dmitry Vyukov
2017-03-23 11:49 ` [PATCH v2] kasan: report only the first error by default Andrey Ryabinin
2017-03-23 11:49   ` Andrey Ryabinin
2017-03-23 12:41   ` Mark Rutland [this message]
2017-03-23 12:41     ` Mark Rutland
2017-03-23 13:06     ` Andrey Ryabinin
2017-03-23 13:06       ` Andrey Ryabinin
2017-03-23 13:29       ` Mark Rutland
2017-03-23 13:29         ` Mark Rutland
2017-03-23 15:44 ` [PATCH v3] " Andrey Ryabinin
2017-03-23 15:44   ` Andrey Ryabinin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170323124154.GE9287@leverpostej \
    --to=mark.rutland@arm.com \
    --cc=akpm@linux-foundation.org \
    --cc=andreyknvl@google.com \
    --cc=aryabinin@virtuozzo.com \
    --cc=dvyukov@google.com \
    --cc=glider@google.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.