When call_rcu() is called, we store the call_rcu() call stack into
slub alloc meta-data, so that KASAN report prints call_rcu() information.
We add new KASAN_RCU_STACK_RECORD configuration option. It will record
first and last call_rcu() call stack and KASAN report will print two
call_rcu() call stack.
This option doesn't increase the cost of memory consumption. Because
we don't enlarge struct kasan_alloc_meta size.
- add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
- remove free track from kasan_alloc_meta, size is 8 bytes.
[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
Signed-off-by: Walter Wu <[email protected]>
Suggested-by: Dmitry Vyukov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Alexander Potapenko <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Joel Fernandes <[email protected]>
---
include/linux/kasan.h | 7 +++++++
kernel/rcu/tree.c | 4 ++++
lib/Kconfig.kasan | 11 +++++++++++
mm/kasan/common.c | 23 +++++++++++++++++++++++
mm/kasan/kasan.h | 12 ++++++++++++
mm/kasan/report.c | 33 +++++++++++++++++++++++++++------
6 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 31314ca7c635..5eeece6893cd 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -96,6 +96,12 @@ size_t kasan_metadata_size(struct kmem_cache *cache);
bool kasan_save_enable_multi_shot(void);
void kasan_restore_multi_shot(bool enabled);
+#ifdef CONFIG_KASAN_RCU_STACK_RECORD
+void kasan_record_callrcu(void *ptr);
+#else
+static inline void kasan_record_callrcu(void *ptr) {}
+#endif
+
#else /* CONFIG_KASAN */
static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
@@ -165,6 +171,7 @@ static inline void kasan_remove_zero_shadow(void *start,
static inline void kasan_unpoison_slab(const void *ptr) { }
static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
+static inline void kasan_record_callrcu(void *ptr) {}
#endif /* CONFIG_KASAN */
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 06548e2ebb72..145c79becf7b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -57,6 +57,7 @@
#include <linux/slab.h>
#include <linux/sched/isolation.h>
#include <linux/sched/clock.h>
+#include <linux/kasan.h>
#include "../time/tick-internal.h"
#include "tree.h"
@@ -2694,6 +2695,9 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func)
trace_rcu_callback(rcu_state.name, head,
rcu_segcblist_n_cbs(&rdp->cblist));
+ if (IS_ENABLED(CONFIG_KASAN_RCU_STACK_RECORD))
+ kasan_record_callrcu(head);
+
/* Go handle any RCU core processing required. */
if (IS_ENABLED(CONFIG_RCU_NOCB_CPU) &&
unlikely(rcu_segcblist_is_offloaded(&rdp->cblist))) {
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 81f5464ea9e1..022934049cc2 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -158,6 +158,17 @@ config KASAN_VMALLOC
for KASAN to detect more sorts of errors (and to support vmapped
stacks), but at the cost of higher memory usage.
+config KASAN_RCU_STACK_RECORD
+ bool "Record and print call_rcu() call stack"
+ depends on KASAN_GENERIC
+ help
+ By default, the KASAN report doesn't print call_rcu() call stack.
+ It is very difficult to analyze memory issues(e.g., use-after-free).
+
+ Enabling this option will print first and last call_rcu() call stack.
+ It doesn't enlarge slub alloc meta-data size, so it doesn't increase
+ the cost of memory consumption.
+
config TEST_KASAN
tristate "Module for testing KASAN for bug detection"
depends on m && KASAN
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 2906358e42f0..32d422bdf127 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -299,6 +299,29 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
return (void *)object + cache->kasan_info.free_meta_offset;
}
+#ifdef CONFIG_KASAN_RCU_STACK_RECORD
+void kasan_record_callrcu(void *addr)
+{
+ struct page *page = kasan_addr_to_page(addr);
+ struct kmem_cache *cache;
+ struct kasan_alloc_meta *alloc_info;
+ void *object;
+
+ if (!(page && PageSlab(page)))
+ return;
+
+ cache = page->slab_cache;
+ object = nearest_obj(cache, page, addr);
+ alloc_info = get_alloc_info(cache, object);
+
+ if (!alloc_info->rcu_free_stack[0])
+ /* record first call_rcu() call stack */
+ alloc_info->rcu_free_stack[0] = save_stack(GFP_NOWAIT);
+ else
+ /* record last call_rcu() call stack */
+ alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
+}
+#endif
static void kasan_set_free_info(struct kmem_cache *cache,
void *object, u8 tag)
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index e8f37199d885..adc105b9cd07 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -96,15 +96,27 @@ struct kasan_track {
depot_stack_handle_t stack;
};
+#ifdef CONFIG_KASAN_RCU_STACK_RECORD
+#define BYTES_PER_WORD 4
+#define KASAN_NR_RCU_FREE_STACKS 2
+#else /* CONFIG_KASAN_RCU_STACK_RECORD */
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
#define KASAN_NR_FREE_STACKS 5
#else
#define KASAN_NR_FREE_STACKS 1
#endif
+#endif /* CONFIG_KASAN_RCU_STACK_RECORD */
struct kasan_alloc_meta {
struct kasan_track alloc_track;
+#ifdef CONFIG_KASAN_RCU_STACK_RECORD
+ /* call_rcu() call stack is stored into kasan_alloc_meta.
+ * free stack is stored into freed object.
+ */
+ depot_stack_handle_t rcu_free_stack[KASAN_NR_RCU_FREE_STACKS];
+#else
struct kasan_track free_track[KASAN_NR_FREE_STACKS];
+#endif
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
u8 free_track_idx;
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 80f23c9da6b0..7aaccc70b65b 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -105,9 +105,13 @@ static void end_report(unsigned long *flags)
kasan_enable_current();
}
-static void print_track(struct kasan_track *track, const char *prefix)
+static void print_track(struct kasan_track *track, const char *prefix,
+ bool is_callrcu)
{
- pr_err("%s by task %u:\n", prefix, track->pid);
+ if (is_callrcu)
+ pr_err("%s:\n", prefix);
+ else
+ pr_err("%s by task %u:\n", prefix, track->pid);
if (track->stack) {
unsigned long *entries;
unsigned int nr_entries;
@@ -159,8 +163,22 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
(void *)(object_addr + cache->object_size));
}
+#ifdef CONFIG_KASAN_RCU_STACK_RECORD
+static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
+{
+ struct kasan_track free_track;
+
+ free_track.stack = alloc_info->rcu_free_stack[0];
+ print_track(&free_track, "First call_rcu() call stack", true);
+ pr_err("\n");
+ free_track.stack = alloc_info->rcu_free_stack[1];
+ print_track(&free_track, "Last call_rcu() call stack", true);
+ pr_err("\n");
+}
+#endif
+
static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag)
+ void *object, u8 tag, const void *addr)
{
struct kasan_alloc_meta *alloc_meta;
int i = 0;
@@ -187,11 +205,14 @@ static void describe_object(struct kmem_cache *cache, void *object,
if (cache->flags & SLAB_KASAN) {
struct kasan_track *free_track;
- print_track(&alloc_info->alloc_track, "Allocated");
+ print_track(&alloc_info->alloc_track, "Allocated", false);
pr_err("\n");
- free_track = kasan_get_free_track(cache, object, tag);
- print_track(free_track, "Freed");
+ free_track = kasan_get_free_track(cache, object, tag, addr);
+ print_track(free_track, "Freed", false);
pr_err("\n");
+#ifdef CONFIG_KASAN_RCU_STACK_RECORD
+ kasan_print_rcu_free_stack(alloc_info);
+#endif
}
describe_object_addr(cache, object, addr);
--
2.18.0
On Wed, May 6, 2020 at 7:21 AM Walter Wu <[email protected]> wrote:
>
> When call_rcu() is called, we store the call_rcu() call stack into
> slub alloc meta-data, so that KASAN report prints call_rcu() information.
>
> We add new KASAN_RCU_STACK_RECORD configuration option. It will record
> first and last call_rcu() call stack and KASAN report will print two
> call_rcu() call stack.
>
> This option doesn't increase the cost of memory consumption. Because
> we don't enlarge struct kasan_alloc_meta size.
> - add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
> - remove free track from kasan_alloc_meta, size is 8 bytes.
>
> [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
>
> Signed-off-by: Walter Wu <[email protected]>
> Suggested-by: Dmitry Vyukov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Paul E. McKenney <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Lai Jiangshan <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> ---
> include/linux/kasan.h | 7 +++++++
> kernel/rcu/tree.c | 4 ++++
> lib/Kconfig.kasan | 11 +++++++++++
> mm/kasan/common.c | 23 +++++++++++++++++++++++
> mm/kasan/kasan.h | 12 ++++++++++++
> mm/kasan/report.c | 33 +++++++++++++++++++++++++++------
> 6 files changed, 84 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 31314ca7c635..5eeece6893cd 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -96,6 +96,12 @@ size_t kasan_metadata_size(struct kmem_cache *cache);
> bool kasan_save_enable_multi_shot(void);
> void kasan_restore_multi_shot(bool enabled);
>
> +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> +void kasan_record_callrcu(void *ptr);
The issue also mentions workqueue and timer stacks.
Have you considered supporting them as well? What was your motivation
for doing only rcu?
Looking at the first report for "workqueue use-after-free":
https://syzkaller.appspot.com/bug?extid=9cba1e478f91aad39876
This is exactly the same situation as for call_rcu, just a workqueue
is used to invoke a callback that frees the object.
If you don't want to do all at the same time, I would at least
name/branch everything inside of KASAN more generally (I think in the
issue I called it "aux" (auxiliary), or maybe something like
"additional"). But then call this kasan_record_aux_stack() only from
rcu for now. But then later we can separately decide and extend to
other callers.
It just feels wrong to have KASAN over-specialized for rcu only in this way.
And I think if the UAF is really caused by call_rcu callback, then it
sill will be recorded as last stack most of the time because rcu
callbacks are invoked relatively fast and there should not be much
else happening with the object since it's near end of life already.
> +#else
> +static inline void kasan_record_callrcu(void *ptr) {}
> +#endif
> +
> #else /* CONFIG_KASAN */
>
> static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -165,6 +171,7 @@ static inline void kasan_remove_zero_shadow(void *start,
>
> static inline void kasan_unpoison_slab(const void *ptr) { }
> static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
> +static inline void kasan_record_callrcu(void *ptr) {}
>
> #endif /* CONFIG_KASAN */
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 06548e2ebb72..145c79becf7b 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -57,6 +57,7 @@
> #include <linux/slab.h>
> #include <linux/sched/isolation.h>
> #include <linux/sched/clock.h>
> +#include <linux/kasan.h>
> #include "../time/tick-internal.h"
>
> #include "tree.h"
> @@ -2694,6 +2695,9 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func)
> trace_rcu_callback(rcu_state.name, head,
> rcu_segcblist_n_cbs(&rdp->cblist));
>
> + if (IS_ENABLED(CONFIG_KASAN_RCU_STACK_RECORD))
The if is not necessary, this function is no-op when not enabled.
> + kasan_record_callrcu(head);
> +
> /* Go handle any RCU core processing required. */
> if (IS_ENABLED(CONFIG_RCU_NOCB_CPU) &&
> unlikely(rcu_segcblist_is_offloaded(&rdp->cblist))) {
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index 81f5464ea9e1..022934049cc2 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -158,6 +158,17 @@ config KASAN_VMALLOC
> for KASAN to detect more sorts of errors (and to support vmapped
> stacks), but at the cost of higher memory usage.
>
> +config KASAN_RCU_STACK_RECORD
> + bool "Record and print call_rcu() call stack"
> + depends on KASAN_GENERIC
> + help
> + By default, the KASAN report doesn't print call_rcu() call stack.
> + It is very difficult to analyze memory issues(e.g., use-after-free).
> +
> + Enabling this option will print first and last call_rcu() call stack.
> + It doesn't enlarge slub alloc meta-data size, so it doesn't increase
> + the cost of memory consumption.
> +
> config TEST_KASAN
> tristate "Module for testing KASAN for bug detection"
> depends on m && KASAN
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 2906358e42f0..32d422bdf127 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -299,6 +299,29 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
> return (void *)object + cache->kasan_info.free_meta_offset;
> }
>
> +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> +void kasan_record_callrcu(void *addr)
> +{
> + struct page *page = kasan_addr_to_page(addr);
> + struct kmem_cache *cache;
> + struct kasan_alloc_meta *alloc_info;
> + void *object;
> +
> + if (!(page && PageSlab(page)))
> + return;
> +
> + cache = page->slab_cache;
> + object = nearest_obj(cache, page, addr);
> + alloc_info = get_alloc_info(cache, object);
> +
> + if (!alloc_info->rcu_free_stack[0])
> + /* record first call_rcu() call stack */
> + alloc_info->rcu_free_stack[0] = save_stack(GFP_NOWAIT);
> + else
> + /* record last call_rcu() call stack */
> + alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
> +}
> +#endif
>
> static void kasan_set_free_info(struct kmem_cache *cache,
> void *object, u8 tag)
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index e8f37199d885..adc105b9cd07 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -96,15 +96,27 @@ struct kasan_track {
> depot_stack_handle_t stack;
> };
>
> +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> +#define BYTES_PER_WORD 4
> +#define KASAN_NR_RCU_FREE_STACKS 2
> +#else /* CONFIG_KASAN_RCU_STACK_RECORD */
> #ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> #define KASAN_NR_FREE_STACKS 5
> #else
> #define KASAN_NR_FREE_STACKS 1
> #endif
> +#endif /* CONFIG_KASAN_RCU_STACK_RECORD */
>
> struct kasan_alloc_meta {
> struct kasan_track alloc_track;
> +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> + /* call_rcu() call stack is stored into kasan_alloc_meta.
> + * free stack is stored into freed object.
> + */
> + depot_stack_handle_t rcu_free_stack[KASAN_NR_RCU_FREE_STACKS];
> +#else
> struct kasan_track free_track[KASAN_NR_FREE_STACKS];
> +#endif
> #ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
> u8 free_track_idx;
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 80f23c9da6b0..7aaccc70b65b 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -105,9 +105,13 @@ static void end_report(unsigned long *flags)
> kasan_enable_current();
> }
>
> -static void print_track(struct kasan_track *track, const char *prefix)
> +static void print_track(struct kasan_track *track, const char *prefix,
> + bool is_callrcu)
> {
> - pr_err("%s by task %u:\n", prefix, track->pid);
> + if (is_callrcu)
> + pr_err("%s:\n", prefix);
> + else
> + pr_err("%s by task %u:\n", prefix, track->pid);
> if (track->stack) {
> unsigned long *entries;
> unsigned int nr_entries;
> @@ -159,8 +163,22 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
> (void *)(object_addr + cache->object_size));
> }
>
> +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> +static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
> +{
> + struct kasan_track free_track;
> +
> + free_track.stack = alloc_info->rcu_free_stack[0];
> + print_track(&free_track, "First call_rcu() call stack", true);
> + pr_err("\n");
> + free_track.stack = alloc_info->rcu_free_stack[1];
> + print_track(&free_track, "Last call_rcu() call stack", true);
> + pr_err("\n");
> +}
> +#endif
> +
> static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> - void *object, u8 tag)
> + void *object, u8 tag, const void *addr)
> {
> struct kasan_alloc_meta *alloc_meta;
> int i = 0;
> @@ -187,11 +205,14 @@ static void describe_object(struct kmem_cache *cache, void *object,
> if (cache->flags & SLAB_KASAN) {
> struct kasan_track *free_track;
>
> - print_track(&alloc_info->alloc_track, "Allocated");
> + print_track(&alloc_info->alloc_track, "Allocated", false);
> pr_err("\n");
> - free_track = kasan_get_free_track(cache, object, tag);
> - print_track(free_track, "Freed");
> + free_track = kasan_get_free_track(cache, object, tag, addr);
> + print_track(free_track, "Freed", false);
> pr_err("\n");
> +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> + kasan_print_rcu_free_stack(alloc_info);
> +#endif
> }
>
> describe_object_addr(cache, object, addr);
> --
> 2.18.0
>
> --
> 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 view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200506052046.14451-1-walter-zh.wu%40mediatek.com.
On Wed, 2020-05-06 at 11:46 +0200, Dmitry Vyukov wrote:
> On Wed, May 6, 2020 at 7:21 AM Walter Wu <[email protected]> wrote:
> >
> > When call_rcu() is called, we store the call_rcu() call stack into
> > slub alloc meta-data, so that KASAN report prints call_rcu() information.
> >
> > We add new KASAN_RCU_STACK_RECORD configuration option. It will record
> > first and last call_rcu() call stack and KASAN report will print two
> > call_rcu() call stack.
> >
> > This option doesn't increase the cost of memory consumption. Because
> > we don't enlarge struct kasan_alloc_meta size.
> > - add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
> > - remove free track from kasan_alloc_meta, size is 8 bytes.
> >
> > [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
> >
> > Signed-off-by: Walter Wu <[email protected]>
> > Suggested-by: Dmitry Vyukov <[email protected]>
> > Cc: Andrey Ryabinin <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > Cc: Alexander Potapenko <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Paul E. McKenney <[email protected]>
> > Cc: Josh Triplett <[email protected]>
> > Cc: Mathieu Desnoyers <[email protected]>
> > Cc: Lai Jiangshan <[email protected]>
> > Cc: Joel Fernandes <[email protected]>
> > ---
> > include/linux/kasan.h | 7 +++++++
> > kernel/rcu/tree.c | 4 ++++
> > lib/Kconfig.kasan | 11 +++++++++++
> > mm/kasan/common.c | 23 +++++++++++++++++++++++
> > mm/kasan/kasan.h | 12 ++++++++++++
> > mm/kasan/report.c | 33 +++++++++++++++++++++++++++------
> > 6 files changed, 84 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index 31314ca7c635..5eeece6893cd 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -96,6 +96,12 @@ size_t kasan_metadata_size(struct kmem_cache *cache);
> > bool kasan_save_enable_multi_shot(void);
> > void kasan_restore_multi_shot(bool enabled);
> >
> > +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > +void kasan_record_callrcu(void *ptr);
>
> The issue also mentions workqueue and timer stacks.
> Have you considered supporting them as well? What was your motivation
> for doing only rcu?
>
I will try to implement them when I have free time, maybe we can do it
step by step, finish the printing call_rcu() first.
I remember that I saw the following link, recording call_rcu seems like
be needed.
https://groups.google.com/forum/#!searchin/kasan-dev/better$20stack
$20traces$20for$20rcu%7Csort:date/kasan-dev/KQsjT_88hDE/7rNUZprRBgAJ
> Looking at the first report for "workqueue use-after-free":
> https://syzkaller.appspot.com/bug?extid=9cba1e478f91aad39876
> This is exactly the same situation as for call_rcu, just a workqueue
> is used to invoke a callback that frees the object.
>
> If you don't want to do all at the same time, I would at least
> name/branch everything inside of KASAN more generally (I think in the
> issue I called it "aux" (auxiliary), or maybe something like
> "additional"). But then call this kasan_record_aux_stack() only from
> rcu for now. But then later we can separately decide and extend to
> other callers.
> It just feels wrong to have KASAN over-specialized for rcu only in this way.
> And I think if the UAF is really caused by call_rcu callback, then it
> sill will be recorded as last stack most of the time because rcu
> callbacks are invoked relatively fast and there should not be much
> else happening with the object since it's near end of life already.
>
Yes, I agree with you. I will send v2 patchset as you saying.
>
>
>
> > +#else
> > +static inline void kasan_record_callrcu(void *ptr) {}
> > +#endif
> > +
> > #else /* CONFIG_KASAN */
> >
> > static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> > @@ -165,6 +171,7 @@ static inline void kasan_remove_zero_shadow(void *start,
> >
> > static inline void kasan_unpoison_slab(const void *ptr) { }
> > static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
> > +static inline void kasan_record_callrcu(void *ptr) {}
> >
> > #endif /* CONFIG_KASAN */
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 06548e2ebb72..145c79becf7b 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -57,6 +57,7 @@
> > #include <linux/slab.h>
> > #include <linux/sched/isolation.h>
> > #include <linux/sched/clock.h>
> > +#include <linux/kasan.h>
> > #include "../time/tick-internal.h"
> >
> > #include "tree.h"
> > @@ -2694,6 +2695,9 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func)
> > trace_rcu_callback(rcu_state.name, head,
> > rcu_segcblist_n_cbs(&rdp->cblist));
> >
> > + if (IS_ENABLED(CONFIG_KASAN_RCU_STACK_RECORD))
>
> The if is not necessary, this function is no-op when not enabled.
>
> > + kasan_record_callrcu(head);
> > +
> > /* Go handle any RCU core processing required. */
> > if (IS_ENABLED(CONFIG_RCU_NOCB_CPU) &&
> > unlikely(rcu_segcblist_is_offloaded(&rdp->cblist))) {
> > diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> > index 81f5464ea9e1..022934049cc2 100644
> > --- a/lib/Kconfig.kasan
> > +++ b/lib/Kconfig.kasan
> > @@ -158,6 +158,17 @@ config KASAN_VMALLOC
> > for KASAN to detect more sorts of errors (and to support vmapped
> > stacks), but at the cost of higher memory usage.
> >
> > +config KASAN_RCU_STACK_RECORD
> > + bool "Record and print call_rcu() call stack"
> > + depends on KASAN_GENERIC
> > + help
> > + By default, the KASAN report doesn't print call_rcu() call stack.
> > + It is very difficult to analyze memory issues(e.g., use-after-free).
> > +
> > + Enabling this option will print first and last call_rcu() call stack.
> > + It doesn't enlarge slub alloc meta-data size, so it doesn't increase
> > + the cost of memory consumption.
> > +
> > config TEST_KASAN
> > tristate "Module for testing KASAN for bug detection"
> > depends on m && KASAN
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 2906358e42f0..32d422bdf127 100644
> > --- a/mm/kasan/common.c
> > +++ b/mm/kasan/common.c
> > @@ -299,6 +299,29 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
> > return (void *)object + cache->kasan_info.free_meta_offset;
> > }
> >
> > +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > +void kasan_record_callrcu(void *addr)
> > +{
> > + struct page *page = kasan_addr_to_page(addr);
> > + struct kmem_cache *cache;
> > + struct kasan_alloc_meta *alloc_info;
> > + void *object;
> > +
> > + if (!(page && PageSlab(page)))
> > + return;
> > +
> > + cache = page->slab_cache;
> > + object = nearest_obj(cache, page, addr);
> > + alloc_info = get_alloc_info(cache, object);
> > +
> > + if (!alloc_info->rcu_free_stack[0])
> > + /* record first call_rcu() call stack */
> > + alloc_info->rcu_free_stack[0] = save_stack(GFP_NOWAIT);
> > + else
> > + /* record last call_rcu() call stack */
> > + alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
> > +}
> > +#endif
> >
> > static void kasan_set_free_info(struct kmem_cache *cache,
> > void *object, u8 tag)
> > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > index e8f37199d885..adc105b9cd07 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -96,15 +96,27 @@ struct kasan_track {
> > depot_stack_handle_t stack;
> > };
> >
> > +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > +#define BYTES_PER_WORD 4
> > +#define KASAN_NR_RCU_FREE_STACKS 2
> > +#else /* CONFIG_KASAN_RCU_STACK_RECORD */
> > #ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> > #define KASAN_NR_FREE_STACKS 5
> > #else
> > #define KASAN_NR_FREE_STACKS 1
> > #endif
> > +#endif /* CONFIG_KASAN_RCU_STACK_RECORD */
> >
> > struct kasan_alloc_meta {
> > struct kasan_track alloc_track;
> > +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > + /* call_rcu() call stack is stored into kasan_alloc_meta.
> > + * free stack is stored into freed object.
> > + */
> > + depot_stack_handle_t rcu_free_stack[KASAN_NR_RCU_FREE_STACKS];
> > +#else
> > struct kasan_track free_track[KASAN_NR_FREE_STACKS];
> > +#endif
> > #ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> > u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
> > u8 free_track_idx;
> > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > index 80f23c9da6b0..7aaccc70b65b 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -105,9 +105,13 @@ static void end_report(unsigned long *flags)
> > kasan_enable_current();
> > }
> >
> > -static void print_track(struct kasan_track *track, const char *prefix)
> > +static void print_track(struct kasan_track *track, const char *prefix,
> > + bool is_callrcu)
> > {
> > - pr_err("%s by task %u:\n", prefix, track->pid);
> > + if (is_callrcu)
> > + pr_err("%s:\n", prefix);
> > + else
> > + pr_err("%s by task %u:\n", prefix, track->pid);
> > if (track->stack) {
> > unsigned long *entries;
> > unsigned int nr_entries;
> > @@ -159,8 +163,22 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
> > (void *)(object_addr + cache->object_size));
> > }
> >
> > +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > +static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
> > +{
> > + struct kasan_track free_track;
> > +
> > + free_track.stack = alloc_info->rcu_free_stack[0];
> > + print_track(&free_track, "First call_rcu() call stack", true);
> > + pr_err("\n");
> > + free_track.stack = alloc_info->rcu_free_stack[1];
> > + print_track(&free_track, "Last call_rcu() call stack", true);
> > + pr_err("\n");
> > +}
> > +#endif
> > +
> > static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > - void *object, u8 tag)
> > + void *object, u8 tag, const void *addr)
> > {
> > struct kasan_alloc_meta *alloc_meta;
> > int i = 0;
> > @@ -187,11 +205,14 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > if (cache->flags & SLAB_KASAN) {
> > struct kasan_track *free_track;
> >
> > - print_track(&alloc_info->alloc_track, "Allocated");
> > + print_track(&alloc_info->alloc_track, "Allocated", false);
> > pr_err("\n");
> > - free_track = kasan_get_free_track(cache, object, tag);
> > - print_track(free_track, "Freed");
> > + free_track = kasan_get_free_track(cache, object, tag, addr);
> > + print_track(free_track, "Freed", false);
> > pr_err("\n");
> > +#ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > + kasan_print_rcu_free_stack(alloc_info);
> > +#endif
> > }
> >
> > describe_object_addr(cache, object, addr);
> > --
> > 2.18.0
> >
> > --
> > 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 view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200506052046.14451-1-walter-zh.wu%40mediatek.com.