This patch records the last two timer queueing stacks and prints
up to 2 timer stacks in KASAN report. It is useful for programmers
to solve use-after-free or double-free memory timer issues.
When timer_setup() or timer_setup_on_stack() is called, then it
prepares to use this timer and sets timer callback, we store
this call stack in order to print it in KASAN report.
Signed-off-by: Walter Wu <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Alexander Potapenko <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Andrew Morton <[email protected]>
---
include/linux/kasan.h | 2 ++
kernel/time/timer.c | 2 ++
mm/kasan/generic.c | 21 +++++++++++++++++++++
mm/kasan/kasan.h | 4 +++-
mm/kasan/report.c | 11 +++++++++++
5 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 23b7ee00572d..763664b36dc6 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -175,12 +175,14 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
void kasan_cache_shrink(struct kmem_cache *cache);
void kasan_cache_shutdown(struct kmem_cache *cache);
void kasan_record_aux_stack(void *ptr);
+void kasan_record_tmr_stack(void *ptr);
#else /* CONFIG_KASAN_GENERIC */
static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
static inline void kasan_record_aux_stack(void *ptr) {}
+static inline void kasan_record_tmr_stack(void *ptr) {}
#endif /* CONFIG_KASAN_GENERIC */
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index a5221abb4594..ef2da9ddfac7 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -783,6 +783,8 @@ static void do_init_timer(struct timer_list *timer,
timer->function = func;
timer->flags = flags | raw_smp_processor_id();
lockdep_init_map(&timer->lockdep_map, name, key, 0);
+
+ kasan_record_tmr_stack(timer);
}
/**
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 4b3cbad7431b..f35dcec990ab 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -347,6 +347,27 @@ void kasan_record_aux_stack(void *addr)
alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT);
}
+void kasan_record_tmr_stack(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);
+
+ /*
+ * record the last two timer stacks.
+ */
+ alloc_info->tmr_stack[1] = alloc_info->tmr_stack[0];
+ alloc_info->tmr_stack[0] = kasan_save_stack(GFP_NOWAIT);
+}
+
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 ef655a1c6e15..c50827f388a3 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -108,10 +108,12 @@ struct kasan_alloc_meta {
struct kasan_track alloc_track;
#ifdef CONFIG_KASAN_GENERIC
/*
- * call_rcu() call stack is stored into struct kasan_alloc_meta.
+ * call_rcu() call stack and timer queueing stack are stored
+ * into struct kasan_alloc_meta.
* The free stack is stored into struct kasan_free_meta.
*/
depot_stack_handle_t aux_stack[2];
+ depot_stack_handle_t tmr_stack[2];
#else
struct kasan_track free_track[KASAN_NR_FREE_STACKS];
#endif
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index fed3c8fdfd25..6fa3bfee381f 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -191,6 +191,17 @@ static void describe_object(struct kmem_cache *cache, void *object,
print_stack(alloc_info->aux_stack[1]);
pr_err("\n");
}
+
+ if (alloc_info->tmr_stack[0]) {
+ pr_err("Last timer stack:\n");
+ print_stack(alloc_info->tmr_stack[0]);
+ pr_err("\n");
+ }
+ if (alloc_info->tmr_stack[1]) {
+ pr_err("Second to last timer stack:\n");
+ print_stack(alloc_info->tmr_stack[1]);
+ pr_err("\n");
+ }
#endif
}
--
2.18.0
On Mon, 10 Aug 2020 at 09:23, Walter Wu <[email protected]> wrote:
> This patch records the last two timer queueing stacks and prints
> up to 2 timer stacks in KASAN report. It is useful for programmers
> to solve use-after-free or double-free memory timer issues.
>
> When timer_setup() or timer_setup_on_stack() is called, then it
> prepares to use this timer and sets timer callback, we store
> this call stack in order to print it in KASAN report.
>
> Signed-off-by: Walter Wu <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Andrew Morton <[email protected]>
> ---
> include/linux/kasan.h | 2 ++
> kernel/time/timer.c | 2 ++
> mm/kasan/generic.c | 21 +++++++++++++++++++++
> mm/kasan/kasan.h | 4 +++-
> mm/kasan/report.c | 11 +++++++++++
> 5 files changed, 39 insertions(+), 1 deletion(-)
I'm commenting on the code here, but it also applies to patch 2/5, as
it's almost a copy-paste.
In general, I'd say the solution to get this feature is poorly
designed, resulting in excessive LOC added. The logic added already
exists for the aux stacks.
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 23b7ee00572d..763664b36dc6 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -175,12 +175,14 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
> void kasan_cache_shrink(struct kmem_cache *cache);
> void kasan_cache_shutdown(struct kmem_cache *cache);
> void kasan_record_aux_stack(void *ptr);
> +void kasan_record_tmr_stack(void *ptr);
>
> #else /* CONFIG_KASAN_GENERIC */
>
> static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
> static inline void kasan_record_aux_stack(void *ptr) {}
> +static inline void kasan_record_tmr_stack(void *ptr) {}
It appears that the 'aux' stack is currently only used for call_rcu
stacks, but this interface does not inherently tie it to call_rcu. The
only thing tying it to call_rcu is the fact that the report calls out
call_rcu.
> /**
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 4b3cbad7431b..f35dcec990ab 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -347,6 +347,27 @@ void kasan_record_aux_stack(void *addr)
> alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT);
> }
>
> +void kasan_record_tmr_stack(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);
> +
> + /*
> + * record the last two timer stacks.
> + */
> + alloc_info->tmr_stack[1] = alloc_info->tmr_stack[0];
> + alloc_info->tmr_stack[0] = kasan_save_stack(GFP_NOWAIT);
> +}
The solution here is, unfortunately, poorly designed. This is a
copy-paste of the kasan_record_aux_stack() function.
> 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 ef655a1c6e15..c50827f388a3 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -108,10 +108,12 @@ struct kasan_alloc_meta {
> struct kasan_track alloc_track;
> #ifdef CONFIG_KASAN_GENERIC
> /*
> - * call_rcu() call stack is stored into struct kasan_alloc_meta.
> + * call_rcu() call stack and timer queueing stack are stored
> + * into struct kasan_alloc_meta.
> * The free stack is stored into struct kasan_free_meta.
> */
> depot_stack_handle_t aux_stack[2];
> + depot_stack_handle_t tmr_stack[2];
> #else
> struct kasan_track free_track[KASAN_NR_FREE_STACKS];
> #endif
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index fed3c8fdfd25..6fa3bfee381f 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -191,6 +191,17 @@ static void describe_object(struct kmem_cache *cache, void *object,
> print_stack(alloc_info->aux_stack[1]);
> pr_err("\n");
> }
> +
> + if (alloc_info->tmr_stack[0]) {
> + pr_err("Last timer stack:\n");
> + print_stack(alloc_info->tmr_stack[0]);
> + pr_err("\n");
> + }
> + if (alloc_info->tmr_stack[1]) {
> + pr_err("Second to last timer stack:\n");
> + print_stack(alloc_info->tmr_stack[1]);
> + pr_err("\n");
> + }
Why can't we just use the aux stack for everything, and simply change
the message printed in the report. After all, the stack trace will
include all the information to tell if it's call_rcu, timer, or
workqueue.
The reporting code would simply have to be changed to say something
like "Last potentially related work creation:" -- because what the
"aux" thing is trying to abstract are stack traces to work creations
that took an address that is closely related. Whether or not you want
to call it "work" is up to you, but that's the most generic term I
could think of right now (any better terms?).
Another argument for this consolidation is that it's highly unlikely
that aux_stack[a] && tmr_stack[b] && wq_stack[c], and you need to
print all the stacks. If you are worried we need more aux stacks, just
make the array size 3+ (but I think it's not necessary).
Thanks,
-- Marco
On Wed, 2020-08-12 at 16:13 +0200, Marco Elver wrote:
> On Mon, 10 Aug 2020 at 09:23, Walter Wu <[email protected]> wrote:
> > This patch records the last two timer queueing stacks and prints
> > up to 2 timer stacks in KASAN report. It is useful for programmers
> > to solve use-after-free or double-free memory timer issues.
> >
> > When timer_setup() or timer_setup_on_stack() is called, then it
> > prepares to use this timer and sets timer callback, we store
> > this call stack in order to print it in KASAN report.
> >
> > Signed-off-by: Walter Wu <[email protected]>
> > Cc: Andrey Ryabinin <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > Cc: Alexander Potapenko <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: John Stultz <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > ---
> > include/linux/kasan.h | 2 ++
> > kernel/time/timer.c | 2 ++
> > mm/kasan/generic.c | 21 +++++++++++++++++++++
> > mm/kasan/kasan.h | 4 +++-
> > mm/kasan/report.c | 11 +++++++++++
> > 5 files changed, 39 insertions(+), 1 deletion(-)
>
> I'm commenting on the code here, but it also applies to patch 2/5, as
> it's almost a copy-paste.
>
> In general, I'd say the solution to get this feature is poorly
> designed, resulting in excessive LOC added. The logic added already
> exists for the aux stacks.
>
That's true, we will have refactoring for it.
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index 23b7ee00572d..763664b36dc6 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -175,12 +175,14 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
> > void kasan_cache_shrink(struct kmem_cache *cache);
> > void kasan_cache_shutdown(struct kmem_cache *cache);
> > void kasan_record_aux_stack(void *ptr);
> > +void kasan_record_tmr_stack(void *ptr);
> >
> > #else /* CONFIG_KASAN_GENERIC */
> >
> > static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> > static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
> > static inline void kasan_record_aux_stack(void *ptr) {}
> > +static inline void kasan_record_tmr_stack(void *ptr) {}
>
> It appears that the 'aux' stack is currently only used for call_rcu
> stacks, but this interface does not inherently tie it to call_rcu. The
> only thing tying it to call_rcu is the fact that the report calls out
> call_rcu.
>
> > /**
> > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> > index 4b3cbad7431b..f35dcec990ab 100644
> > --- a/mm/kasan/generic.c
> > +++ b/mm/kasan/generic.c
> > @@ -347,6 +347,27 @@ void kasan_record_aux_stack(void *addr)
> > alloc_info->aux_stack[0] = kasan_save_stack(GFP_NOWAIT);
> > }
> >
> > +void kasan_record_tmr_stack(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);
> > +
> > + /*
> > + * record the last two timer stacks.
> > + */
> > + alloc_info->tmr_stack[1] = alloc_info->tmr_stack[0];
> > + alloc_info->tmr_stack[0] = kasan_save_stack(GFP_NOWAIT);
> > +}
>
> The solution here is, unfortunately, poorly designed. This is a
> copy-paste of the kasan_record_aux_stack() function.
>
kasan_record_aux_stack() will be re-used for call_rcu, timer, and
workqueue.
> > 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 ef655a1c6e15..c50827f388a3 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -108,10 +108,12 @@ struct kasan_alloc_meta {
> > struct kasan_track alloc_track;
> > #ifdef CONFIG_KASAN_GENERIC
> > /*
> > - * call_rcu() call stack is stored into struct kasan_alloc_meta.
> > + * call_rcu() call stack and timer queueing stack are stored
> > + * into struct kasan_alloc_meta.
> > * The free stack is stored into struct kasan_free_meta.
> > */
> > depot_stack_handle_t aux_stack[2];
> > + depot_stack_handle_t tmr_stack[2];
> > #else
> > struct kasan_track free_track[KASAN_NR_FREE_STACKS];
> > #endif
> > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > index fed3c8fdfd25..6fa3bfee381f 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -191,6 +191,17 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > print_stack(alloc_info->aux_stack[1]);
> > pr_err("\n");
> > }
> > +
> > + if (alloc_info->tmr_stack[0]) {
> > + pr_err("Last timer stack:\n");
> > + print_stack(alloc_info->tmr_stack[0]);
> > + pr_err("\n");
> > + }
> > + if (alloc_info->tmr_stack[1]) {
> > + pr_err("Second to last timer stack:\n");
> > + print_stack(alloc_info->tmr_stack[1]);
> > + pr_err("\n");
> > + }
>
> Why can't we just use the aux stack for everything, and simply change
> the message printed in the report. After all, the stack trace will
> include all the information to tell if it's call_rcu, timer, or
> workqueue.
>
This is a good suggestion, next patch will do it.
> The reporting code would simply have to be changed to say something
> like "Last potentially related work creation:" -- because what the
> "aux" thing is trying to abstract are stack traces to work creations
> that took an address that is closely related. Whether or not you want
> to call it "work" is up to you, but that's the most generic term I
> could think of right now (any better terms?).
>
Work is good.
> Another argument for this consolidation is that it's highly unlikely
> that aux_stack[a] && tmr_stack[b] && wq_stack[c], and you need to
> print all the stacks. If you are worried we need more aux stacks, just
> make the array size 3+ (but I think it's not necessary).
>
We hope that next patch keep it as it is aux_stack[2], it may record
call_rcu, timer, or workqueue. Because struct kasan_alloc_meta is 16
bytes, it will have the minimal redzone size and good alignment, lets
get better memory consumption.
Thanks for your good suggestion.
Walter
> Thanks,
> -- Marco
Walter,
Walter Wu <[email protected]> writes:
> This patch records the last two timer queueing stacks and prints
"This patch" is useless information as we already know from the subject
line that this is a patch.
git grep 'This patch' Documentation/process/
> up to 2 timer stacks in KASAN report. It is useful for programmers
> to solve use-after-free or double-free memory timer issues.
>
> When timer_setup() or timer_setup_on_stack() is called, then it
> prepares to use this timer and sets timer callback, we store
> this call stack in order to print it in KASAN report.
we store nothing. Don't impersonate code please.
Also please structure the changelog in a way that it's easy to
understand what this is about instead of telling first what the patch
does and then some half baken information why this is useful followed by
more information about what it does.
Something like this:
For analysing use after free or double free of objects it is helpful
to preserve usage history which potentially gives a hint about the
affected code.
For timers it has turned out to be useful to record the stack trace
of the timer init call. <ADD technical explanation why this is useful>
Record the most recent two timer init calls in KASAN which are printed
on failure in the KASAN report.
See, this gives a clear context, an explanation why it is useful and a
high level description of what it does. The details are in the patch
ifself and do not have to be epxlained in the changelog.
For the technical explanation which you need to add, you really need to
tell what's the advantage or additional coverage vs. existing debug
facilities like debugobjects. Just claiming that it's useful does not
make an argument.
The UAF problem with timers is nasty because if you free an active timer
then either the softirq which expires the timer will corrupt potentially
reused memory or the reuse will corrupt the linked list which makes the
softirq or some unrelated code which adds/removes a different timer
explode in undebuggable ways. debugobject prevents that because it
tracks per timer state and invokes the fixup function which keeps the
system alive and also tells you exactly where the free of the active
object happens which is the really interesting place to look at. The
init function is pretty uninteresting in that case because you really
want to know where the freeing of the active object happens.
So if KASAN detects UAF in the timer softirq then the init trace is not
giving any information especially not in cases where the timer is part
of a common and frequently allocated/freed other data structure.
> static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
> static inline void kasan_record_aux_stack(void *ptr) {}
> +static inline void kasan_record_tmr_stack(void *ptr) {}
Duh, so you are adding per object type functions and storage? That's
going to be a huge copy and pasta orgy as every object requires the same
code and extra storage space.
Why not just using kasan_record_aux_stack() for all of this?
The 'call_rcu' 'timer' 'whatever next' printout is not really required
because the stack trace already tells you the function which was
invoked. If TOS is call_rcu() or do_timer_init() then it's entirely
clear which object is affected. If the two aux records are not enough
then making the array larger is not the end of the world.
> #endif /* CONFIG_KASAN_GENERIC */
>
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index a5221abb4594..ef2da9ddfac7 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -783,6 +783,8 @@ static void do_init_timer(struct timer_list *timer,
> timer->function = func;
> timer->flags = flags | raw_smp_processor_id();
> lockdep_init_map(&timer->lockdep_map, name, key, 0);
> +
> + kasan_record_tmr_stack(timer);
> }
Are you sure this is correct for all timers?
This is also called for timers which are temporarily allocated on stack
and for timers which are statically allocated at compile time. How is
that supposed to work?
These kind of things want to be explained upfront an not left to the
reviewer as an exercise.
Thanks,
tglx
On Thu, 2020-08-13 at 13:48 +0200, Thomas Gleixner wrote:
> Walter,
>
> Walter Wu <[email protected]> writes:
> > This patch records the last two timer queueing stacks and prints
>
> "This patch" is useless information as we already know from the subject
> line that this is a patch.
>
> git grep 'This patch' Documentation/process/
>
Thanks for your information.
> > up to 2 timer stacks in KASAN report. It is useful for programmers
> > to solve use-after-free or double-free memory timer issues.
> >
> > When timer_setup() or timer_setup_on_stack() is called, then it
> > prepares to use this timer and sets timer callback, we store
> > this call stack in order to print it in KASAN report.
>
> we store nothing. Don't impersonate code please.
>
> Also please structure the changelog in a way that it's easy to
> understand what this is about instead of telling first what the patch
> does and then some half baken information why this is useful followed by
> more information about what it does.
>
> Something like this:
>
> For analysing use after free or double free of objects it is helpful
> to preserve usage history which potentially gives a hint about the
> affected code.
>
> For timers it has turned out to be useful to record the stack trace
> of the timer init call. <ADD technical explanation why this is useful>
>
> Record the most recent two timer init calls in KASAN which are printed
> on failure in the KASAN report.
>
> See, this gives a clear context, an explanation why it is useful and a
> high level description of what it does. The details are in the patch
> ifself and do not have to be epxlained in the changelog.
>
> For the technical explanation which you need to add, you really need to
> tell what's the advantage or additional coverage vs. existing debug
> facilities like debugobjects. Just claiming that it's useful does not
> make an argument.
>
> The UAF problem with timers is nasty because if you free an active timer
> then either the softirq which expires the timer will corrupt potentially
> reused memory or the reuse will corrupt the linked list which makes the
> softirq or some unrelated code which adds/removes a different timer
> explode in undebuggable ways. debugobject prevents that because it
> tracks per timer state and invokes the fixup function which keeps the
> system alive and also tells you exactly where the free of the active
> object happens which is the really interesting place to look at. The
> init function is pretty uninteresting in that case because you really
> want to know where the freeing of the active object happens.
>
It is what we want to achieve, maybe we have shortcomings, but my patch
> So if KASAN detects UAF in the timer softirq then the init trace is not
> giving any information especially not in cases where the timer is part
> of a common and frequently allocated/freed other data structure.
>
> > static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> > static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
> > static inline void kasan_record_aux_stack(void *ptr) {}
> > +static inline void kasan_record_tmr_stack(void *ptr) {}
>
> Duh, so you are adding per object type functions and storage? That's
> going to be a huge copy and pasta orgy as every object requires the same
> code and extra storage space.
>
> Why not just using kasan_record_aux_stack() for all of this?
>
> The 'call_rcu' 'timer' 'whatever next' printout is not really required
> because the stack trace already tells you the function which was
> invoked. If TOS is call_rcu() or do_timer_init() then it's entirely
> clear which object is affected. If the two aux records are not enough
> then making the array larger is not the end of the world.
>
> > #endif /* CONFIG_KASAN_GENERIC */
> >
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index a5221abb4594..ef2da9ddfac7 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -783,6 +783,8 @@ static void do_init_timer(struct timer_list *timer,
> > timer->function = func;
> > timer->flags = flags | raw_smp_processor_id();
> > lockdep_init_map(&timer->lockdep_map, name, key, 0);
> > +
> > + kasan_record_tmr_stack(timer);
> > }
>
> Are you sure this is correct for all timers?
>
> This is also called for timers which are temporarily allocated on stack
> and for timers which are statically allocated at compile time. How is
> that supposed to work?
>
> These kind of things want to be explained upfront an not left to the
> reviewer as an exercise.
>
> Thanks,
>
> tglx
I have already
Hi Thomas,
Please ignore my previous mail. Thanks.
On Thu, 2020-08-13 at 13:48 +0200, Thomas Gleixner wrote:
> Walter,
>
> Walter Wu <[email protected]> writes:
> > This patch records the last two timer queueing stacks and prints
>
> "This patch" is useless information as we already know from the subject
> line that this is a patch.
>
> git grep 'This patch' Documentation/process/
>
Thanks for your information.
> > up to 2 timer stacks in KASAN report. It is useful for programmers
> > to solve use-after-free or double-free memory timer issues.
> >
> > When timer_setup() or timer_setup_on_stack() is called, then it
> > prepares to use this timer and sets timer callback, we store
> > this call stack in order to print it in KASAN report.
>
> we store nothing. Don't impersonate code please.
>
> Also please structure the changelog in a way that it's easy to
> understand what this is about instead of telling first what the patch
> does and then some half baken information why this is useful followed by
> more information about what it does.
>
> Something like this:
>
> For analysing use after free or double free of objects it is helpful
> to preserve usage history which potentially gives a hint about the
> affected code.
>
> For timers it has turned out to be useful to record the stack trace
> of the timer init call. <ADD technical explanation why this is useful>
>
> Record the most recent two timer init calls in KASAN which are printed
> on failure in the KASAN report.
>
> See, this gives a clear context, an explanation why it is useful and a
> high level description of what it does. The details are in the patch
> ifself and do not have to be epxlained in the changelog.
>
Thanks for your explanation, Our patch will use this as a template from
now on.
> For the technical explanation which you need to add, you really need to
> tell what's the advantage or additional coverage vs. existing debug
> facilities like debugobjects. Just claiming that it's useful does not
> make an argument.
>
We originally wanted him to have similar functions. Maybe he can't
completely replace, but KASAN can ave this ability.
> The UAF problem with timers is nasty because if you free an active timer
> then either the softirq which expires the timer will corrupt potentially
> reused memory or the reuse will corrupt the linked list which makes the
> softirq or some unrelated code which adds/removes a different timer
> explode in undebuggable ways. debugobject prevents that because it
> tracks per timer state and invokes the fixup function which keeps the
> system alive and also tells you exactly where the free of the active
> object happens which is the really interesting place to look at. The
> init function is pretty uninteresting in that case because you really
> want to know where the freeing of the active object happens.
>
> So if KASAN detects UAF in the timer softirq then the init trace is not
> giving any information especially not in cases where the timer is part
> of a common and frequently allocated/freed other data structure.
>
I don't have experience using this tool, but I will survey it.
> > static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> > static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
> > static inline void kasan_record_aux_stack(void *ptr) {}
> > +static inline void kasan_record_tmr_stack(void *ptr) {}
>
> Duh, so you are adding per object type functions and storage? That's
> going to be a huge copy and pasta orgy as every object requires the same
> code and extra storage space.
>
> Why not just using kasan_record_aux_stack() for all of this?
>
> The 'call_rcu' 'timer' 'whatever next' printout is not really required
> because the stack trace already tells you the function which was
> invoked. If TOS is call_rcu() or do_timer_init() then it's entirely
> clear which object is affected. If the two aux records are not enough
> then making the array larger is not the end of the world.
>
My previous mail say that we will re-use kasan_record_aux_stack() and
only have aux_stack.
> > #endif /* CONFIG_KASAN_GENERIC */
> >
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index a5221abb4594..ef2da9ddfac7 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -783,6 +783,8 @@ static void do_init_timer(struct timer_list *timer,
> > timer->function = func;
> > timer->flags = flags | raw_smp_processor_id();
> > lockdep_init_map(&timer->lockdep_map, name, key, 0);
> > +
> > + kasan_record_tmr_stack(timer);
> > }
>
> Are you sure this is correct for all timers?
>
> This is also called for timers which are temporarily allocated on stack
> and for timers which are statically allocated at compile time. How is
> that supposed to work?
>
If I understand correctly, KASAN report have this record only for slub
variable. So what you said shouldn't be a problem.
> These kind of things want to be explained upfront an not left to the
> reviewer as an exercise.
>
Sorry, My fault. Later we will be more cautious to send patch.
> Thanks,
>
> tglx