From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Tue, 24 May 2016 11:21:40 +0100
Subject: [U-Boot] [PATCH 04/10] ARM: allocate extra space for PSCI stack
 in secure section during link phase
In-Reply-To: <1464007306-30269-5-git-send-email-wens@csie.org>
References: <1464007306-30269-1-git-send-email-wens@csie.org>
 <1464007306-30269-5-git-send-email-wens@csie.org>
Message-ID: <57442B34.7020007@arm.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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

Thanks,

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