* [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend. @ 2016-02-26 12:38 Alexander Potapenko 2016-02-26 13:53 ` Mark Rutland 2016-02-27 1:05 ` [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend kbuild test robot 0 siblings, 2 replies; 13+ messages in thread From: Alexander Potapenko @ 2016-02-26 12:38 UTC (permalink / raw) To: adech.fo, dvyukov, ryabinin.a.a, will.deacon, catalin.marinas, mark.rutland, akpm Cc: kasan-dev, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend. 2016-02-26 12:38 [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend Alexander Potapenko @ 2016-02-26 13:53 ` Mark Rutland 2016-02-26 17:28 ` Alexander Potapenko 2016-02-27 1:05 ` [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend kbuild test robot 1 sibling, 1 reply; 13+ messages in thread From: Mark Rutland @ 2016-02-26 13:53 UTC (permalink / raw) To: Alexander Potapenko Cc: adech.fo, dvyukov, ryabinin.a.a, will.deacon, catalin.marinas, akpm, kasan-dev, linux-kernel, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend. 2016-02-26 13:53 ` Mark Rutland @ 2016-02-26 17:28 ` Alexander Potapenko 2016-03-01 19:37 ` Mark Rutland 2016-03-01 19:42 ` Mark Rutland 0 siblings, 2 replies; 13+ messages in thread From: Alexander Potapenko @ 2016-02-26 17:28 UTC (permalink / raw) To: Mark Rutland Cc: Andrey Konovalov, Dmitriy Vyukov, Andrey Ryabinin, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, LKML, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend. 2016-02-26 17:28 ` Alexander Potapenko @ 2016-03-01 19:37 ` Mark Rutland 2016-03-01 19:42 ` Mark Rutland 1 sibling, 0 replies; 13+ messages in thread From: Mark Rutland @ 2016-03-01 19:37 UTC (permalink / raw) To: Alexander Potapenko Cc: Andrey Konovalov, Dmitriy Vyukov, Andrey Ryabinin, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, LKML, linux-arm-kernel 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: ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend. 2016-02-26 17:28 ` Alexander Potapenko 2016-03-01 19:37 ` Mark Rutland @ 2016-03-01 19:42 ` Mark Rutland 2016-03-01 20:05 ` [PATCH] sched/kasan: clear stale stack poison kbuild test robot 2016-03-02 12:53 ` [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend Andrey Ryabinin 1 sibling, 2 replies; 13+ messages in thread From: Mark Rutland @ 2016-03-01 19:42 UTC (permalink / raw) To: Alexander Potapenko Cc: Andrey Konovalov, Dmitriy Vyukov, Andrey Ryabinin, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, LKML, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] sched/kasan: clear stale stack poison 2016-03-01 19:42 ` Mark Rutland @ 2016-03-01 20:05 ` kbuild test robot 2016-03-02 12:53 ` [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend Andrey Ryabinin 1 sibling, 0 replies; 13+ messages in thread From: kbuild test robot @ 2016-03-01 20:05 UTC (permalink / raw) To: Mark Rutland Cc: kbuild-all, Alexander Potapenko, Andrey Konovalov, Dmitriy Vyukov, Andrey Ryabinin, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, LKML, linux-arm-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend. 2016-03-01 19:42 ` Mark Rutland 2016-03-01 20:05 ` [PATCH] sched/kasan: clear stale stack poison kbuild test robot @ 2016-03-02 12:53 ` Andrey Ryabinin 2016-03-02 13:51 ` [PATCH 1/2] cpu, idle: move init_idle() out of idle_thread_get() Andrey Ryabinin 1 sibling, 1 reply; 13+ messages in thread From: Andrey Ryabinin @ 2016-03-02 12:53 UTC (permalink / raw) To: Mark Rutland, Alexander Potapenko Cc: Andrey Konovalov, Dmitriy Vyukov, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, LKML, linux-arm-kernel, Ingo Molnar, Peter Zijlstra, Thomas Gleixner 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] cpu, idle: move init_idle() out of idle_thread_get() 2016-03-02 12:53 ` [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend Andrey Ryabinin @ 2016-03-02 13:51 ` Andrey Ryabinin 2016-03-02 13:51 ` [PATCH 2/2] kasan: unpoison stack of idle task on cpu online Andrey Ryabinin 0 siblings, 1 reply; 13+ messages in thread From: Andrey Ryabinin @ 2016-03-02 13:51 UTC (permalink / raw) To: linux-kernel Cc: Mark Rutland, Alexander Potapenko, Andrey Konovalov, Dmitriy Vyukov, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, Ingo Molnar, Peter Zijlstra, Thomas Gleixner, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] kasan: unpoison stack of idle task on cpu online 2016-03-02 13:51 ` [PATCH 1/2] cpu, idle: move init_idle() out of idle_thread_get() Andrey Ryabinin @ 2016-03-02 13:51 ` Andrey Ryabinin 2016-03-02 14:50 ` Mark Rutland 0 siblings, 1 reply; 13+ messages in thread From: Andrey Ryabinin @ 2016-03-02 13:51 UTC (permalink / raw) To: linux-kernel Cc: Mark Rutland, Alexander Potapenko, Andrey Konovalov, Dmitriy Vyukov, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, Ingo Molnar, Peter Zijlstra, Thomas Gleixner, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online 2016-03-02 13:51 ` [PATCH 2/2] kasan: unpoison stack of idle task on cpu online Andrey Ryabinin @ 2016-03-02 14:50 ` Mark Rutland 2016-03-02 15:27 ` Andrey Ryabinin 0 siblings, 1 reply; 13+ messages in thread From: Mark Rutland @ 2016-03-02 14:50 UTC (permalink / raw) To: Andrey Ryabinin Cc: linux-kernel, Alexander Potapenko, Andrey Konovalov, Dmitriy Vyukov, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, Ingo Molnar, Peter Zijlstra, Thomas Gleixner 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online 2016-03-02 14:50 ` Mark Rutland @ 2016-03-02 15:27 ` Andrey Ryabinin 2016-03-02 15:43 ` Mark Rutland 0 siblings, 1 reply; 13+ messages in thread From: Andrey Ryabinin @ 2016-03-02 15:27 UTC (permalink / raw) To: Mark Rutland Cc: linux-kernel, Alexander Potapenko, Andrey Konovalov, Dmitriy Vyukov, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, Ingo Molnar, Peter Zijlstra, Thomas Gleixner 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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] kasan: unpoison stack of idle task on cpu online 2016-03-02 15:27 ` Andrey Ryabinin @ 2016-03-02 15:43 ` Mark Rutland 0 siblings, 0 replies; 13+ messages in thread From: Mark Rutland @ 2016-03-02 15:43 UTC (permalink / raw) To: Andrey Ryabinin Cc: linux-kernel, Alexander Potapenko, Andrey Konovalov, Dmitriy Vyukov, will.deacon, catalin.marinas, Andrew Morton, kasan-dev, Ingo Molnar, Peter Zijlstra, Thomas Gleixner 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend. 2016-02-26 12:38 [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend Alexander Potapenko 2016-02-26 13:53 ` Mark Rutland @ 2016-02-27 1:05 ` kbuild test robot 1 sibling, 0 replies; 13+ messages in thread From: kbuild test robot @ 2016-02-27 1:05 UTC (permalink / raw) To: Alexander Potapenko Cc: kbuild-all, adech.fo, dvyukov, ryabinin.a.a, will.deacon, catalin.marinas, mark.rutland, akpm, kasan-dev, linux-kernel, linux-arm-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-03-02 15:44 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-26 12:38 [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend Alexander Potapenko 2016-02-26 13:53 ` Mark Rutland 2016-02-26 17:28 ` Alexander Potapenko 2016-03-01 19:37 ` Mark Rutland 2016-03-01 19:42 ` Mark Rutland 2016-03-01 20:05 ` [PATCH] sched/kasan: clear stale stack poison kbuild test robot 2016-03-02 12:53 ` [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend Andrey Ryabinin 2016-03-02 13:51 ` [PATCH 1/2] cpu, idle: move init_idle() out of idle_thread_get() Andrey Ryabinin 2016-03-02 13:51 ` [PATCH 2/2] kasan: unpoison stack of idle task on cpu online Andrey Ryabinin 2016-03-02 14:50 ` Mark Rutland 2016-03-02 15:27 ` Andrey Ryabinin 2016-03-02 15:43 ` Mark Rutland 2016-02-27 1:05 ` [PATCH v1] kasan, arm64: Unpoison dirty stack frames when resuming from suspend kbuild test robot
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).