linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
@ 2016-03-02 14:26 Mark Rutland
  2016-03-02 14:26 ` [PATCH 1/3] kasan: add functions to clear stack poison Mark Rutland
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Mark Rutland @ 2016-03-02 14:26 UTC (permalink / raw)
  To: linux-kernel, linux-arch, linux-arm-kernel
  Cc: akpm, aryabinin, catalin.marinas, glider, lorenzo.pieralisi,
	mark.rutland, mingo, peterz, will.deacon

Functions which the compiler has instrumented for ASAN place poison on
the stack shadow upon entry and remove this poison prior to returning.

In some cases (e.g. hotplug and idle), CPUs may exit the kernel a number
of levels deep in C code. If there are any instrumented functions on
this critical path, these will leave portions of the idle thread stack
shadow poisoned.

If a CPU returns to the kernel via a different path (e.g. a cold entry),
then depending on stack frame layout subsequent calls to instrumented
functions may use regions of the stack with stale poison, resulting in
(spurious) KASAN splats to the console.

Contemporary GCCs always add stack shadow poisoning when ASAN is
enabled, even when asked to not instrument a function [1], so we can't
simply annotate functions on the critical path to avoid poisoning.

Instead, this series explicitly removes any stale poison before it can
be hit. In the common hotplug case we clear the entire stack shadow in
common code, before a CPU is brought online.

On architectures which perform a cold return as part of cpu idle may
retain an architecture-specific amount of stack contents. To retain the
poison for this retained context, the arch code must call the core KASAN
code, passing a "watermark" stack pointer value beyond which shadow will
be cleared. Architectures which don't perform a cold return as part of
idle do not need any additional code.

This is a combination of previous approaches [2,3], attempting to keep
as much as possible generic.

Thanks,
Mark.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69863
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/409466.html
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/411850.html

Mark Rutland (3):
  kasan: add functions to clear stack poison
  sched/kasan: remove stale KASAN poison after hotplug
  arm64: kasan: clear stale stack poison

 arch/arm64/kernel/sleep.S |  4 ++++
 include/linux/kasan.h     |  6 +++++-
 kernel/sched/core.c       |  3 +++
 mm/kasan/kasan.c          | 20 ++++++++++++++++++++
 4 files changed, 32 insertions(+), 1 deletion(-)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] kasan: add functions to clear stack poison
  2016-03-02 14:26 [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Mark Rutland
@ 2016-03-02 14:26 ` Mark Rutland
  2016-03-02 14:26 ` [PATCH 2/3] sched/kasan: remove stale KASAN poison after hotplug Mark Rutland
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2016-03-02 14:26 UTC (permalink / raw)
  To: linux-kernel, linux-arch, linux-arm-kernel
  Cc: akpm, aryabinin, catalin.marinas, glider, lorenzo.pieralisi,
	mark.rutland, mingo, peterz, will.deacon

Functions which the compiler has instrumented for ASAN place poison on
the stack shadow upon entry and remove this poison prior to returning.

In some cases (e.g. hotplug and idle), CPUs may exit the kernel a number
of levels deep in C code. If there are any instrumented functions on
this critical path, these will leave portions of the stack shadow
poisoned.

If a CPU returns to the kernel via a different path (e.g. a cold entry),
then depending on stack frame layout subsequent calls to instrumented
functions may use regions of the stack with stale poison, resulting in
(spurious) KASAN splats to the console.

To avoid this, we must clear stale poison from the stack prior to
instrumented functions being called. This patch adds functions to the
KASAN core for removing poison from (portions of) a task's stack. These
will be used by subsequent patches to avoid problems with hotplug and
idle.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 include/linux/kasan.h |  6 +++++-
 mm/kasan/kasan.c      | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 4b9f85c..0fdc798 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -1,6 +1,7 @@
 #ifndef _LINUX_KASAN_H
 #define _LINUX_KASAN_H
 
+#include <linux/sched.h>
 #include <linux/types.h>
 
 struct kmem_cache;
@@ -13,7 +14,6 @@ struct vm_struct;
 
 #include <asm/kasan.h>
 #include <asm/pgtable.h>
-#include <linux/sched.h>
 
 extern unsigned char kasan_zero_page[PAGE_SIZE];
 extern pte_t kasan_zero_pte[PTRS_PER_PTE];
@@ -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 *task);
+
 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 *task) {}
+
 static inline void kasan_enable_current(void) {}
 static inline void kasan_disable_current(void) {}
 
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index bc0a8d8..76cf26cc 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/kmemleak.h>
+#include <linux/linkage.h>
 #include <linux/memblock.h>
 #include <linux/memory.h>
 #include <linux/mm.h>
@@ -60,6 +61,25 @@ void kasan_unpoison_shadow(const void *address, size_t size)
 	}
 }
 
+static void __kasan_unpoison_stack(struct task_struct *task, void *sp)
+{
+	void *base = task_thread_info(task) + 1;
+	size_t size = sp - base;
+
+	kasan_unpoison_shadow(base, size);
+}
+
+/* Unpoison the entire stack for a task. */
+void kasan_unpoison_task_stack(struct task_struct *task)
+{
+	__kasan_unpoison_stack(task, task_stack_page(task) + THREAD_SIZE);
+}
+
+/* Unpoison the stack for the current task beyond a watermark sp value. */
+asmlinkage void kasan_unpoison_remaining_stack(void *sp)
+{
+	__kasan_unpoison_stack(current, sp);
+}
 
 /*
  * All functions below always inlined so compiler could
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] sched/kasan: remove stale KASAN poison after hotplug
  2016-03-02 14:26 [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Mark Rutland
  2016-03-02 14:26 ` [PATCH 1/3] kasan: add functions to clear stack poison Mark Rutland
@ 2016-03-02 14:26 ` Mark Rutland
  2016-03-02 14:26 ` [PATCH 3/3] arm64: kasan: clear stale stack poison Mark Rutland
  2016-03-03 12:02 ` [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Ingo Molnar
  3 siblings, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2016-03-02 14:26 UTC (permalink / raw)
  To: linux-kernel, linux-arch, linux-arm-kernel
  Cc: akpm, aryabinin, catalin.marinas, glider, lorenzo.pieralisi,
	mark.rutland, mingo, peterz, will.deacon

Functions which the compiler has instrumented for ASAN place poison on
the stack shadow upon entry and remove this poison prior to returning.

In the case of CPU hotplug, CPUs exit the kernel a number of levels deep
in C code. Any instrumented functions on this critical path will leave
portions of the stack shadow poisoned.

When a CPU is subsequently brought back into the kernel via a different
path, depending on stackframe, layout calls to instrumented functions
may hit this stale poison, resulting in (spurious) KASAN splats to the
console.

To avoid this, clear any stale poison from the idle thread for a CPU
prior to bringing a CPU online.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 kernel/sched/core.c | 3 +++
 1 file changed, 3 insertions(+)

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] arm64: kasan: clear stale stack poison
  2016-03-02 14:26 [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Mark Rutland
  2016-03-02 14:26 ` [PATCH 1/3] kasan: add functions to clear stack poison Mark Rutland
  2016-03-02 14:26 ` [PATCH 2/3] sched/kasan: remove stale KASAN poison after hotplug Mark Rutland
@ 2016-03-02 14:26 ` Mark Rutland
  2016-03-03 14:14   ` Mark Rutland
  2016-03-03 12:02 ` [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Ingo Molnar
  3 siblings, 1 reply; 13+ messages in thread
From: Mark Rutland @ 2016-03-02 14:26 UTC (permalink / raw)
  To: linux-kernel, linux-arch, linux-arm-kernel
  Cc: akpm, aryabinin, catalin.marinas, glider, lorenzo.pieralisi,
	mark.rutland, mingo, peterz, will.deacon

Functions which the compiler has instrumented for ASAN place poison on
the stack shadow upon entry and remove this poison prior to returning.

In the case of cpuidle, CPUs exit the kernel a number of levels deep
in C code. Any instrumented functions on this critical path will leave
portions of the stack shadow poisoned.

If CPUs lose context and return to the kernel via a cold path, we
restore a prior context saved in __cpu_suspend_enter are forgotten, and
we never remove the poison they placed in the stack shadow area by
functions calls between this and the actual exit of the kernel.

Thus, (depending on stackframe layout) subsequent calls to instrumented
functions may hit this stale poison, resulting in (spurious) KASAN
splats to the console.

To avoid this, clear any stale poison from the idle thread for a CPU
prior to bringing a CPU online.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/sleep.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index e33fe33..fd10eb6 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -145,6 +145,10 @@ ENTRY(cpu_resume_mmu)
 ENDPROC(cpu_resume_mmu)
 	.popsection
 cpu_resume_after_mmu:
+#ifdef CONFIG_KASAN
+	mov	x0, sp
+	bl	kasan_unpoison_remaining_stack
+#endif
 	mov	x0, #0			// return zero on success
 	ldp	x19, x20, [sp, #16]
 	ldp	x21, x22, [sp, #32]
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
  2016-03-02 14:26 [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Mark Rutland
                   ` (2 preceding siblings ...)
  2016-03-02 14:26 ` [PATCH 3/3] arm64: kasan: clear stale stack poison Mark Rutland
@ 2016-03-03 12:02 ` Ingo Molnar
  2016-03-03 12:38   ` Mark Rutland
  3 siblings, 1 reply; 13+ messages in thread
From: Ingo Molnar @ 2016-03-03 12:02 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-kernel, linux-arch, linux-arm-kernel, akpm, aryabinin,
	catalin.marinas, glider, lorenzo.pieralisi, mingo, peterz,
	will.deacon


* Mark Rutland <mark.rutland@arm.com> wrote:

> Functions which the compiler has instrumented for ASAN place poison on
> the stack shadow upon entry and remove this poison prior to returning.
> 
> In some cases (e.g. hotplug and idle), CPUs may exit the kernel a number
> of levels deep in C code. If there are any instrumented functions on
> this critical path, these will leave portions of the idle thread stack
> shadow poisoned.
> 
> If a CPU returns to the kernel via a different path (e.g. a cold entry),
> then depending on stack frame layout subsequent calls to instrumented
> functions may use regions of the stack with stale poison, resulting in
> (spurious) KASAN splats to the console.
> 
> Contemporary GCCs always add stack shadow poisoning when ASAN is
> enabled, even when asked to not instrument a function [1], so we can't
> simply annotate functions on the critical path to avoid poisoning.
> 
> Instead, this series explicitly removes any stale poison before it can
> be hit. In the common hotplug case we clear the entire stack shadow in
> common code, before a CPU is brought online.
> 
> On architectures which perform a cold return as part of cpu idle may
> retain an architecture-specific amount of stack contents. To retain the
> poison for this retained context, the arch code must call the core KASAN
> code, passing a "watermark" stack pointer value beyond which shadow will
> be cleared. Architectures which don't perform a cold return as part of
> idle do not need any additional code.
> 
> This is a combination of previous approaches [2,3], attempting to keep
> as much as possible generic.
> 
> Thanks,
> Mark.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69863
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/409466.html
> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/411850.html
> 
> Mark Rutland (3):
>   kasan: add functions to clear stack poison
>   sched/kasan: remove stale KASAN poison after hotplug
>   arm64: kasan: clear stale stack poison
> 
>  arch/arm64/kernel/sleep.S |  4 ++++
>  include/linux/kasan.h     |  6 +++++-
>  kernel/sched/core.c       |  3 +++
>  mm/kasan/kasan.c          | 20 ++++++++++++++++++++
>  4 files changed, 32 insertions(+), 1 deletion(-)

Looks good to me - via which tree would you like to see this merged upstream?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
  2016-03-03 12:02 ` [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Ingo Molnar
@ 2016-03-03 12:38   ` Mark Rutland
  2016-03-03 12:44     ` Ingo Molnar
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Mark Rutland @ 2016-03-03 12:38 UTC (permalink / raw)
  To: Ingo Molnar, akpm, aryabinin, catalin.marinas
  Cc: linux-kernel, linux-arch, linux-arm-kernel, glider,
	lorenzo.pieralisi, mingo, peterz, will.deacon

On Thu, Mar 03, 2016 at 01:02:27PM +0100, Ingo Molnar wrote:
> 
> * Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Functions which the compiler has instrumented for ASAN place poison on
> > the stack shadow upon entry and remove this poison prior to returning.
> > 
> > In some cases (e.g. hotplug and idle), CPUs may exit the kernel a number
> > of levels deep in C code. If there are any instrumented functions on
> > this critical path, these will leave portions of the idle thread stack
> > shadow poisoned.
> > 
> > If a CPU returns to the kernel via a different path (e.g. a cold entry),
> > then depending on stack frame layout subsequent calls to instrumented
> > functions may use regions of the stack with stale poison, resulting in
> > (spurious) KASAN splats to the console.
> > 
> > Contemporary GCCs always add stack shadow poisoning when ASAN is
> > enabled, even when asked to not instrument a function [1], so we can't
> > simply annotate functions on the critical path to avoid poisoning.
> > 
> > Instead, this series explicitly removes any stale poison before it can
> > be hit. In the common hotplug case we clear the entire stack shadow in
> > common code, before a CPU is brought online.
> > 
> > On architectures which perform a cold return as part of cpu idle may
> > retain an architecture-specific amount of stack contents. To retain the
> > poison for this retained context, the arch code must call the core KASAN
> > code, passing a "watermark" stack pointer value beyond which shadow will
> > be cleared. Architectures which don't perform a cold return as part of
> > idle do not need any additional code.
> > 
> > This is a combination of previous approaches [2,3], attempting to keep
> > as much as possible generic.
> > 
> > Thanks,
> > Mark.
> > 
> > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69863
> > [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/409466.html
> > [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/411850.html
> > 
> > Mark Rutland (3):
> >   kasan: add functions to clear stack poison
> >   sched/kasan: remove stale KASAN poison after hotplug
> >   arm64: kasan: clear stale stack poison
> > 
> >  arch/arm64/kernel/sleep.S |  4 ++++
> >  include/linux/kasan.h     |  6 +++++-
> >  kernel/sched/core.c       |  3 +++
> >  mm/kasan/kasan.c          | 20 ++++++++++++++++++++
> >  4 files changed, 32 insertions(+), 1 deletion(-)
> 
> Looks good to me - via which tree would you like to see this merged upstream?

I'd prefer the arm64 tree as arm64 is (the most) affected by the issue
in practice.

I'm happy for this to go via another tree if that's simpler; I'm not
aware of anything that's likely to conflict in the arm64 tree.

Catalin, Andrey, Andrew, any preference?

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
  2016-03-03 12:38   ` Mark Rutland
@ 2016-03-03 12:44     ` Ingo Molnar
  2016-03-03 14:30     ` Andrey Ryabinin
  2016-03-03 16:11     ` Catalin Marinas
  2 siblings, 0 replies; 13+ messages in thread
From: Ingo Molnar @ 2016-03-03 12:44 UTC (permalink / raw)
  To: Mark Rutland
  Cc: akpm, aryabinin, catalin.marinas, linux-kernel, linux-arch,
	linux-arm-kernel, glider, lorenzo.pieralisi, mingo, peterz,
	will.deacon


* Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Mar 03, 2016 at 01:02:27PM +0100, Ingo Molnar wrote:
> > 
> > * Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > Functions which the compiler has instrumented for ASAN place poison on
> > > the stack shadow upon entry and remove this poison prior to returning.
> > > 
> > > In some cases (e.g. hotplug and idle), CPUs may exit the kernel a number
> > > of levels deep in C code. If there are any instrumented functions on
> > > this critical path, these will leave portions of the idle thread stack
> > > shadow poisoned.
> > > 
> > > If a CPU returns to the kernel via a different path (e.g. a cold entry),
> > > then depending on stack frame layout subsequent calls to instrumented
> > > functions may use regions of the stack with stale poison, resulting in
> > > (spurious) KASAN splats to the console.
> > > 
> > > Contemporary GCCs always add stack shadow poisoning when ASAN is
> > > enabled, even when asked to not instrument a function [1], so we can't
> > > simply annotate functions on the critical path to avoid poisoning.
> > > 
> > > Instead, this series explicitly removes any stale poison before it can
> > > be hit. In the common hotplug case we clear the entire stack shadow in
> > > common code, before a CPU is brought online.
> > > 
> > > On architectures which perform a cold return as part of cpu idle may
> > > retain an architecture-specific amount of stack contents. To retain the
> > > poison for this retained context, the arch code must call the core KASAN
> > > code, passing a "watermark" stack pointer value beyond which shadow will
> > > be cleared. Architectures which don't perform a cold return as part of
> > > idle do not need any additional code.
> > > 
> > > This is a combination of previous approaches [2,3], attempting to keep
> > > as much as possible generic.
> > > 
> > > Thanks,
> > > Mark.
> > > 
> > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69863
> > > [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/409466.html
> > > [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/411850.html
> > > 
> > > Mark Rutland (3):
> > >   kasan: add functions to clear stack poison
> > >   sched/kasan: remove stale KASAN poison after hotplug
> > >   arm64: kasan: clear stale stack poison
> > > 
> > >  arch/arm64/kernel/sleep.S |  4 ++++
> > >  include/linux/kasan.h     |  6 +++++-
> > >  kernel/sched/core.c       |  3 +++
> > >  mm/kasan/kasan.c          | 20 ++++++++++++++++++++
> > >  4 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > Looks good to me - via which tree would you like to see this merged upstream?
> 
> I'd prefer the arm64 tree as arm64 is (the most) affected by the issue
> in practice.
> 
> I'm happy for this to go via another tree if that's simpler; I'm not
> aware of anything that's likely to conflict in the arm64 tree.
> 
> Catalin, Andrey, Andrew, any preference?

Ok, for the scheduler bits:

  Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] arm64: kasan: clear stale stack poison
  2016-03-02 14:26 ` [PATCH 3/3] arm64: kasan: clear stale stack poison Mark Rutland
@ 2016-03-03 14:14   ` Mark Rutland
  2016-03-03 14:32     ` Lorenzo Pieralisi
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Rutland @ 2016-03-03 14:14 UTC (permalink / raw)
  To: linux-kernel, linux-arch, linux-arm-kernel, lorenzo.pieralisi
  Cc: akpm, aryabinin, catalin.marinas, glider, mingo, peterz, will.deacon

On Wed, Mar 02, 2016 at 02:26:18PM +0000, Mark Rutland wrote:
> Functions which the compiler has instrumented for ASAN place poison on
> the stack shadow upon entry and remove this poison prior to returning.
> 
> In the case of cpuidle, CPUs exit the kernel a number of levels deep
> in C code. Any instrumented functions on this critical path will leave
> portions of the stack shadow poisoned.
> 
> If CPUs lose context and return to the kernel via a cold path, we
> restore a prior context saved in __cpu_suspend_enter are forgotten, and
> we never remove the poison they placed in the stack shadow area by
> functions calls between this and the actual exit of the kernel.
> 
> Thus, (depending on stackframe layout) subsequent calls to instrumented
> functions may hit this stale poison, resulting in (spurious) KASAN
> splats to the console.
> 
> To avoid this, clear any stale poison from the idle thread for a CPU
> prior to bringing a CPU online.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/sleep.S | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
> index e33fe33..fd10eb6 100644
> --- a/arch/arm64/kernel/sleep.S
> +++ b/arch/arm64/kernel/sleep.S
> @@ -145,6 +145,10 @@ ENTRY(cpu_resume_mmu)
>  ENDPROC(cpu_resume_mmu)
>  	.popsection
>  cpu_resume_after_mmu:
> +#ifdef CONFIG_KASAN
> +	mov	x0, sp
> +	bl	kasan_unpoison_remaining_stack
> +#endif

Lorenzo, as this was following your suggestion [1], I hope that this
patch looks ok to you?

Are you happy to provide an Ack / Reviewed-by?

Thanks,
Mark.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
  2016-03-03 12:38   ` Mark Rutland
  2016-03-03 12:44     ` Ingo Molnar
@ 2016-03-03 14:30     ` Andrey Ryabinin
  2016-03-03 14:49       ` Mark Rutland
  2016-03-03 16:11     ` Catalin Marinas
  2 siblings, 1 reply; 13+ messages in thread
From: Andrey Ryabinin @ 2016-03-03 14:30 UTC (permalink / raw)
  To: Mark Rutland, Ingo Molnar, akpm, catalin.marinas
  Cc: linux-kernel, linux-arch, linux-arm-kernel, glider,
	lorenzo.pieralisi, mingo, peterz, will.deacon

On 03/03/2016 03:38 PM, Mark Rutland wrote:
> On Thu, Mar 03, 2016 at 01:02:27PM +0100, Ingo Molnar wrote:
>>>
>>> Mark Rutland (3):
>>>   kasan: add functions to clear stack poison
>>>   sched/kasan: remove stale KASAN poison after hotplug
>>>   arm64: kasan: clear stale stack poison
>>>
>>>  arch/arm64/kernel/sleep.S |  4 ++++
>>>  include/linux/kasan.h     |  6 +++++-
>>>  kernel/sched/core.c       |  3 +++
>>>  mm/kasan/kasan.c          | 20 ++++++++++++++++++++
>>>  4 files changed, 32 insertions(+), 1 deletion(-)
>>
>> Looks good to me - via which tree would you like to see this merged upstream?
> 
> I'd prefer the arm64 tree as arm64 is (the most) affected by the issue
> in practice.
> 
> I'm happy for this to go via another tree if that's simpler; I'm not
> aware of anything that's likely to conflict in the arm64 tree.
> 
> Catalin, Andrey, Andrew, any preference?
> 

I don't have any. arm64 tree is fine by me.

For the patchset:

	Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] arm64: kasan: clear stale stack poison
  2016-03-03 14:14   ` Mark Rutland
@ 2016-03-03 14:32     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 13+ messages in thread
From: Lorenzo Pieralisi @ 2016-03-03 14:32 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-kernel, linux-arch, linux-arm-kernel, akpm, aryabinin,
	catalin.marinas, glider, mingo, peterz, will.deacon

On Thu, Mar 03, 2016 at 02:14:29PM +0000, Mark Rutland wrote:
> On Wed, Mar 02, 2016 at 02:26:18PM +0000, Mark Rutland wrote:
> > Functions which the compiler has instrumented for ASAN place poison on
> > the stack shadow upon entry and remove this poison prior to returning.
> > 
> > In the case of cpuidle, CPUs exit the kernel a number of levels deep
> > in C code. Any instrumented functions on this critical path will leave
> > portions of the stack shadow poisoned.
> > 
> > If CPUs lose context and return to the kernel via a cold path, we
> > restore a prior context saved in __cpu_suspend_enter are forgotten, and
> > we never remove the poison they placed in the stack shadow area by
> > functions calls between this and the actual exit of the kernel.
> > 
> > Thus, (depending on stackframe layout) subsequent calls to instrumented
> > functions may hit this stale poison, resulting in (spurious) KASAN
> > splats to the console.
> > 
> > To avoid this, clear any stale poison from the idle thread for a CPU
> > prior to bringing a CPU online.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Alexander Potapenko <glider@google.com>
> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm64/kernel/sleep.S | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
> > index e33fe33..fd10eb6 100644
> > --- a/arch/arm64/kernel/sleep.S
> > +++ b/arch/arm64/kernel/sleep.S
> > @@ -145,6 +145,10 @@ ENTRY(cpu_resume_mmu)
> >  ENDPROC(cpu_resume_mmu)
> >  	.popsection
> >  cpu_resume_after_mmu:
> > +#ifdef CONFIG_KASAN
> > +	mov	x0, sp
> > +	bl	kasan_unpoison_remaining_stack
> > +#endif
> 
> Lorenzo, as this was following your suggestion [1], I hope that this
> patch looks ok to you?
> 
> Are you happy to provide an Ack / Reviewed-by?

Yes sure, thanks for putting it together:

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
  2016-03-03 14:30     ` Andrey Ryabinin
@ 2016-03-03 14:49       ` Mark Rutland
  2016-03-03 14:53         ` Andrey Ryabinin
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Rutland @ 2016-03-03 14:49 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Ingo Molnar, akpm, catalin.marinas, linux-kernel, linux-arch,
	linux-arm-kernel, glider, lorenzo.pieralisi, mingo, peterz,
	will.deacon

On Thu, Mar 03, 2016 at 05:30:17PM +0300, Andrey Ryabinin wrote:
> On 03/03/2016 03:38 PM, Mark Rutland wrote:
> > On Thu, Mar 03, 2016 at 01:02:27PM +0100, Ingo Molnar wrote:
> >>>
> >>> Mark Rutland (3):
> >>>   kasan: add functions to clear stack poison
> >>>   sched/kasan: remove stale KASAN poison after hotplug
> >>>   arm64: kasan: clear stale stack poison
> >>>
> >>>  arch/arm64/kernel/sleep.S |  4 ++++
> >>>  include/linux/kasan.h     |  6 +++++-
> >>>  kernel/sched/core.c       |  3 +++
> >>>  mm/kasan/kasan.c          | 20 ++++++++++++++++++++
> >>>  4 files changed, 32 insertions(+), 1 deletion(-)
> >>
> >> Looks good to me - via which tree would you like to see this merged upstream?
> > 
> > I'd prefer the arm64 tree as arm64 is (the most) affected by the issue
> > in practice.
> > 
> > I'm happy for this to go via another tree if that's simpler; I'm not
> > aware of anything that's likely to conflict in the arm64 tree.
> > 
> > Catalin, Andrey, Andrew, any preference?
> > 
> 
> I don't have any. arm64 tree is fine by me.
> 
> For the patchset:
> 
> 	Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> 

Cheers!

Following [1], I intend to change patch 1 to start at task_stack_page(t)
rather than task_thread_info(task) + 1, to keep things simple.

I assume that your Reviewed-by would still apply in that case?

Thanks,
Mark.

[1] https://lkml.org/lkml/2016/3/2/428

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
  2016-03-03 14:49       ` Mark Rutland
@ 2016-03-03 14:53         ` Andrey Ryabinin
  0 siblings, 0 replies; 13+ messages in thread
From: Andrey Ryabinin @ 2016-03-03 14:53 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Ingo Molnar, akpm, catalin.marinas, linux-kernel, linux-arch,
	linux-arm-kernel, glider, lorenzo.pieralisi, mingo, peterz,
	will.deacon



On 03/03/2016 05:49 PM, Mark Rutland wrote:
> On Thu, Mar 03, 2016 at 05:30:17PM +0300, Andrey Ryabinin wrote:
>> On 03/03/2016 03:38 PM, Mark Rutland wrote:
>>> On Thu, Mar 03, 2016 at 01:02:27PM +0100, Ingo Molnar wrote:
>>>>>
>>>>> Mark Rutland (3):
>>>>>   kasan: add functions to clear stack poison
>>>>>   sched/kasan: remove stale KASAN poison after hotplug
>>>>>   arm64: kasan: clear stale stack poison
>>>>>
>>>>>  arch/arm64/kernel/sleep.S |  4 ++++
>>>>>  include/linux/kasan.h     |  6 +++++-
>>>>>  kernel/sched/core.c       |  3 +++
>>>>>  mm/kasan/kasan.c          | 20 ++++++++++++++++++++
>>>>>  4 files changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> Looks good to me - via which tree would you like to see this merged upstream?
>>>
>>> I'd prefer the arm64 tree as arm64 is (the most) affected by the issue
>>> in practice.
>>>
>>> I'm happy for this to go via another tree if that's simpler; I'm not
>>> aware of anything that's likely to conflict in the arm64 tree.
>>>
>>> Catalin, Andrey, Andrew, any preference?
>>>
>>
>> I don't have any. arm64 tree is fine by me.
>>
>> For the patchset:
>>
>> 	Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>
> 
> Cheers!
> 
> Following [1], I intend to change patch 1 to start at task_stack_page(t)
> rather than task_thread_info(task) + 1, to keep things simple.
> 
> I assume that your Reviewed-by would still apply in that case?
> 

Sure.

> Thanks,
> Mark.
> 
> [1] https://lkml.org/lkml/2016/3/2/428
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel
  2016-03-03 12:38   ` Mark Rutland
  2016-03-03 12:44     ` Ingo Molnar
  2016-03-03 14:30     ` Andrey Ryabinin
@ 2016-03-03 16:11     ` Catalin Marinas
  2 siblings, 0 replies; 13+ messages in thread
From: Catalin Marinas @ 2016-03-03 16:11 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Ingo Molnar, akpm, aryabinin, linux-arch, lorenzo.pieralisi,
	peterz, will.deacon, linux-kernel, mingo, glider,
	linux-arm-kernel

On Thu, Mar 03, 2016 at 12:38:09PM +0000, Mark Rutland wrote:
> On Thu, Mar 03, 2016 at 01:02:27PM +0100, Ingo Molnar wrote:
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > Mark Rutland (3):
> > >   kasan: add functions to clear stack poison
> > >   sched/kasan: remove stale KASAN poison after hotplug
> > >   arm64: kasan: clear stale stack poison
> > > 
> > >  arch/arm64/kernel/sleep.S |  4 ++++
> > >  include/linux/kasan.h     |  6 +++++-
> > >  kernel/sched/core.c       |  3 +++
> > >  mm/kasan/kasan.c          | 20 ++++++++++++++++++++
> > >  4 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > Looks good to me - via which tree would you like to see this merged upstream?
> 
> I'd prefer the arm64 tree as arm64 is (the most) affected by the issue
> in practice.
> 
> I'm happy for this to go via another tree if that's simpler; I'm not
> aware of anything that's likely to conflict in the arm64 tree.
> 
> Catalin, Andrey, Andrew, any preference?

I'm happy for this to go via the Andrew's -mm tree. For the series:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Please report the series to linux-mm@kvack.org with the corresponding
acks in place and the fix-up on patch 1.

Thanks.

-- 
Catalin

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-03-03 16:11 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-02 14:26 [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Mark Rutland
2016-03-02 14:26 ` [PATCH 1/3] kasan: add functions to clear stack poison Mark Rutland
2016-03-02 14:26 ` [PATCH 2/3] sched/kasan: remove stale KASAN poison after hotplug Mark Rutland
2016-03-02 14:26 ` [PATCH 3/3] arm64: kasan: clear stale stack poison Mark Rutland
2016-03-03 14:14   ` Mark Rutland
2016-03-03 14:32     ` Lorenzo Pieralisi
2016-03-03 12:02 ` [PATCH 0/3] KASAN: clean stale poison upon cold re-entry to kernel Ingo Molnar
2016-03-03 12:38   ` Mark Rutland
2016-03-03 12:44     ` Ingo Molnar
2016-03-03 14:30     ` Andrey Ryabinin
2016-03-03 14:49       ` Mark Rutland
2016-03-03 14:53         ` Andrey Ryabinin
2016-03-03 16:11     ` Catalin Marinas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).