| Message ID | 20200810072313.529-1-walter-zh.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: add workqueue and timer stack for generic KASAN | expand |
On Mon, 10 Aug 2020 at 09:23, Walter Wu <walter-zh.wu@mediatek.com> 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 <walter-zh.wu@mediatek.com> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Alexander Potapenko <glider@google.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > --- > 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 <walter-zh.wu@mediatek.com> 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 <walter-zh.wu@mediatek.com> > > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> > > Cc: Dmitry Vyukov <dvyukov@google.com> > > Cc: Alexander Potapenko <glider@google.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: John Stultz <john.stultz@linaro.org> > > Cc: Stephen Boyd <sboyd@kernel.org> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > --- > > 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 <walter-zh.wu@mediatek.com> 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 <walter-zh.wu@mediatek.com> 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 <walter-zh.wu@mediatek.com> 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
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 }
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 <walter-zh.wu@mediatek.com> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Alexander Potapenko <glider@google.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: John Stultz <john.stultz@linaro.org> Cc: Stephen Boyd <sboyd@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> --- 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(-)