On Fri, 2018-10-19 at 16:59 +1100, Michael Ellerman wrote:
> Recently in commit 7241d26e8175 ("powerpc/64: properly initialise
> the stackprotector canary on SMP.") we fixed a crash with stack
> protector on SMP by initialising the stack canary in
> cpu_idle_thread_init().
>
> But this can also causes crashes, when a CPU comes back online after
> being offline:
>
> Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in:
> pnv_smp_cpu_kill_self+0x2a0/0x2b0
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted
> 4.19.0-rc3-gcc-7.3.1-00168-g4ffe713b7587 #94
> Call Trace:
> dump_stack+0xb0/0xf4 (unreliable)
> panic+0x144/0x328
> __stack_chk_fail+0x2c/0x30
> pnv_smp_cpu_kill_self+0x2a0/0x2b0
> cpu_die+0x48/0x70
> arch_cpu_idle_dead+0x20/0x40
> do_idle+0x274/0x390
> cpu_startup_entry+0x38/0x50
> start_secondary+0x5e4/0x600
> start_secondary_prolog+0x10/0x14
>
> Looking at the stack we see that the canary value in the stack frame
> doesn't match the canary in the task/paca. That is because we have
> reinitialised the task/paca value, but then the CPU coming online has
> returned into a function using the old canary value. That causes the
> comparison to fail.
>
> Instead we can call boot_init_stack_canary() from start_secondary()
> which never returns. This is essentially what the generic code does in
> cpu_startup_entry() under #ifdef X86, we should make that non-x86
> specific in a future patch.
>
> Fixes: 7241d26e8175 ("powerpc/64: properly initialise the stackprotector
> canary on SMP.")
> Reported-by: Joel Stanley <j...@jms.id.au>
> Signed-off-by: Michael Ellerman <m...@ellerman.id.au>
> ---
> arch/powerpc/kernel/smp.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 8e3a5da24d59..951c476faffc 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -61,6 +61,7 @@
> #include <asm/asm-prototypes.h>
> #include <asm/cpu_has_feature.h>
> #include <asm/ftrace.h>
> +#include <asm/stackprotector.h>
>
> #ifdef DEBUG
> #include <asm/udbg.h>
> @@ -1014,16 +1015,9 @@ static void cpu_idle_thread_init(unsigned int cpu,
> struct task_struct *idle)
> {
> struct thread_info *ti = task_thread_info(idle);
>
> -#ifdef CONFIG_STACKPROTECTOR
> - idle->stack_canary = get_random_canary();
> -#endif
> -
> #ifdef CONFIG_PPC64
> paca_ptrs[cpu]->__current = idle;
> paca_ptrs[cpu]->kstack = (unsigned long)ti + THREAD_SIZE -
> STACK_FRAME_OVERHEAD;
> -#ifdef CONFIG_STACKPROTECTOR
> - paca_ptrs[cpu]->canary = idle->stack_canary;
> -#endif
> #endif
> ti->cpu = cpu;
> secondary_ti = current_set[cpu] = ti;
> @@ -1316,6 +1310,8 @@ void start_secondary(void *unused)
> notify_cpu_starting(cpu);
> set_cpu_online(cpu, true);
>
> + boot_init_stack_canary();
> +
> local_irq_enable();
>
> /* We can enable ftrace for secondary cpus now */
Tested-by: Abdul Haleem <abdha...@linux.vnet.ibm.com>
--
Regard's
Abdul Haleem
IBM Linux Technology Centre
