Disable kasan after the first report. There are several reasons for this:
* Single bug quite often has multiple invalid memory accesses causing
storm in the dmesg.
* Write OOB access might corrupt metadata so the next report will print
bogus alloc/free stacktraces.
* Reports after the first easily could be not bugs by itself but just side
effects of the first one.
Given that multiple reports only do harm, it makes sense to disable
kasan after the first one. Except for the tests in lib/test_kasan.c
as we obviously want to see all reports from test.
Signed-off-by: Andrey Ryabinin <[email protected]>
---
lib/test_kasan.c | 9 +++++++++
mm/kasan/report.c | 7 +++++++
2 files changed, 16 insertions(+)
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 0b1d314..5112663 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -11,6 +11,7 @@
#define pr_fmt(fmt) "kasan test: %s " fmt, __func__
+#include <linux/atomic.h>
#include <linux/delay.h>
#include <linux/kernel.h>
#include <linux/mman.h>
@@ -21,6 +22,8 @@
#include <linux/uaccess.h>
#include <linux/module.h>
+extern atomic_t kasan_report_count;
+
/*
* Note: test functions are marked noinline so that their names appear in
* reports.
@@ -474,6 +477,9 @@ static noinline void __init use_after_scope_test(void)
static int __init kmalloc_tests_init(void)
{
+ /* Rise reports limit high enough to see all the following bugs */
+ atomic_set(&kasan_report_count, 100);
+
kmalloc_oob_right();
kmalloc_oob_left();
kmalloc_node_oob_right();
@@ -499,6 +505,9 @@ static int __init kmalloc_tests_init(void)
ksize_unpoisons_memory();
copy_user_test();
use_after_scope_test();
+
+ /* kasan is unreliable now, disable reports */
+ atomic_set(&kasan_report_count, 0);
return -EAGAIN;
}
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 718a10a..7eab229 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -13,6 +13,7 @@
*
*/
+#include <linux/atomic.h>
#include <linux/ftrace.h>
#include <linux/kernel.h>
#include <linux/mm.h>
@@ -354,6 +355,9 @@ static void kasan_report_error(struct kasan_access_info *info)
kasan_end_report(&flags);
}
+atomic_t kasan_report_count = ATOMIC_INIT(1);
+EXPORT_SYMBOL_GPL(kasan_report_count);
+
void kasan_report(unsigned long addr, size_t size,
bool is_write, unsigned long ip)
{
@@ -362,6 +366,9 @@ void kasan_report(unsigned long addr, size_t size,
if (likely(!kasan_report_enabled()))
return;
+ if (atomic_dec_if_positive(&kasan_report_count) < 0)
+ return;
+
disable_trace_on_warning();
info.access_addr = (void *)addr;
--
2.10.2
On Wed, Mar 22, 2017 at 5:06 PM, Andrey Ryabinin
<[email protected]> wrote:
> Disable kasan after the first report. There are several reasons for this:
> * Single bug quite often has multiple invalid memory accesses causing
> storm in the dmesg.
> * Write OOB access might corrupt metadata so the next report will print
> bogus alloc/free stacktraces.
> * Reports after the first easily could be not bugs by itself but just side
> effects of the first one.
>
> Given that multiple reports only do harm, it makes sense to disable
> kasan after the first one. Except for the tests in lib/test_kasan.c
> as we obviously want to see all reports from test.
Hi Andrey,
Could you make it configurable via CONFIG_KASAN_SOMETHING (which can
default to showing only the first report)?
I sometimes use KASAN to see what bad accesses a particular bug
causes, and seeing all of them (even knowing that they may be
corrupt/induced) helps a lot.
Thanks!
>
> Signed-off-by: Andrey Ryabinin <[email protected]>
> ---
> lib/test_kasan.c | 9 +++++++++
> mm/kasan/report.c | 7 +++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 0b1d314..5112663 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -11,6 +11,7 @@
>
> #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
>
> +#include <linux/atomic.h>
> #include <linux/delay.h>
> #include <linux/kernel.h>
> #include <linux/mman.h>
> @@ -21,6 +22,8 @@
> #include <linux/uaccess.h>
> #include <linux/module.h>
>
> +extern atomic_t kasan_report_count;
> +
> /*
> * Note: test functions are marked noinline so that their names appear in
> * reports.
> @@ -474,6 +477,9 @@ static noinline void __init use_after_scope_test(void)
>
> static int __init kmalloc_tests_init(void)
> {
> + /* Rise reports limit high enough to see all the following bugs */
> + atomic_set(&kasan_report_count, 100);
> +
> kmalloc_oob_right();
> kmalloc_oob_left();
> kmalloc_node_oob_right();
> @@ -499,6 +505,9 @@ static int __init kmalloc_tests_init(void)
> ksize_unpoisons_memory();
> copy_user_test();
> use_after_scope_test();
> +
> + /* kasan is unreliable now, disable reports */
> + atomic_set(&kasan_report_count, 0);
> return -EAGAIN;
> }
>
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 718a10a..7eab229 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -13,6 +13,7 @@
> *
> */
>
> +#include <linux/atomic.h>
> #include <linux/ftrace.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> @@ -354,6 +355,9 @@ static void kasan_report_error(struct kasan_access_info *info)
> kasan_end_report(&flags);
> }
>
> +atomic_t kasan_report_count = ATOMIC_INIT(1);
> +EXPORT_SYMBOL_GPL(kasan_report_count);
> +
> void kasan_report(unsigned long addr, size_t size,
> bool is_write, unsigned long ip)
> {
> @@ -362,6 +366,9 @@ void kasan_report(unsigned long addr, size_t size,
> if (likely(!kasan_report_enabled()))
> return;
>
> + if (atomic_dec_if_positive(&kasan_report_count) < 0)
> + return;
> +
> disable_trace_on_warning();
>
> info.access_addr = (void *)addr;
> --
> 2.10.2
>
> --
> 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 [email protected].
> To post to this group, send email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20170322160647.32032-1-aryabinin%40virtuozzo.com.
> For more options, visit https://groups.google.com/d/optout.
On Wed, Mar 22, 2017 at 5:54 PM, Andrey Ryabinin
<[email protected]> wrote:
> On 03/22/2017 07:34 PM, Andrey Konovalov wrote:
>> On Wed, Mar 22, 2017 at 5:06 PM, Andrey Ryabinin
>> <[email protected]> wrote:
>>> Disable kasan after the first report. There are several reasons for this:
>>> * Single bug quite often has multiple invalid memory accesses causing
>>> storm in the dmesg.
>>> * Write OOB access might corrupt metadata so the next report will print
>>> bogus alloc/free stacktraces.
>>> * Reports after the first easily could be not bugs by itself but just side
>>> effects of the first one.
>>>
>>> Given that multiple reports only do harm, it makes sense to disable
>>> kasan after the first one. Except for the tests in lib/test_kasan.c
>>> as we obviously want to see all reports from test.
>>
>> Hi Andrey,
>>
>> Could you make it configurable via CONFIG_KASAN_SOMETHING (which can
>> default to showing only the first report)?
>
> I'd rather make this boot time configurable, but wouldn't want to without
> a good reason.
That would work for me.
>
>
>> I sometimes use KASAN to see what bad accesses a particular bug
>> causes, and seeing all of them (even knowing that they may be
>> corrupt/induced) helps a lot.
>
> I'm wondering why you need to see all reports?
To get a better picture of what are the consequences of a bug. For
example whether it leads to some bad or controllable memory
corruption. Sometimes it's easier to let KASAN track the memory
accesses then do that manually.
>
>>
>> Thanks!
>>
On 03/22/2017 07:34 PM, Andrey Konovalov wrote:
> On Wed, Mar 22, 2017 at 5:06 PM, Andrey Ryabinin
> <[email protected]> wrote:
>> Disable kasan after the first report. There are several reasons for this:
>> * Single bug quite often has multiple invalid memory accesses causing
>> storm in the dmesg.
>> * Write OOB access might corrupt metadata so the next report will print
>> bogus alloc/free stacktraces.
>> * Reports after the first easily could be not bugs by itself but just side
>> effects of the first one.
>>
>> Given that multiple reports only do harm, it makes sense to disable
>> kasan after the first one. Except for the tests in lib/test_kasan.c
>> as we obviously want to see all reports from test.
>
> Hi Andrey,
>
> Could you make it configurable via CONFIG_KASAN_SOMETHING (which can
> default to showing only the first report)?
I'd rather make this boot time configurable, but wouldn't want to without
a good reason.
> I sometimes use KASAN to see what bad accesses a particular bug
> causes, and seeing all of them (even knowing that they may be
> corrupt/induced) helps a lot.
I'm wondering why you need to see all reports?
>
> Thanks!
>
On Wed, Mar 22, 2017 at 6:07 PM, Andrey Konovalov <[email protected]> wrote:
> On Wed, Mar 22, 2017 at 5:54 PM, Andrey Ryabinin
> <[email protected]> wrote:
>> On 03/22/2017 07:34 PM, Andrey Konovalov wrote:
>>> On Wed, Mar 22, 2017 at 5:06 PM, Andrey Ryabinin
>>> <[email protected]> wrote:
>>>> Disable kasan after the first report. There are several reasons for this:
>>>> * Single bug quite often has multiple invalid memory accesses causing
>>>> storm in the dmesg.
>>>> * Write OOB access might corrupt metadata so the next report will print
>>>> bogus alloc/free stacktraces.
>>>> * Reports after the first easily could be not bugs by itself but just side
>>>> effects of the first one.
>>>>
>>>> Given that multiple reports only do harm, it makes sense to disable
>>>> kasan after the first one. Except for the tests in lib/test_kasan.c
>>>> as we obviously want to see all reports from test.
>>>
>>> Hi Andrey,
>>>
>>> Could you make it configurable via CONFIG_KASAN_SOMETHING (which can
>>> default to showing only the first report)?
>>
>> I'd rather make this boot time configurable, but wouldn't want to without
>> a good reason.
>
> That would work for me.
>
>>
>>
>>> I sometimes use KASAN to see what bad accesses a particular bug
>>> causes, and seeing all of them (even knowing that they may be
>>> corrupt/induced) helps a lot.
>>
>> I'm wondering why you need to see all reports?
>
> To get a better picture of what are the consequences of a bug. For
> example whether it leads to some bad or controllable memory
> corruption. Sometimes it's easier to let KASAN track the memory
> accesses then do that manually.
Another case is when you're seeing an OOB read at boot time, which has
limited impact, and you don't want to wait for the code owner to fix
it to move forward.
>>
>>>
>>> Thanks!
>>>
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
On Wed, Mar 22, 2017 at 6:33 PM, Alexander Potapenko <[email protected]> wrote:
> On Wed, Mar 22, 2017 at 6:07 PM, Andrey Konovalov <[email protected]> wrote:
>> On Wed, Mar 22, 2017 at 5:54 PM, Andrey Ryabinin
>> <[email protected]> wrote:
>>> On 03/22/2017 07:34 PM, Andrey Konovalov wrote:
>>>> On Wed, Mar 22, 2017 at 5:06 PM, Andrey Ryabinin
>>>> <[email protected]> wrote:
>>>>> Disable kasan after the first report. There are several reasons for this:
>>>>> * Single bug quite often has multiple invalid memory accesses causing
>>>>> storm in the dmesg.
>>>>> * Write OOB access might corrupt metadata so the next report will print
>>>>> bogus alloc/free stacktraces.
>>>>> * Reports after the first easily could be not bugs by itself but just side
>>>>> effects of the first one.
>>>>>
>>>>> Given that multiple reports only do harm, it makes sense to disable
>>>>> kasan after the first one. Except for the tests in lib/test_kasan.c
>>>>> as we obviously want to see all reports from test.
>>>>
>>>> Hi Andrey,
>>>>
>>>> Could you make it configurable via CONFIG_KASAN_SOMETHING (which can
>>>> default to showing only the first report)?
>>>
>>> I'd rather make this boot time configurable, but wouldn't want to without
>>> a good reason.
>>
>> That would work for me.
Also note that KASAN now supports panic_on_warn=1, which achieves more
or less the same. Of course, WARNINGs may be not that bad, but KASAN
reports may be not tool bad as well (e.g. off-by-one reads).
>>>> I sometimes use KASAN to see what bad accesses a particular bug
>>>> causes, and seeing all of them (even knowing that they may be
>>>> corrupt/induced) helps a lot.
>>>
>>> I'm wondering why you need to see all reports?
>>
>> To get a better picture of what are the consequences of a bug. For
>> example whether it leads to some bad or controllable memory
>> corruption. Sometimes it's easier to let KASAN track the memory
>> accesses then do that manually.
> Another case is when you're seeing an OOB read at boot time, which has
> limited impact, and you don't want to wait for the code owner to fix
> it to move forward.
>>>
>>>>
>>>> Thanks!
>>>>
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
Disable kasan after the first report. There are several reasons for this:
* Single bug quite often has multiple invalid memory accesses causing
storm in the dmesg.
* Write OOB access might corrupt metadata so the next report will print
bogus alloc/free stacktraces.
* Reports after the first easily could be not bugs by itself but just side
effects of the first one.
Given that multiple reports usually only do harm, it makes sense to disable
kasan after the first one. If user wants to see all the reports, the
boot-time parameter kasan_multi_shot must be used.
Signed-off-by: Andrey Ryabinin <[email protected]>
---
Changes since v1:
- provide kasan_multi_shot boot parameter.
Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
lib/test_kasan.c | 12 ++++++++++++
mm/kasan/kasan.h | 5 -----
mm/kasan/report.c | 18 ++++++++++++++++++
4 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2906987..f88d60e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1726,6 +1726,12 @@
kernel and module base offset ASLR (Address Space
Layout Randomization).
+ 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.
+
keepinitrd [HW,ARM]
kernelcore= [KNL,X86,IA-64,PPC]
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 0b1d314..f3acece 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -11,6 +11,7 @@
#define pr_fmt(fmt) "kasan test: %s " fmt, __func__
+#include <linux/atomic.h>
#include <linux/delay.h>
#include <linux/kernel.h>
#include <linux/mman.h>
@@ -21,6 +22,8 @@
#include <linux/uaccess.h>
#include <linux/module.h>
+extern atomic_t kasan_report_count;
+
/*
* Note: test functions are marked noinline so that their names appear in
* reports.
@@ -474,6 +477,9 @@ static noinline void __init use_after_scope_test(void)
static int __init kmalloc_tests_init(void)
{
+ /* Rise reports limit high enough to see all the following bugs */
+ atomic_add(100, &kasan_report_count);
+
kmalloc_oob_right();
kmalloc_oob_left();
kmalloc_node_oob_right();
@@ -499,6 +505,12 @@ static int __init kmalloc_tests_init(void)
ksize_unpoisons_memory();
copy_user_test();
use_after_scope_test();
+
+ /*
+ * kasan is unreliable now, disable reports if
+ * we are in single shot mode
+ */
+ atomic_sub(100, &kasan_report_count);
return -EAGAIN;
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7572917..1229298 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 718a10a..5650534 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -13,7 +13,9 @@
*
*/
+#include <linux/atomic.h>
#include <linux/ftrace.h>
+#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/printk.h>
@@ -354,6 +356,22 @@ static void kasan_report_error(struct kasan_access_info *info)
kasan_end_report(&flags);
}
+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);
+
+static inline bool kasan_report_enabled(void)
+{
+ return !current->kasan_depth &&
+ (atomic_dec_if_positive(&kasan_report_count) >= 0);
+}
+
void kasan_report(unsigned long addr, size_t size,
bool is_write, unsigned long ip)
{
--
2.10.2
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)
{
On 03/23/2017 03:41 PM, Mark Rutland wrote:
> 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?
Yes, it deferentially looks better.
Can you send a patch with a changelog, or do you want me to care of it?
> Thanks,
> Mark.
>
> 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>
We also need <linux/init.h> for __setup().
> #include <linux/kernel.h>
> #include <linux/mm.h>
> @@ -293,6 +294,40 @@ static void kasan_report_error(struct kasan_access_info *info)
On Thu, Mar 23, 2017 at 04:06:59PM +0300, Andrey Ryabinin wrote:
> On 03/23/2017 03:41 PM, Mark Rutland wrote:
> > 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?
>
> Yes, it deferentially looks better.
> Can you send a patch with a changelog, or do you want me to care of it?
Would you be happy to take care of it, along with the fixup you
suggested below, as v3?
You can add my:
Signed-off-by: Mark Rutland <[email protected]>
Thanks,
Mark.
> >
> > +#include <linux/bitops.h>
> > #include <linux/ftrace.h>
>
> We also need <linux/init.h> for __setup().
>
> > #include <linux/kernel.h>
> > #include <linux/mm.h>
> > @@ -293,6 +294,40 @@ static void kasan_report_error(struct kasan_access_info *info)
From: Mark Rutland <[email protected]>
Disable kasan after the first report. There are several reasons for this:
* Single bug quite often has multiple invalid memory accesses causing
storm in the dmesg.
* Write OOB access might corrupt metadata so the next report will print
bogus alloc/free stacktraces.
* Reports after the first easily could be not bugs by itself but just side
effects of the first one.
Given that multiple reports usually only do harm, it makes sense to disable
kasan after the first one. If user wants to see all the reports, the
boot-time parameter kasan_multi_shot must be used.
Signed-off-by: Mark Rutland <[email protected]>
[ aryabinin: wrote changelog and doc, added missing include ]
Signed-off-by: Andrey Ryabinin <[email protected]>
---
Changes since v2:
- Instead of using atomic report counter, use separate flags to determine
whether we're in multi-shot mode, and whether a (oneshot) report
has been made.
Changes since v1:
- provide kasan_multi_shot boot parameter.
Documentation/admin-guide/kernel-parameters.txt | 6 +++++
include/linux/kasan.h | 3 +++
lib/test_kasan.c | 10 +++++++
mm/kasan/kasan.h | 5 ----
mm/kasan/report.c | 36 +++++++++++++++++++++++++
5 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2906987..f88d60e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1726,6 +1726,12 @@
kernel and module base offset ASLR (Address Space
Layout Randomization).
+ 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.
+
keepinitrd [HW,ARM]
kernelcore= [KNL,X86,IA-64,PPC]
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 5734480c9..a5c7046 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -76,6 +76,9 @@ size_t ksize(const void *);
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..a25c976 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -20,6 +20,7 @@
#include <linux/string.h>
#include <linux/uaccess.h>
#include <linux/module.h>
+#include <linux/kasan.h>
/*
* Note: test functions are marked noinline so that their names appear in
@@ -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 7572917..1229298 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 718a10a..beee0e9 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -13,7 +13,9 @@
*
*/
+#include <linux/bitops.h>
#include <linux/ftrace.h>
+#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/printk.h>
@@ -354,6 +356,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)
{
--
2.10.2