Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754386AbcCBOuQ (ORCPT ); Wed, 2 Mar 2016 09:50:16 -0500 Received: from foss.arm.com ([217.140.101.70]:58956 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754136AbcCBOuN (ORCPT ); Wed, 2 Mar 2016 09:50:13 -0500 Date: Wed, 2 Mar 2016 14:50:05 +0000 From: Mark Rutland To: Andrey Ryabinin Cc: linux-kernel@vger.kernel.org, Alexander Potapenko , Andrey Konovalov , Dmitriy Vyukov , will.deacon@arm.com, catalin.marinas@arm.com, Andrew Morton , kasan-dev@googlegroups.com, Ingo Molnar , Peter Zijlstra , Thomas Gleixner Subject: Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online Message-ID: <20160302145004.GC11670@leverpostej> References: <56D6E250.7070301@gmail.com> <1456926719-13102-1-git-send-email-aryabinin@virtuozzo.com> <1456926719-13102-2-git-send-email-aryabinin@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1456926719-13102-2-git-send-email-aryabinin@virtuozzo.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3594 Lines: 103 Hi, On Wed, Mar 02, 2016 at 04:51:59PM +0300, Andrey Ryabinin wrote: > KASAN poisons stack redzones on function's entrance and unpoisons prior > return. So when cpu goes offline the stack of idle task left poisoned. > When cpu goes back online it re-enters the kernel via another path and > starts using idle task's stack. Hence it's possible to hit stale poison > values which results in false-positive KASAN splats. > > This patch registers cpu hotplug notifier which unpoisons idle task's > stack prior to onlining cpu. Sorry, I failed to spot this before sending my series just now. FWIW, I have no strong feelings either way as to how we hook up the stack shadow clearing in the hotplug case. It would be good if we could organise to share the infrastructure for idle, though. Otherwise, I have a couple of comments below. > Signed-off-by: Andrey Ryabinin > --- > include/linux/sched.h | 6 ++++++ > kernel/smpboot.h | 2 -- > mm/kasan/kasan.c | 33 +++++++++++++++++++++++++++------ > 3 files changed, 33 insertions(+), 8 deletions(-) > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index a10494a..18e526d 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -337,6 +337,12 @@ extern asmlinkage void schedule_tail(struct task_struct *prev); > extern void init_idle(struct task_struct *idle, int cpu); > extern void init_idle_bootup_task(struct task_struct *idle); > > +#ifdef CONFIG_GENERIC_SMP_IDLE_THREAD > +extern struct task_struct *idle_thread_get(unsigned int cpu); > +#else > +static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; } > +#endif > + > extern cpumask_var_t cpu_isolated_map; > > extern int runqueue_is_locked(int cpu); > diff --git a/kernel/smpboot.h b/kernel/smpboot.h > index 72415a0..eebf9ec 100644 > --- a/kernel/smpboot.h > +++ b/kernel/smpboot.h > @@ -4,11 +4,9 @@ > struct task_struct; > > #ifdef CONFIG_GENERIC_SMP_IDLE_THREAD > -struct task_struct *idle_thread_get(unsigned int cpu); > void idle_thread_set_boot_cpu(void); > void idle_threads_init(void); > #else > -static inline struct task_struct *idle_thread_get(unsigned int cpu) { return NULL; } > static inline void idle_thread_set_boot_cpu(void) { } > static inline void idle_threads_init(void) { } > #endif Is all the above necessary? Surely we can just include in mm/kasan/kasan.c? > diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c > index bc0a8d8..c4ffd82 100644 > --- a/mm/kasan/kasan.c > +++ b/mm/kasan/kasan.c > @@ -16,6 +16,7 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > #define DISABLE_BRANCH_PROFILING > > +#include > #include > #include > #include > @@ -537,16 +538,36 @@ static int kasan_mem_notifier(struct notifier_block *nb, > { > return (action == MEM_GOING_ONLINE) ? NOTIFY_BAD : NOTIFY_OK; > } > +#endif > + > +static int kasan_cpu_callback(struct notifier_block *nfb, > + unsigned long action, void *hcpu) > +{ > + unsigned int cpu = (unsigned long)hcpu; > + > + if ((action == CPU_UP_PREPARE) || (action == CPU_UP_PREPARE_FROZEN)) { > + struct task_struct *tidle = idle_thread_get(cpu); > + kasan_unpoison_shadow(task_stack_page(tidle), THREAD_SIZE); We never expect the stack to hit the end of the thread_info, so we can start at task_stack_page(tidle) + 1, and avoid the shadow for sizeof(struct thread_info). Do we do any poisoning of the thread_info structure in the thread_union? If so, we'd be erroneously clearing it here. Thanks, Mark.