Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754647AbcCBP2D (ORCPT ); Wed, 2 Mar 2016 10:28:03 -0500 Received: from mx2.parallels.com ([199.115.105.18]:46687 "EHLO mx2.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751772AbcCBP2A (ORCPT ); Wed, 2 Mar 2016 10:28:00 -0500 Subject: Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online To: Mark Rutland References: <56D6E250.7070301@gmail.com> <1456926719-13102-1-git-send-email-aryabinin@virtuozzo.com> <1456926719-13102-2-git-send-email-aryabinin@virtuozzo.com> <20160302145004.GC11670@leverpostej> CC: , Alexander Potapenko , Andrey Konovalov , Dmitriy Vyukov , , , Andrew Morton , , Ingo Molnar , Peter Zijlstra , Thomas Gleixner From: Andrey Ryabinin Message-ID: <56D70675.7050503@virtuozzo.com> Date: Wed, 2 Mar 2016 18:27:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <20160302145004.GC11670@leverpostej> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: US-EXCH.sw.swsoft.com (10.255.249.47) To US-EXCH2.sw.swsoft.com (10.255.249.46) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4304 Lines: 124 On 03/02/2016 05:50 PM, Mark Rutland wrote: > 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. > In fact, I'm also don't have strong opinion on this. Ingo, Peter, what's your preference? These patches or http://lkml.kernel.org/g/<1456928778-22491-3-git-send-email-mark.rutland@arm.com> ? > 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? > It is necessary. kernel/smpboot.h != include/linux/smpboot.h >> 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). > I wouldn't bother, it's simpler to unpoison all. Size of struct thread_info is 32-bytes. That's 4-bytes of shadow. I don't think it matters whether you do memset of 2048 or 2044 bytes. > Do we do any poisoning of the thread_info structure in the thread_union? No, why would we poison it? It's absolutely valid memory and available for access. > If so, we'd be erroneously clearing it here. > > Thanks, > Mark. >