linux-kernel - Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 2 Mar 2016 14:50:05 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Andrey Ryabinin <aryabinin@...tuozzo.com>
Cc:	linux-kernel@...r.kernel.org,
	Alexander Potapenko <glider@...gle.com>,
	Andrey Konovalov <adech.fo@...il.com>,
	Dmitriy Vyukov <dvyukov@...gle.com>, will.deacon@....com,
	catalin.marinas@....com, Andrew Morton <akpm@...ux-foundation.org>,
	kasan-dev@...glegroups.com, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online

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 <aryabinin@...tuozzo.com>
> ---
>  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 <linux/smpboot.h> 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 <linux/cpu.h>
>  #include <linux/export.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -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.