Re: [PATCH v5 2/2] powerpc/64: Prevent stack protection in early boot

[Michael Ellerman <mpe@ellerman.id.au>]

Daniel Axtens <dja@axtens.net> writes:
> Michael Ellerman <mpe@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.

The key details for this bug are that 1) some functions get stack
protection, if they have on-stack buffers etc. 2) that stack protection
involves reading a canary from the paca.


> I have checked and I can find no other C functions that are called
> before early_setup.

Thanks.

Except for all of prom_init.c but that's already built with no stack
protector.

> 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

No I don't think so.

Tracing is less of a concern during very early boot because although the
functions may be built to support tracing, you can't actually turn
tracing *on* that early.

Also setup_64.c is not purely early boot code, there are some functions
in there we would like to be able to trace.

As I was saying the other day, we may want to create a specific
directory (or file) for all the really early boot code where we turn off
all special options like tracing, kcov, stack protector etc.

cheers