mail archive of the barebox mailing list
 help / color / mirror / Atom feed
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




  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