[PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address

[André Przywara]

On 10/09/2020 13:38, Michal Simek wrote:
> 
> 
> On 09. 09. 20 19:07, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias at xilinx.com>
>>
>> Trap non-PIE builds early if the start address doesn't match
>> between run-time and link-time. This will trap the startup
>> sequence rather than letting it run into obscure errors.
>>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias at xilinx.com>
>> ---
>>  arch/arm/cpu/armv8/start.S | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
>> index e5c2856cf5..39e1b842c4 100644
>> --- a/arch/arm/cpu/armv8/start.S
>> +++ b/arch/arm/cpu/armv8/start.S
>> @@ -101,6 +101,19 @@ pie_skip_reloc:
>>  	cmp	x2, x3
>>  	b.lo	pie_fix_loop
>>  pie_fixup_done:
>> +#else
>> +	adr	x0, _start
>> +	ldr	x1, _TEXT_BASE
>> +	cmp	x0, x1
>> +	beq	1f
>> +0:
>> +	/*
>> +	 * FATAL, can't continue.
>> +	 * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE.
>> +	 */
>> +	wfi
>> +	b	0b
>> +1:
>>  #endif
>>  
>>  #ifdef CONFIG_SYS_RESET_SCTRL
>>
> 
> NACK for this.
> 
> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for
> U-Boot proper
> 2. It likely also breaks TPL flow for the same reason
> 
> 3. And last thing is that this code is used only for U-Boot proper.
> .globl	_TEXT_BASE
> _TEXT_BASE:
> 	.quad	CONFIG_SYS_TEXT_BASE
> 
> The fixes are below. Point 3 should be likely be in separate patch
> because it is unrelated.

So if this patch causes issues, can't we just drop it? I mean right now
you will probably just crash anyway if you load it at the wrong address,
but maybe late enough that you get more hints or even some output.

Now this patch makes sure that you don't get anything, so I don't see
how this is really improving the situation. It seems like a case of
"don't fix things that ain't broken".

Cheers,
Andre


> 
> Thanks,
> Michal
> 
> 
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index b37dbabf4d42..cdc609e873fc 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -33,10 +33,11 @@ _start:
> 
>         .align 3
> 
> +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD))
>  .globl _TEXT_BASE
>  _TEXT_BASE:
>         .quad   CONFIG_SYS_TEXT_BASE
> -
> +#endif
>  /*
>   * These are defined in the linker script.
>   */
> @@ -102,6 +103,7 @@ pie_skip_reloc:
>         b.lo    pie_fix_loop
>  pie_fixup_done:
>  #else
> +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD))
>         adr     x0, _start
>         ldr     x1, _TEXT_BASE
>         cmp     x0, x1
> @@ -115,6 +117,7 @@ pie_fixup_done:
>         b       0b
>  1:
>  #endif
> +#endif
> 
>  #ifdef CONFIG_SYS_RESET_SCTRL
>         bl reset_sctrl
>