[PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[Alexander Potapenko]

Before an ARM64 CPU is suspended, the kernel saves the context which will
be used to initialize the register state upon resume. After that and
before the actual execution of the SMC instruction the kernel creates
several stack frames which are never unpoisoned because arm_smccc_smc()
does not return. This may cause false positive stack buffer overflow
reports from KASAN.

The solution is to record the stack pointer value just before the CPU is
suspended, and unpoison the part of stack between the saved value and
the stack pointer upon resume.

Signed-off-by: Alexander Potapenko <glider@google.com>
---
 arch/arm64/kernel/suspend.c |  5 +++++
 drivers/firmware/psci.c     |  5 +++++
 include/linux/kasan.h       |  5 +++++
 mm/kasan/kasan.c            | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index 1095aa4..1070415 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -1,4 +1,5 @@
 #include <linux/ftrace.h>
+#include <linux/kasan.h>
 #include <linux/percpu.h>
 #include <linux/slab.h>
 #include <asm/cacheflush.h>
@@ -117,6 +118,10 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 		 */
 		if (hw_breakpoint_restore)
 			hw_breakpoint_restore(NULL);
+#ifdef CONFIG_KASAN
+		/* Unpoison the stack above the current frame. */
+		kasan_cpu_resume();
+#endif
 	}
 
 	unpause_graph_tracing();
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index f25cd79..2480189 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -15,6 +15,7 @@
 
 #include <linux/arm-smccc.h>
 #include <linux/errno.h>
+#include <linux/kasan.h>
 #include <linux/linkage.h>
 #include <linux/of.h>
 #include <linux/pm.h>
@@ -122,6 +123,10 @@ static unsigned long __invoke_psci_fn_smc(unsigned long function_id,
 {
 	struct arm_smccc_res res;
 
+#ifdef CONFIG_KASAN
+	/* Notify KASAN it should unpoison the stack up to this point. */
+	kasan_stack_watermark();
+#endif
 	arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
 	return res.a0;
 }
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 4b9f85c..d4fd7a4 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -62,6 +62,11 @@ void kasan_slab_free(struct kmem_cache *s, void *object);
 int kasan_module_alloc(void *addr, size_t size);
 void kasan_free_shadow(const struct vm_struct *vm);
 
+#ifdef CONFIG_ARM64
+void kasan_stack_watermark(void);
+void kasan_cpu_resume(void);
+#endif
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index bc0a8d8..6529d345 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -550,3 +550,35 @@ static int __init kasan_memhotplug_init(void)
 
 module_init(kasan_memhotplug_init);
 #endif
+
+#ifdef CONFIG_ARM64
+static DEFINE_PER_CPU(unsigned long, cpu_stack_watermark);
+
+/* Record the stack pointer before the CPU is suspended. The recorded value
+ * will be used upon resume to unpoison the dirty stack frames.
+ */
+void kasan_stack_watermark(void)
+{
+	unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
+
+	*watermark = __builtin_frame_address(0);
+}
+EXPORT_SYMBOL_GPL(kasan_stack_watermark);
+
+void kasan_cpu_resume(void)
+{
+	unsigned long sp = __builtin_frame_address(0);
+	unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
+
+	if (*watermark == 0) {
+		WARN_ON_ONCE(*watermark == 0);
+		*watermark = sp;
+		return;
+	}
+	if (sp > *watermark) {
+		kasan_unpoison_shadow(*watermark, sp - *watermark);
+		*watermark = 0;
+	}
+}
+EXPORT_SYMBOL_GPL(kasan_cpu_resume);
+#endif
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[Mark Rutland]

Hi,

On Fri, Feb 26, 2016 at 01:38:37PM +0100, Alexander Potapenko wrote:
> Before an ARM64 CPU is suspended, the kernel saves the context which will
> be used to initialize the register state upon resume. After that and
> before the actual execution of the SMC instruction the kernel creates
> several stack frames which are never unpoisoned because arm_smccc_smc()
> does not return. This may cause false positive stack buffer overflow
> reports from KASAN.
> 
> The solution is to record the stack pointer value just before the CPU is
> suspended, and unpoison the part of stack between the saved value and
> the stack pointer upon resume.

Thanks for looking into this! That's much appreciated.

I think the general approach (unposioning the stack upon cold return to
the kernel) is fine, but I have concerns with the implementation, which
I've noted below.

The problem also applies for hotplug, as leftover poison from the
hot-unplug path isn't cleaned before a CPU is hotplugged back on. The
first few functions are likely deterministic in their stack usage, so
it's not seen with a defconfig, but I think it's possible to trigger,
and it's also a cross-architecture problem shared with x86.

> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  arch/arm64/kernel/suspend.c |  5 +++++
>  drivers/firmware/psci.c     |  5 +++++
>  include/linux/kasan.h       |  5 +++++
>  mm/kasan/kasan.c            | 32 ++++++++++++++++++++++++++++++++
>  4 files changed, 47 insertions(+)
> 
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index 1095aa4..1070415 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -1,4 +1,5 @@
>  #include <linux/ftrace.h>
> +#include <linux/kasan.h>
>  #include <linux/percpu.h>
>  #include <linux/slab.h>
>  #include <asm/cacheflush.h>
> @@ -117,6 +118,10 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  		 */
>  		if (hw_breakpoint_restore)
>  			hw_breakpoint_restore(NULL);
> +#ifdef CONFIG_KASAN
> +		/* Unpoison the stack above the current frame. */
> +		kasan_cpu_resume();
> +#endif

I think this is too late. Since we returned from __cpu_suspend_enter we
have called several functions, any of which may have touched the stack,
and could have hit stale poison.

Do we have any strong guarantee that the compiler won't (in future)
extend the current stack frame arbitrarily? I can imagine that happening
if the compiler does some interprocedural analysis and/or splits this
function into specialised parts.

Given that, I think it's possible to hit stale poison even in the
current function, and I'm not keen on having the kasan_cpu_resume call
within cpu_suspend due to that.

If we're going to unpoison the stack upon re-entry to the kernel, I
think that has to happen in the return path of __cpu_suspend_enter.

>  	}
>  
>  	unpause_graph_tracing();
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index f25cd79..2480189 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -15,6 +15,7 @@
>  
>  #include <linux/arm-smccc.h>
>  #include <linux/errno.h>
> +#include <linux/kasan.h>
>  #include <linux/linkage.h>
>  #include <linux/of.h>
>  #include <linux/pm.h>
> @@ -122,6 +123,10 @@ static unsigned long __invoke_psci_fn_smc(unsigned long function_id,
>  {
>  	struct arm_smccc_res res;
>  
> +#ifdef CONFIG_KASAN
> +	/* Notify KASAN it should unpoison the stack up to this point. */
> +	kasan_stack_watermark();
> +#endif

Similarly to the comment above, I'm not sure this necessarily gives us
an accurate bound in all cases, and could easily be perturbed by
other compiler instrumentation/optimisation/specialisation.

If we go ahead with unpoisoning rather than moving functions into
uninstrumented compilation units, I think we have to clear everything
from the end of the current thread_info up to the SP in
__cpu_suspend_enter.

>  	arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>  	return res.a0;
>  }
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 4b9f85c..d4fd7a4 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -62,6 +62,11 @@ void kasan_slab_free(struct kmem_cache *s, void *object);
>  int kasan_module_alloc(void *addr, size_t size);
>  void kasan_free_shadow(const struct vm_struct *vm);
>  
> +#ifdef CONFIG_ARM64
> +void kasan_stack_watermark(void);
> +void kasan_cpu_resume(void);
> +#endif
> +
>  #else /* CONFIG_KASAN */
>  
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index bc0a8d8..6529d345 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -550,3 +550,35 @@ static int __init kasan_memhotplug_init(void)
>  
>  module_init(kasan_memhotplug_init);
>  #endif
> +
> +#ifdef CONFIG_ARM64
> +static DEFINE_PER_CPU(unsigned long, cpu_stack_watermark);
> +
> +/* Record the stack pointer before the CPU is suspended. The recorded value
> + * will be used upon resume to unpoison the dirty stack frames.
> + */
> +void kasan_stack_watermark(void)
> +{
> +	unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
> +
> +	*watermark = __builtin_frame_address(0);
> +}
> +EXPORT_SYMBOL_GPL(kasan_stack_watermark);
> +
> +void kasan_cpu_resume(void)
> +{
> +	unsigned long sp = __builtin_frame_address(0);
> +	unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
> +
> +	if (*watermark == 0) {
> +		WARN_ON_ONCE(*watermark == 0);
> +		*watermark = sp;
> +		return;
> +	}
> +	if (sp > *watermark) {
> +		kasan_unpoison_shadow(*watermark, sp - *watermark);
> +		*watermark = 0;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(kasan_cpu_resume);
> +#endif

As above, we'll need to clear the entire stack upon hotplug on all
architectures, and this should probably be reused for that (and shared
with other architectures).

Thanks,
Mark.

Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[Alexander Potapenko]

On Fri, Feb 26, 2016 at 2:53 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Fri, Feb 26, 2016 at 01:38:37PM +0100, Alexander Potapenko wrote:
>> Before an ARM64 CPU is suspended, the kernel saves the context which will
>> be used to initialize the register state upon resume. After that and
>> before the actual execution of the SMC instruction the kernel creates
>> several stack frames which are never unpoisoned because arm_smccc_smc()
>> does not return. This may cause false positive stack buffer overflow
>> reports from KASAN.
>>
>> The solution is to record the stack pointer value just before the CPU is
>> suspended, and unpoison the part of stack between the saved value and
>> the stack pointer upon resume.
>
> Thanks for looking into this! That's much appreciated.
>
> I think the general approach (unposioning the stack upon cold return to
> the kernel) is fine, but I have concerns with the implementation, which
> I've noted below.
>
> The problem also applies for hotplug, as leftover poison from the
> hot-unplug path isn't cleaned before a CPU is hotplugged back on. The
> first few functions are likely deterministic in their stack usage, so
> it's not seen with a defconfig, but I think it's possible to trigger,
> and it's also a cross-architecture problem shared with x86.
Agreed, but since I haven't yet seen problems with hotplug, it's hard
to test the fix for them.

>>
>> Signed-off-by: Alexander Potapenko <glider@google.com>
>> ---
>>  arch/arm64/kernel/suspend.c |  5 +++++
>>  drivers/firmware/psci.c     |  5 +++++
>>  include/linux/kasan.h       |  5 +++++
>>  mm/kasan/kasan.c            | 32 ++++++++++++++++++++++++++++++++
>>  4 files changed, 47 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
>> index 1095aa4..1070415 100644
>> --- a/arch/arm64/kernel/suspend.c
>> +++ b/arch/arm64/kernel/suspend.c
>> @@ -1,4 +1,5 @@
>>  #include <linux/ftrace.h>
>> +#include <linux/kasan.h>
>>  #include <linux/percpu.h>
>>  #include <linux/slab.h>
>>  #include <asm/cacheflush.h>
>> @@ -117,6 +118,10 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>>                */
>>               if (hw_breakpoint_restore)
>>                       hw_breakpoint_restore(NULL);
>> +#ifdef CONFIG_KASAN
>> +             /* Unpoison the stack above the current frame. */
>> +             kasan_cpu_resume();
>> +#endif
>
> I think this is too late. Since we returned from __cpu_suspend_enter we
> have called several functions, any of which may have touched the stack,
> and could have hit stale poison.
True.
We can probably move kasan_cpu_resume() right after
__cpu_suspend_enter() returns.

> Do we have any strong guarantee that the compiler won't (in future)
> extend the current stack frame arbitrarily? I can imagine that happening
> if the compiler does some interprocedural analysis and/or splits this
> function into specialised parts.
>
> Given that, I think it's possible to hit stale poison even in the
> current function, and I'm not keen on having the kasan_cpu_resume call
> within cpu_suspend due to that.
>
> If we're going to unpoison the stack upon re-entry to the kernel, I
> think that has to happen in the return path of __cpu_suspend_enter.
I didn't want to mess up with arch/arm64/kernel/sleep.S, but perhaps
it's better to inject kasan_cpu_resume() right into
cpu_resume_after_mmu() then?
>>       }
>>
>>       unpause_graph_tracing();
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index f25cd79..2480189 100644
>> --- a/drivers/firmware/psci.c
>> +++ b/drivers/firmware/psci.c
>> @@ -15,6 +15,7 @@
>>
>>  #include <linux/arm-smccc.h>
>>  #include <linux/errno.h>
>> +#include <linux/kasan.h>
>>  #include <linux/linkage.h>
>>  #include <linux/of.h>
>>  #include <linux/pm.h>
>> @@ -122,6 +123,10 @@ static unsigned long __invoke_psci_fn_smc(unsigned long function_id,
>>  {
>>       struct arm_smccc_res res;
>>
>> +#ifdef CONFIG_KASAN
>> +     /* Notify KASAN it should unpoison the stack up to this point. */
>> +     kasan_stack_watermark();
>> +#endif
>
> Similarly to the comment above, I'm not sure this necessarily gives us
> an accurate bound in all cases, and could easily be perturbed by
> other compiler instrumentation/optimisation/specialisation.
>
> If we go ahead with unpoisoning rather than moving functions into
> uninstrumented compilation units, I think we have to clear everything
> from the end of the current thread_info up to the SP in
> __cpu_suspend_enter.
Agreed, it should be fine to start at the current thread_info.
>>       arm_smccc_smc(function_id, arg0, arg1, arg2, 0, 0, 0, 0, &res);
>>       return res.a0;
>>  }
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index 4b9f85c..d4fd7a4 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -62,6 +62,11 @@ void kasan_slab_free(struct kmem_cache *s, void *object);
>>  int kasan_module_alloc(void *addr, size_t size);
>>  void kasan_free_shadow(const struct vm_struct *vm);
>>
>> +#ifdef CONFIG_ARM64
>> +void kasan_stack_watermark(void);
>> +void kasan_cpu_resume(void);
>> +#endif
>> +
>>  #else /* CONFIG_KASAN */
>>
>>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index bc0a8d8..6529d345 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -550,3 +550,35 @@ static int __init kasan_memhotplug_init(void)
>>
>>  module_init(kasan_memhotplug_init);
>>  #endif
>> +
>> +#ifdef CONFIG_ARM64
>> +static DEFINE_PER_CPU(unsigned long, cpu_stack_watermark);
>> +
>> +/* Record the stack pointer before the CPU is suspended. The recorded value
>> + * will be used upon resume to unpoison the dirty stack frames.
>> + */
>> +void kasan_stack_watermark(void)
>> +{
>> +     unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
>> +
>> +     *watermark = __builtin_frame_address(0);
>> +}
>> +EXPORT_SYMBOL_GPL(kasan_stack_watermark);
>> +
>> +void kasan_cpu_resume(void)
>> +{
>> +     unsigned long sp = __builtin_frame_address(0);
>> +     unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
>> +
>> +     if (*watermark == 0) {
>> +             WARN_ON_ONCE(*watermark == 0);
>> +             *watermark = sp;
>> +             return;
>> +     }
>> +     if (sp > *watermark) {
>> +             kasan_unpoison_shadow(*watermark, sp - *watermark);
>> +             *watermark = 0;
>> +     }
>> +}
>> +EXPORT_SYMBOL_GPL(kasan_cpu_resume);
>> +#endif
>
> As above, we'll need to clear the entire stack upon hotplug on all
> architectures, and this should probably be reused for that (and shared
> with other architectures).
>
> Thanks,
> Mark.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2548 bytes --]

Hi Alexander,

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on v4.5-rc5 next-20160226]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Alexander-Potapenko/kasan-arm64-Unpoison-dirty-stack-frames-when-resuming-from-suspend/20160226-204052
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux for-next/core
config: arm64-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All warnings (new ones prefixed by >>):

   mm/kasan/kasan.c: In function 'kasan_stack_watermark':
>> mm/kasan/kasan.c:564:13: warning: assignment makes integer from pointer without a cast
     *watermark = __builtin_frame_address(0);
                ^
   mm/kasan/kasan.c: In function 'kasan_cpu_resume':
>> mm/kasan/kasan.c:570:21: warning: initialization makes integer from pointer without a cast
     unsigned long sp = __builtin_frame_address(0);
                        ^
>> mm/kasan/kasan.c:579:3: warning: passing argument 1 of 'kasan_unpoison_shadow' makes pointer from integer without a cast
      kasan_unpoison_shadow(*watermark, sp - *watermark);
      ^
   mm/kasan/kasan.c:53:6: note: expected 'const void *' but argument is of type 'long unsigned int'
    void kasan_unpoison_shadow(const void *address, size_t size)
         ^

vim +564 mm/kasan/kasan.c

   558	 * will be used upon resume to unpoison the dirty stack frames.
   559	 */
   560	void kasan_stack_watermark(void)
   561	{
   562		unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
   563	
 > 564		*watermark = __builtin_frame_address(0);
   565	}
   566	EXPORT_SYMBOL_GPL(kasan_stack_watermark);
   567	
   568	void kasan_cpu_resume(void)
   569	{
 > 570		unsigned long sp = __builtin_frame_address(0);
   571		unsigned long *watermark = this_cpu_ptr(&cpu_stack_watermark);
   572	
   573		if (*watermark == 0) {
   574			WARN_ON_ONCE(*watermark == 0);
   575			*watermark = sp;
   576			return;
   577		}
   578		if (sp > *watermark) {
 > 579			kasan_unpoison_shadow(*watermark, sp - *watermark);
   580			*watermark = 0;
   581		}
   582	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 48245 bytes --]

Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[Mark Rutland]

On Fri, Feb 26, 2016 at 06:28:27PM +0100, Alexander Potapenko wrote:
> On Fri, Feb 26, 2016 at 2:53 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi,
> >
> > On Fri, Feb 26, 2016 at 01:38:37PM +0100, Alexander Potapenko wrote:
> >> Before an ARM64 CPU is suspended, the kernel saves the context which will
> >> be used to initialize the register state upon resume. After that and
> >> before the actual execution of the SMC instruction the kernel creates
> >> several stack frames which are never unpoisoned because arm_smccc_smc()
> >> does not return. This may cause false positive stack buffer overflow
> >> reports from KASAN.
> >>
> >> The solution is to record the stack pointer value just before the CPU is
> >> suspended, and unpoison the part of stack between the saved value and
> >> the stack pointer upon resume.
> >
> > Thanks for looking into this! That's much appreciated.
> >
> > I think the general approach (unposioning the stack upon cold return to
> > the kernel) is fine, but I have concerns with the implementation, which
> > I've noted below.
> >
> > The problem also applies for hotplug, as leftover poison from the
> > hot-unplug path isn't cleaned before a CPU is hotplugged back on. The
> > first few functions are likely deterministic in their stack usage, so
> > it's not seen with a defconfig, but I think it's possible to trigger,
> > and it's also a cross-architecture problem shared with x86.
> Agreed, but since I haven't yet seen problems with hotplug, it's hard
> to test the fix for them.

For testing, I used the below to deliberately hit stale poison after a
hotplug. It deliberately creates large stack frames, accessing as much
of the stack as possible to increase the chance of hitting any posion.

Mark.

---->8----
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 544a713..ef4693f 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -195,6 +195,21 @@ exit_idle:
 
 DEFINE_PER_CPU(bool, cpu_dead_idle);
 
+#define NR_STACK_ELEMS 128
+static noinline void hit_stale_poison(unsigned int frames)
+{
+       volatile unsigned long magic[NR_STACK_ELEMS];
+       int i;
+
+       for (i = 0; i < NR_STACK_ELEMS; i++)
+               magic[i] = 0;
+
+       if (frames)
+               hit_stale_poison(frames - 1);
+
+       return;
+}
+
 /*
  * Generic idle loop implementation
  *
@@ -202,6 +217,8 @@ DEFINE_PER_CPU(bool, cpu_dead_idle);
  */
 static void cpu_idle_loop(void)
 {
+       hit_stale_poison(4);
+
        while (1) {
                /*
                 * If the arch has a polling bit, we maintain an invariant:

Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[Mark Rutland]

On Fri, Feb 26, 2016 at 06:28:27PM +0100, Alexander Potapenko wrote:
> On Fri, Feb 26, 2016 at 2:53 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi,
> >
> > On Fri, Feb 26, 2016 at 01:38:37PM +0100, Alexander Potapenko wrote:
> >> Before an ARM64 CPU is suspended, the kernel saves the context which will
> >> be used to initialize the register state upon resume. After that and
> >> before the actual execution of the SMC instruction the kernel creates
> >> several stack frames which are never unpoisoned because arm_smccc_smc()
> >> does not return. This may cause false positive stack buffer overflow
> >> reports from KASAN.
> >>
> >> The solution is to record the stack pointer value just before the CPU is
> >> suspended, and unpoison the part of stack between the saved value and
> >> the stack pointer upon resume.
> >
> > Thanks for looking into this! That's much appreciated.
> >
> > I think the general approach (unposioning the stack upon cold return to
> > the kernel) is fine, but I have concerns with the implementation, which
> > I've noted below.

For the idle case I intend to respin my patch [1] which calls
kasan_unpoison_shadow from assembly in the resume path, as I think
that's the only reliable approach.

> > The problem also applies for hotplug, as leftover poison from the
> > hot-unplug path isn't cleaned before a CPU is hotplugged back on. The
> > first few functions are likely deterministic in their stack usage, so
> > it's not seen with a defconfig, but I think it's possible to trigger,
> > and it's also a cross-architecture problem shared with x86.
> Agreed, but since I haven't yet seen problems with hotplug, it's hard
> to test the fix for them.

For the common hotplug case, how about the below?

I've given it a spin locally on arm64 with the reproducer I posted
earlier.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/409466.html

---->8----
>From 34839286826c88338cd91a142b1bcc3c077a87aa Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Tue, 1 Mar 2016 19:27:23 +0000
Subject: [PATCH] sched/kasan: clear stale stack poison

CPUs get hotplugged out some levels deep in C code, and hence when KASAN
is in use, the instrumented function preambles will have left the stack
shadow area poisoned.

This poison is not cleared, so when a CPU re-enters the kernel, it is
possible for accesses in instrumented functions to hit this stale
poison, resulting in (spurious) KASAN splats.

This patch forcefully unpoisons an idle task's stack shadow when it is
re-initialised prior to a hotplug, avoiding spurious hits against stale
poison.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 include/linux/kasan.h |  4 ++++
 kernel/sched/core.c   |  3 +++
 mm/kasan/kasan.c      | 10 ++++++++++
 3 files changed, 17 insertions(+)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 4b9f85c..e00486f 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -43,6 +43,8 @@ static inline void kasan_disable_current(void)
 
 void kasan_unpoison_shadow(const void *address, size_t size);
 
+void kasan_unpoison_task_stack(struct task_struct *idle);
+
 void kasan_alloc_pages(struct page *page, unsigned int order);
 void kasan_free_pages(struct page *page, unsigned int order);
 
@@ -66,6 +68,8 @@ void kasan_free_shadow(const struct vm_struct *vm);
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
 
+static inline void kasan_unpoison_task_stack(struct task_struct *idle) {}
+
 static inline void kasan_enable_current(void) {}
 static inline void kasan_disable_current(void) {}
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9503d59..41f6b22 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -26,6 +26,7 @@
  *              Thomas Gleixner, Mike Kravetz
  */
 
+#include <linux/kasan.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/nmi.h>
@@ -5096,6 +5097,8 @@ void init_idle(struct task_struct *idle, int cpu)
 	idle->state = TASK_RUNNING;
 	idle->se.exec_start = sched_clock();
 
+	kasan_unpoison_task_stack(idle);
+
 #ifdef CONFIG_SMP
 	/*
 	 * Its possible that init_idle() gets called multiple times on a task,
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index bc0a8d8..467f394 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -60,6 +60,16 @@ void kasan_unpoison_shadow(const void *address, size_t size)
 	}
 }
 
+/*
+ * Remove any poison left on the stack from a prior hot-unplug.
+ */
+void kasan_unpoison_task_stack(struct task_struct *idle)
+{
+	void *base = task_stack_page(idle) + sizeof(struct thread_info);
+	size_t size = THREAD_SIZE - sizeof(struct thread_info);
+
+	kasan_unpoison_shadow(base, size);
+}
 
 /*
  * All functions below always inlined so compiler could
-- 
1.9.1

Re: [PATCH] sched/kasan: clear stale stack poison

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

Hi Mark,

[auto build test WARNING on tip/sched/core]
[also build test WARNING on v4.5-rc6 next-20160301]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Mark-Rutland/sched-kasan-clear-stale-stack-poison/20160302-034556
config: sparc64-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   In file included from kernel/sched/core.c:29:0:
   include/linux/kasan.h:71:53: warning: 'struct task_struct' declared inside parameter list
    static inline void kasan_unpoison_task_stack(struct task_struct *idle) {}
                                                        ^
   include/linux/kasan.h:71:53: warning: its scope is only this definition or declaration, which is probably not what you want
   kernel/sched/core.c: In function 'init_idle':
>> kernel/sched/core.c:5026:2: warning: passing argument 1 of 'kasan_unpoison_task_stack' from incompatible pointer type
     kasan_unpoison_task_stack(idle);
     ^
   In file included from kernel/sched/core.c:29:0:
   include/linux/kasan.h:71:20: note: expected 'struct task_struct *' but argument is of type 'struct task_struct *'
    static inline void kasan_unpoison_task_stack(struct task_struct *idle) {}
                       ^

vim +/kasan_unpoison_task_stack +5026 kernel/sched/core.c

  5010	 *
  5011	 * NOTE: this function does not set the idle thread's NEED_RESCHED
  5012	 * flag, to make booting more robust.
  5013	 */
  5014	void init_idle(struct task_struct *idle, int cpu)
  5015	{
  5016		struct rq *rq = cpu_rq(cpu);
  5017		unsigned long flags;
  5018	
  5019		raw_spin_lock_irqsave(&idle->pi_lock, flags);
  5020		raw_spin_lock(&rq->lock);
  5021	
  5022		__sched_fork(0, idle);
  5023		idle->state = TASK_RUNNING;
  5024		idle->se.exec_start = sched_clock();
  5025	
> 5026		kasan_unpoison_task_stack(idle);
  5027	
  5028	#ifdef CONFIG_SMP
  5029		/*
  5030		 * Its possible that init_idle() gets called multiple times on a task,
  5031		 * in that case do_set_cpus_allowed() will not do the right thing.
  5032		 *
  5033		 * And since this is boot we can forgo the serialization.
  5034		 */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 45097 bytes --]

Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend.

[Andrey Ryabinin]

On 03/01/2016 10:42 PM, Mark Rutland wrote:
> On Fri, Feb 26, 2016 at 06:28:27PM +0100, Alexander Potapenko wrote:
>> On Fri, Feb 26, 2016 at 2:53 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> Hi,
>>>
>>> On Fri, Feb 26, 2016 at 01:38:37PM +0100, Alexander Potapenko wrote:
>>>> Before an ARM64 CPU is suspended, the kernel saves the context which will
>>>> be used to initialize the register state upon resume. After that and
>>>> before the actual execution of the SMC instruction the kernel creates
>>>> several stack frames which are never unpoisoned because arm_smccc_smc()
>>>> does not return. This may cause false positive stack buffer overflow
>>>> reports from KASAN.
>>>>
>>>> The solution is to record the stack pointer value just before the CPU is
>>>> suspended, and unpoison the part of stack between the saved value and
>>>> the stack pointer upon resume.
>>>
>>> Thanks for looking into this! That's much appreciated.
>>>
>>> I think the general approach (unposioning the stack upon cold return to
>>> the kernel) is fine, but I have concerns with the implementation, which
>>> I've noted below.
> 
> For the idle case I intend to respin my patch [1] which calls
> kasan_unpoison_shadow from assembly in the resume path, as I think
> that's the only reliable approach.
> 
>>> The problem also applies for hotplug, as leftover poison from the
>>> hot-unplug path isn't cleaned before a CPU is hotplugged back on. The
>>> first few functions are likely deterministic in their stack usage, so
>>> it's not seen with a defconfig, but I think it's possible to trigger,
>>> and it's also a cross-architecture problem shared with x86.
>> Agreed, but since I haven't yet seen problems with hotplug, it's hard
>> to test the fix for them.
> 
> For the common hotplug case, how about the below?
> 

Nah, looks a bit hacky IMO. I think it's better to use cpu hotplug notifier.
I'll send patch shortly.

> I've given it a spin locally on arm64 with the reproducer I posted
> earlier.
> 
> Thanks,
> Mark.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/409466.html
> 
> ---->8----
> From 34839286826c88338cd91a142b1bcc3c077a87aa Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Tue, 1 Mar 2016 19:27:23 +0000
> Subject: [PATCH] sched/kasan: clear stale stack poison
> 
> CPUs get hotplugged out some levels deep in C code, and hence when KASAN
> is in use, the instrumented function preambles will have left the stack
> shadow area poisoned.
> 
> This poison is not cleared, so when a CPU re-enters the kernel, it is
> possible for accesses in instrumented functions to hit this stale
> poison, resulting in (spurious) KASAN splats.
> 
> This patch forcefully unpoisons an idle task's stack shadow when it is
> re-initialised prior to a hotplug, avoiding spurious hits against stale
> poison.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  include/linux/kasan.h |  4 ++++
>  kernel/sched/core.c   |  3 +++
>  mm/kasan/kasan.c      | 10 ++++++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 4b9f85c..e00486f 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -43,6 +43,8 @@ static inline void kasan_disable_current(void)
>  
>  void kasan_unpoison_shadow(const void *address, size_t size);
>  
> +void kasan_unpoison_task_stack(struct task_struct *idle);
> +
>  void kasan_alloc_pages(struct page *page, unsigned int order);
>  void kasan_free_pages(struct page *page, unsigned int order);
>  
> @@ -66,6 +68,8 @@ void kasan_free_shadow(const struct vm_struct *vm);
>  
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
>  
> +static inline void kasan_unpoison_task_stack(struct task_struct *idle) {}
> +
>  static inline void kasan_enable_current(void) {}
>  static inline void kasan_disable_current(void) {}
>  
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9503d59..41f6b22 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -26,6 +26,7 @@
>   *              Thomas Gleixner, Mike Kravetz
>   */
>  
> +#include <linux/kasan.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/nmi.h>
> @@ -5096,6 +5097,8 @@ void init_idle(struct task_struct *idle, int cpu)
>  	idle->state = TASK_RUNNING;
>  	idle->se.exec_start = sched_clock();
>  
> +	kasan_unpoison_task_stack(idle);
> +
>  #ifdef CONFIG_SMP
>  	/*
>  	 * Its possible that init_idle() gets called multiple times on a task,
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index bc0a8d8..467f394 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -60,6 +60,16 @@ void kasan_unpoison_shadow(const void *address, size_t size)
>  	}
>  }
>  
> +/*
> + * Remove any poison left on the stack from a prior hot-unplug.
> + */
> +void kasan_unpoison_task_stack(struct task_struct *idle)
> +{
> +	void *base = task_stack_page(idle) + sizeof(struct thread_info);
> +	size_t size = THREAD_SIZE - sizeof(struct thread_info);
> +
> +	kasan_unpoison_shadow(base, size);
> +}
>  
>  /*
>   * All functions below always inlined so compiler could
> 

[PATCH 1/2] cpu, idle: move init_idle() out of idle_thread_get()

[Andrey Ryabinin]

Currently idle_thread_get() not only gets the idle task but also
initializes it. This is fine for _cpu_up() which is the only caller
of idle_thread_get().

idle_thread_get() is going to be used in the next patch, but it
would be nice to avoid double initialization of idle task.
Hence this patch removes init_idle() call from idle_thread_get()
and invokes it directly from _cpu_up().

No functional changes here.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 kernel/cpu.c     | 2 ++
 kernel/smpboot.c | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 5b9d396..63d8e7b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -503,6 +503,8 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen)
 		goto out;
 	}
 
+	init_idle(idle, cpu);
+
 	ret = smpboot_create_threads(cpu);
 	if (ret)
 		goto out;
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index d264f59..74687e8 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -31,7 +31,6 @@ struct task_struct *idle_thread_get(unsigned int cpu)
 
 	if (!tsk)
 		return ERR_PTR(-ENOMEM);
-	init_idle(tsk, cpu);
 	return tsk;
 }
 
-- 
2.4.10

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

[Andrey Ryabinin]

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.

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
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);
+	}
+	return NOTIFY_OK;
+}
 
-static int __init kasan_memhotplug_init(void)
+static struct notifier_block kasan_cpu_notifier =
 {
-	pr_err("WARNING: KASAN doesn't support memory hot-add\n");
-	pr_err("Memory hot-add will be disabled\n");
+	.notifier_call = kasan_cpu_callback,
+};
 
-	hotplug_memory_notifier(kasan_mem_notifier, 0);
+static int __init kasan_notifiers_init(void)
+{
+	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG)) {
+		pr_err("WARNING: KASAN doesn't support memory hot-add\n");
+		pr_err("Memory hot-add will be disabled\n");
+		hotplug_memory_notifier(kasan_mem_notifier, 0);
+	}
+
+	register_hotcpu_notifier(&kasan_cpu_notifier);
 
 	return 0;
 }
 
-module_init(kasan_memhotplug_init);
-#endif
+module_init(kasan_notifiers_init);
-- 
2.4.10

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

[Mark Rutland]

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@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?

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

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

[Andrey Ryabinin]

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

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

[Mark Rutland]

On Wed, Mar 02, 2016 at 06:27:49PM +0300, Andrey Ryabinin wrote:
> On 03/02/2016 05:50 PM, Mark Rutland wrote:
> > On Wed, Mar 02, 2016 at 04:51:59PM +0300, Andrey Ryabinin wrote:

[...]

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

Ah, I'd misread the patch. Sorry for the noise!

[...]

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

For some reason I thought ASAN might poison gaps between struct
elements, or at least held open the option to. I guess inserting padding
would be an ABI issue, so it probably doesn't.

In the absence of that, I agree that always starting at
task_stack_page(t), and clearing the shadow for THREAD_SIZE bytes of
stack makes sense).

Thanks,
Mark.