[U-Boot] [PATCH 04/10] ARM: allocate extra space for PSCI stack in secure section during link phase

[Marc Zyngier]

On 24/05/16 11:21, Marc Zyngier wrote:
> On 23/05/16 13:41, Chen-Yu Tsai wrote:
>> The PSCI implementation expects at most 2 pages worth of space reserved
>> at the end of the secure section for its stacks. This was not properly
>> marked and taken into consideration when reserving memory from the
>> kernel.
>>
>> If one accesses PSCI after Linux has fully booted, the memory that should
>> have been reserved for the PSCI stacks may have been used by the kernel
>> or userspace, and would be corrupted. Observed after effects include the
>> system hanging or telinit core dumping when trying to reboot. It seems
>> the init process gets hit the most on my test bed.
>>
>> This fix is only a stop gap. It would be better to rework the stack
>> allocation mechanism, maybe with proper usage of CONFIG_ macros and an
>> explicit symbol.
>>
>> Signed-off-by: Chen-Yu Tsai <wens at csie.org>
>> ---
>>  arch/arm/cpu/u-boot.lds | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>> index cfab8b041234..c7f37b606ad5 100644
>> --- a/arch/arm/cpu/u-boot.lds
>> +++ b/arch/arm/cpu/u-boot.lds
>> @@ -67,6 +67,9 @@ SECTIONS
>>  		SIZEOF(.__secure_start) +
>>  		SIZEOF(.secure_text);
>>  
>> +	/* Align to page boundary and skip 2 pages */
>> +	. = (. & ~ 0xfff) + 0x2000;
>> +
>>  	__secure_end_lma = .;
>>  	.__secure_end : AT(__secure_end_lma) {
>>  		*(.__secure_end)
>>
> 
> Something worries me here. The PSCI stacks are on the secure side (in
> your case in SRAM), and shouldn't be part of the u-boot binary. If Linux
> sees some corruption, that's because you're not putting the stacks where
> they should, and that's where the issue is.
> 
> One possible bug would be if like the stack address computing is done
> using absolute addresses from one of the labels, and not using
> PC-relative addresses.
> 
> And crucially, this:
> 
> +	ldr	r3, =psci_text_end		@ end of monitor text
> 
> which was introduced by 4c681a3d22f0 ("ARM: Factor out reusable
> psci_get_cpu_stack_top").
> 
> Unless you actually relocate this value, this will base your stack in
> RAM, corrupting the hell out of the whatever is there, and moving the
> goalpost by 8kB is just papering over the issue.
> 
> The original code was:
> 
> +       adr     r5, text_end            @ end of text
> +       add     r5, r5, #0x2000         @ Skip two pages
> +       lsr     r5, r5, #12             @ Align to start of page
> +       lsl     r5, r5, #12
> +       sub     sp, r5, r4              @ here's our stack!
> 
> which had its own share of bug, but was actually safe, thanks to the use
> of 'adr' and not 'ldr'.
> 
> Can you please check whether this value gets relocated?

I had a check by building a semi-recent u-boot (that is, one that
actually builds), and the relocation seems to be correct (I've forced a
call to relocate_secure_section() in an unsuspecting command). I feel
relieved.

So this bug only affects systems that have their PSCI in main memory.
Maybe a CONFIG_ALLOCATE_PSCI_STACK_IN_RAM would be in order so that
systems with SRAM do not have to see their u-boot grow by another 8kB?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...