lkml.org 
[lkml]   [2016]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online
From
Date


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

\
 
 \ /
  Last update: 2016-03-02 17:21    [W:1.127 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site