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

[Marc Zyngier]

On 24/05/16 16:49, Chen-Yu Tsai wrote:
> Hi,
> 
> On Tue, May 24, 2016 at 9:58 PM, Marc Zyngier <marc.zyngier at 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 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?
> 
> 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.

Yup, that'd work.

> 
> 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?

That'd make sense. Given how hard it has become to keep the A20 SPL
under 24kB in the recent months, having a basic check on the size of the
relocated part would be a good thing. Probably for a separate patch
series though.

Thanks,

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