On 24/05/16 16:49, Chen-Yu Tsai wrote:
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 textwhich 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 textadd r5, r5, #0x2000 @ Skip two pageslsr r5, r5, #12 @ Align to start of pagelsl r5, r5, #12sub 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.