[PATCH master v2 2/3] ARM: cpu: add compiler barrier around unrelocated access

[Ahmad Fatoum]

GCC v11.1.1 was observed miscompiling relocate_to_current_adr() while
generating THUMB2 code:

        dynsym = (void *)__dynsym_start + offset_var;
    178c:       4b48            ldr     r3, [pc, #288]  ; (18b0 <relocate_to_current_adr+0x13c>)
    178e:       5869            ldr     r1, [r5, r1]
        dend = (void *)__rel_dyn_end + offset_var;
    1790:       4407            add     r7, r0
        dynsym = (void *)__dynsym_start + offset_var;
    1792:       58e8            ldr     r0, [r5, r3]
    1794:       f102 0308       add.w   r3, r2, #8
    1798:       440b            add     r3, r1
    179a:       4410            add     r0, r2
        dynend = (void *)__dynsym_end + offset_var;

        while (dstart < dend) {
    179c:       f1a3 0608       sub.w   r6, r3, #8
    17a0:       42b7            cmp     r7, r6
    17a2:       d80a            bhi.n   17ba <relocate_to_current_adr+0x46>
        dynend = (void *)__dynsym_end + offset_var;
    17a4:       4b43            ldr     r3, [pc, #268]  ; (18b4 <relocate_to_current_adr+0x140>)
                }

                dstart += sizeof(*rel);
        }

        __memset(dynsym, 0, (unsigned long)dynend - (unsigned long)dynsym);
    17a6:       2100            movs    r1, #0
        dynend = (void *)__dynsym_end + offset_var;
    17a8:       58eb            ldr     r3, [r5, r3]
    17aa:       441a            add     r2, r3
        __memset(dynsym, 0, (unsigned long)dynend - (unsigned long)dynsym);
    17ac:       1a12            subs    r2, r2, r0
    17ae:       f000 fda5       bl      22fc <__memset>

Both &__dynsym_start and &__dynsym_end will change value after relocation,
so we absolutely want address calculation and addition of offset_var to
happen before relocation. Compiler is within rights though to assume
variables to be already relocated though and thus proves that &__dynsym_end
may not change in the loop and thus move dynend calculation below the
relocation loop and thus we end up with dynend being incremented by
offset_var once more. The resulting out-of-bounds memset() will overwrite
parts of barebox and break its startup.

The naive solution of moving dynsym/dynend calculation beyond the
relocation loop is insufficient as the compiler may decide to move
it back. Instead the only solution short of rewriting this all in
assembly seems to be hiding the origin of dynsym's value, so the
optimizer may not prove the assumption that relocation would not affect
its value. This is done using runtime_address, which was introduced in
a previous commit. With this, the __memset call now uses precomputed
values as expected: no last minute ldr, everything tidily placed into
registers prior to the relocation loop:

	17be:       2100            movs    r1, #0
	17c0:       1b52            subs    r2, r2, r5
	17c2:       4628            mov     r0, r5
	17c4:       f000 fdaa       bl      231c <__memset>

Fixes: a8b788ba61eb ("relocate_to_current_adr: hang directly on error instead of panic()")
Signed-off-by: Ahmad Fatoum <a.fatoum at pengutronix.de>
---
 arch/arm/cpu/common.c     | 15 +++++++++------
 arch/arm/cpu/uncompress.c |  4 ++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
index 5ccacf204751..62781b76ce68 100644
--- a/arch/arm/cpu/common.c
+++ b/arch/arm/cpu/common.c
@@ -61,16 +61,19 @@ void pbl_barebox_break(void)
  */
 void relocate_to_current_adr(void)
 {
-	unsigned long offset, offset_var;
+	unsigned long offset;
 	unsigned long __maybe_unused *dynsym, *dynend;
 	void *dstart, *dend;
 
 	/* Get offset between linked address and runtime address */
 	offset = get_runtime_offset();
-	offset_var = global_variable_offset();
 
-	dstart = (void *)__rel_dyn_start + offset_var;
-	dend = (void *)__rel_dyn_end + offset_var;
+	/*
+	 * We have yet to relocate, so using runtime_address
+	 * to compute the relocated address
+	 */
+	dstart = runtime_address(__rel_dyn_start);
+	dend = runtime_address(__rel_dyn_end);
 
 #if defined(CONFIG_CPU_64)
 	while (dstart < dend) {
@@ -96,8 +99,8 @@ void relocate_to_current_adr(void)
 		dstart += sizeof(*rel);
 	}
 #elif defined(CONFIG_CPU_32)
-	dynsym = (void *)__dynsym_start + offset_var;
-	dynend = (void *)__dynsym_end + offset_var;
+	dynsym = runtime_address(__dynsym_start);
+	dynend = runtime_address(__dynsym_end);
 
 	while (dstart < dend) {
 		struct elf32_rel *rel = dstart;
diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
index 537ee63229d7..65de87f10923 100644
--- a/arch/arm/cpu/uncompress.c
+++ b/arch/arm/cpu/uncompress.c
@@ -53,8 +53,8 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
 	unsigned long pc = get_pc();
 
 	/* piggy data is not relocated, so determine the bounds now */
-	pg_start = input_data + global_variable_offset();
-	pg_end = input_data_end + global_variable_offset();
+	pg_start = runtime_address(input_data);
+	pg_end = runtime_address(input_data_end);
 
 	if (IS_ENABLED(CONFIG_PBL_RELOCATABLE)) {
 		/*
-- 
2.30.2