From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Date: Tue, 24 May 2016 14:58:34 +0100 Subject: [U-Boot] [PATCH 04/10] ARM: allocate extra space for PSCI stack in secure section during link phase In-Reply-To: <57442B34.7020007@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> Message-ID: <57445E0A.2070407@arm.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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? Thanks, M. -- Jazz is not dead. It just smells funny...