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

[Andrey Ryabinin <aryabinin@virtuozzo.com>]

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 <aryabinin@virtuozzo.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?
> 

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 <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).
> 

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.
>