Michael Ellerman <m...@ellerman.id.au> writes:
> The previous commit reduced the amount of code that is run before we
> setup a paca. However there are still a few remaining functions that
> run with no paca, or worse, with an arbitrary value in r13 that will
> be used as a paca pointer.
>
> In particular the stack protector canary is stored in the paca, so if
> stack protector is activated for any of these functions we will read
> the stack canary from wherever r13 points. If r13 happens to point
> outside of memory we will get a machine check / checkstop.
>
> For example if we modify initialise_paca() to trigger stack
> protection, and then boot in the mambo simulator with r13 poisoned in
> skiboot before calling the kernel:
>
> DEBUG: 19952232: (19952232): INSTRUCTION: PC=0xC0000000191FC1E8:
> [0x3C4C006D]: addis r2,r12,0x6D [fetch]
> DEBUG: 19952236: (19952236): INSTRUCTION: PC=0xC00000001807EAD8:
> [0x7D8802A6]: mflr r12 [fetch]
> FATAL ERROR: 19952276: (19952276): Check Stop for 0:0: Machine Check with
> ME bit of MSR off
> DEBUG: 19952276: (19952276): INSTRUCTION: PC=0xC0000000191FCA7C:
> [0xE90D0CF8]: ld r8,0xCF8(r13) [Instruction Failed]
> INFO: 19952276: (19952277): ** Execution stopped: Mambo Error, Machine
> Check Stop, **
> systemsim % bt
> pc: 0xC0000000191FCA7C initialise_paca+0x54
> lr: 0xC0000000191FC22C early_setup+0x44
> stack:0x00000000198CBED0 0x0 +0x0
> stack:0x00000000198CBF00 0xC0000000191FC22C early_setup+0x44
> stack:0x00000000198CBF90 0x1801C968 +0x1801C968
>
> So annotate the relevant functions to ensure stack protection is never
> enabled for them.
This all makes sense to me, although I don't really understand the stack
protector especially well.
I have checked and I can find no other C functions that are called
before early_setup.
Do we need to do add setup_64.c to the part of the Makefile that
disables tracing of early boot?
ifdef CONFIG_FUNCTION_TRACER
# Do not trace early boot code
CFLAGS_REMOVE_cputable.o = $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_prom_init.o = $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_btext.o = $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_prom.o = $(CC_FLAGS_FTRACE)
-> should we add setup_64.c here?
endif
Regards,
Daniel
>
> Fixes: 06ec27aea9fc ("powerpc/64: add stack protector support")
> Cc: sta...@vger.kernel.org # v4.20+
> Signed-off-by: Michael Ellerman <m...@ellerman.id.au>
> ---
> arch/powerpc/kernel/paca.c | 4 ++--
> arch/powerpc/kernel/setup.h | 2 ++
> arch/powerpc/kernel/setup_64.c | 2 +-
> 3 files changed, 5 insertions(+), 3 deletions(-)
>
> v5: New.
>
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 0ee6308541b1..3f91ccaa9c74 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -176,7 +176,7 @@ static struct slb_shadow * __init new_slb_shadow(int cpu,
> unsigned long limit)
> struct paca_struct **paca_ptrs __read_mostly;
> EXPORT_SYMBOL(paca_ptrs);
>
> -void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> +void __init __nostackprotector initialise_paca(struct paca_struct *new_paca,
> int cpu)
> {
> #ifdef CONFIG_PPC_PSERIES
> new_paca->lppaca_ptr = NULL;
> @@ -205,7 +205,7 @@ void __init initialise_paca(struct paca_struct *new_paca,
> int cpu)
> }
>
> /* Put the paca pointer into r13 and SPRG_PACA */
> -void setup_paca(struct paca_struct *new_paca)
> +void __nostackprotector setup_paca(struct paca_struct *new_paca)
> {
> /* Setup r13 */
> local_paca = new_paca;
> diff --git a/arch/powerpc/kernel/setup.h b/arch/powerpc/kernel/setup.h
> index 2dd0d9cb5a20..d210671026e9 100644
> --- a/arch/powerpc/kernel/setup.h
> +++ b/arch/powerpc/kernel/setup.h
> @@ -8,6 +8,8 @@
> #ifndef __ARCH_POWERPC_KERNEL_SETUP_H
> #define __ARCH_POWERPC_KERNEL_SETUP_H
>
> +#define __nostackprotector
> __attribute__((__optimize__("no-stack-protector")))
> +
> void initialize_cache_info(void);
> void irqstack_early_init(void);
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 17886d147dd0..438a9befce41 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -279,7 +279,7 @@ void __init record_spr_defaults(void)
> * device-tree is not accessible via normal means at this point.
> */
>
> -void __init early_setup(unsigned long dt_ptr)
> +void __init __nostackprotector early_setup(unsigned long dt_ptr)
> {
> static __initdata struct paca_struct boot_paca;
>
> --
> 2.24.1
