From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chen-Yu Tsai Date: Tue, 24 May 2016 23:49:23 +0800 Subject: [U-Boot] [PATCH 04/10] ARM: allocate extra space for PSCI stack in secure section during link phase In-Reply-To: <57445E0A.2070407@arm.com> References: <1464007306-30269-1-git-send-email-wens@csie.org> <1464007306-30269-5-git-send-email-wens@csie.org> <57442B34.7020007@arm.com> <57445E0A.2070407@arm.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, On Tue, May 24, 2016 at 9:58 PM, Marc Zyngier 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 >>> --- >>> 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?