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@xxxxxxxxxxxxxx>
---
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
