On 24/05/16 16:49, Chen-Yu Tsai wrote:
> Hi,
>
> On Tue, May 24, 2016 at 9:58 PM, Marc Zyngier <marc.zyng...@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 <w...@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...
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
