On Tue, 12 Dec 2023 at 01:14, <andrey.konovalov@xxxxxxxxx> wrote: > > From: Andrey Konovalov <andreyknvl@xxxxxxxxxx> > > kasan_record_aux_stack can be called concurrently on the same object. > This might lead to a race condition when rotating the saved aux stack > trace handles. > > Fix by introducing a spinlock to protect the aux stack trace handles > in kasan_record_aux_stack. > > Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > Reported-by: syzbot+186b55175d8360728234@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://lore.kernel.org/all/000000000000784b1c060b0074a2@xxxxxxxxxx/ > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx> > > --- > > This can be squashed into "kasan: use stack_depot_put for Generic mode" > or left standalone. > --- > mm/kasan/generic.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c > index 54e20b2bc3e1..ca5c75a1866c 100644 > --- a/mm/kasan/generic.c > +++ b/mm/kasan/generic.c > @@ -25,6 +25,7 @@ > #include <linux/sched.h> > #include <linux/sched/task_stack.h> > #include <linux/slab.h> > +#include <linux/spinlock.h> > #include <linux/stackdepot.h> > #include <linux/stacktrace.h> > #include <linux/string.h> > @@ -35,6 +36,8 @@ > #include "kasan.h" > #include "../slab.h" > > +DEFINE_SPINLOCK(aux_lock); No, please don't. > /* > * All functions below always inlined so compiler could > * perform better optimizations in each of __asan_loadX/__assn_storeX > @@ -502,6 +505,8 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags) > struct kmem_cache *cache; > struct kasan_alloc_meta *alloc_meta; > void *object; > + depot_stack_handle_t new_handle, old_handle; > + unsigned long flags; > > if (is_kfence_address(addr) || !slab) > return; > @@ -512,9 +517,15 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags) > if (!alloc_meta) > return; > > - stack_depot_put(alloc_meta->aux_stack[1]); > + new_handle = kasan_save_stack(0, depot_flags); > + > + spin_lock_irqsave(&aux_lock, flags); This is a unnecessary global lock. What's the problem here? As far as I can understand a race is possible where we may end up with duplicated or lost stack handles. Since storing this information is best effort anyway, and bugs are rare, a global lock protecting this is overkill. I'd just accept the racyness and use READ_ONCE() / WRITE_ONCE() just to make sure we don't tear any reads/writes and the depot handles are valid. There are other more complex schemes [1], but I think they are overkill as well. [1]: Since a depot stack handle is just an u32, we can have a union { depot_stack_handle_t handles[2]; atomic64_t atomic_handle; } aux_stack; (BUILD_BUG_ON somewhere if sizeof handles and atomic_handle mismatch.) Then in the code here create the same union and load atomic_handle. Swap handle[1] into handle[0] and write the new one in handles[1]. Then do a cmpxchg loop to store the new atomic_handle. > + old_handle = alloc_meta->aux_stack[1]; > alloc_meta->aux_stack[1] = alloc_meta->aux_stack[0]; > - alloc_meta->aux_stack[0] = kasan_save_stack(0, depot_flags); > + alloc_meta->aux_stack[0] = new_handle; > + spin_unlock_irqrestore(&aux_lock, flags); > + > + stack_depot_put(old_handle); > } > > void kasan_record_aux_stack(void *addr) > -- > 2.25.1 >
