From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: barebox@lists.infradead.org
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Subject: [PATCH master 2/3] ARM: cpu: add compiler barrier around unrelocated access
Date: Thu, 13 Oct 2022 11:03:51 +0200 [thread overview]
Message-ID: <20221013090352.562170-3-a.fatoum@pengutronix.de> (raw)
In-Reply-To: <20221013090352.562170-1-a.fatoum@pengutronix.de>
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 get_unrelocated, 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>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
arch/arm/cpu/common.c | 11 +++++------
arch/arm/cpu/uncompress.c | 4 ++--
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/arm/cpu/common.c b/arch/arm/cpu/common.c
index 7cd97e938b3d..ed579ab94ed4 100644
--- a/arch/arm/cpu/common.c
+++ b/arch/arm/cpu/common.c
@@ -61,16 +61,15 @@ 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;
+ dstart = get_unrelocated(__rel_dyn_start);
+ dend = get_unrelocated(__rel_dyn_end);
#if defined(CONFIG_CPU_64)
while (dstart < dend) {
@@ -96,8 +95,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 = get_unrelocated(__dynsym_start);
+ dynend = get_unrelocated(__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..929a3a22c29b 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 = get_unrelocated(input_data);
+ pg_end = get_unrelocated(input_data_end);
if (IS_ENABLED(CONFIG_PBL_RELOCATABLE)) {
/*
--
2.30.2
next prev parent reply other threads:[~2022-10-13 9:05 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-13 9:03 [PATCH master 0/3] Fix GCC 11 THUMB2 relocate_to_current_adr miscompile Ahmad Fatoum
2022-10-13 9:03 ` [PATCH master 1/3] include: asm-generic: reloc: implement get_unrelocated() Ahmad Fatoum
2022-10-13 9:03 ` Ahmad Fatoum [this message]
2022-10-13 9:03 ` [PATCH master 3/3] RISC-V: add compiler barriers around unrelocated accesses Ahmad Fatoum
2022-10-13 10:36 ` Ahmad Fatoum
2022-10-13 11:52 ` [PATCH master 0/3] Fix GCC 11 THUMB2 relocate_to_current_adr miscompile Marco Felsch
2022-10-13 11:53 ` Ahmad Fatoum
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221013090352.562170-3-a.fatoum@pengutronix.de \
--to=a.fatoum@pengutronix.de \
--cc=barebox@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox