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

[Chen-Yu Tsai <wens@csie.org>]

Hi,

On Tue, May 24, 2016 at 9:58 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> 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@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?

Maybe we could just put the new macro in the "#ifndef CONFIG_ARMV7_SECURE_BASE"
above? The code get relocated if CONFIG_ARMV7_SECURE_BASE is set, and the
region is not reserved. I think the current status is that if one uses
CONFIG_ARMV7_SECURE_BASE then it should be secure SRAM/DRAM.

I'll also make it clear in the commit message that this only affects
systems that put PSCI in main memory.

Sorry for the confusion.


Regards
ChenYu

P.S. I wonder if we should do a size check for the secure section?