[PATCH v2 1/1] ld: microblaze: don't index the local symbol cache
Samuel Price <[email protected]>
| Newsgroups | gmane.comp.gnu.binutils |
|---|---|
| Message-ID | <[email protected]> |
microblaze_elf_relax_section() was copied from sh_elf_relax_delete_bytes()
in 2009 (7ba29e2a41) but the copy dropped a bounds check that was already
present in the sh original. This adds the missing two-line guard from
bfd/elf32-sh.c:1227-1228, preventing an out-of-bounds read that silently
corrupts relocation addends.
The bug: when scanning relocations in other sections, the code indexes
isymbuf with a global symbol index without checking it against sh_info.
isymbuf (from symtab_hdr->contents) holds only sh_info local symbols, so
global symbol indices read past the end of the allocation.
If the out-of-bounds bytes happen to look like a local section symbol for
the section being relaxed, the relocation's addend is incorrectly adjusted,
losing 4 bytes per deleted IMM instruction. This corrupts structure member
accesses built at -O2:
lwi rD, r0, sym + offsetof(struct s, member)
This was found as a silent miscompilation in RTEMS where
Per_CPU_Control::executing was fetched as Per_CPU_Control::dispatch_necessary.
The fix copies the guard from bfd/elf32-sh.c:1227-1228, present since the
first binutils commit (252b5132c7, 1999-05-03).
Three tests are added (the first for this target), checking that addends
survive relaxation. relax-addend.d and relax-addend-data.d use
--gc-sections; relax-addend-eh.d uses .eh_frame editing (the route live on
current master).
Test behavior depends on how ld was built:
ld built normally all three pass with and without the fix, because
whether the out-of-bounds read corrupts the addend
depends on what is in the adjacent heap
ld built with ASan relax-addend-eh.d FAILS without the fix, because the
read itself is unconditional and ASan traps it, and
passes with the fix
ld/testsuite results on microblaze-elf are otherwise unchanged.
Alan Modra reviewed v1 and pointed out that none of the code in
microblaze_elf_relax_section needs to re-read global symbols, so this
version also reads only the sh_info local symbols rather than the whole
symbol table, fails the relaxation rather than asserting if the read
fails, uses PTR_ADD for the end of the local symbol buffer, and drops a
dead assignment to isym before the global symbol loop. isymbuf is left
NULL when there are no local symbols, which is safe because every site
that dereferences it is now gated on an index below sh_info.
bfd/
* elf32-microblaze.c (microblaze_elf_relax_section): Skip
relocations against global symbols when scanning the relocations
of other sections. Read only the local symbols, and fail rather
than assert if they cannot be read. Use PTR_ADD when computing
the end of the local symbol buffer. Remove a dead assignment to
isym before the global symbol loop.
ld/
* testsuite/ld-microblaze/microblaze.exp: New file.
* testsuite/ld-microblaze/relax-addend.s: New file.
* testsuite/ld-microblaze/relax-addend-support.s: New file.
* testsuite/ld-microblaze/relax-addend.ld: New file.
* testsuite/ld-microblaze/relax-addend.d: New test.
* testsuite/ld-microblaze/relax-addend-data.d: New test.
* testsuite/ld-microblaze/relax-addend-eh.s: New file.
* testsuite/ld-microblaze/relax-addend-eh-support.s: New file.
* testsuite/ld-microblaze/relax-addend-eh.ld: New file.
* testsuite/ld-microblaze/relax-addend-eh.d: New test.
Signed-off-by: Neal Frager <[email protected]>
Signed-off-by: Sam Price <[email protected]>
Assisted-by: Claude (Anthropic)
---
v1 -> v2:
Applied Alan Modra's suggested tidy-up verbatim: read only the sh_info
local symbols rather than the whole symbol table, fail rather than assert
if the read fails, use PTR_ADD for the end of the local symbol buffer, and
drop the dead assignment to isym before the global symbol loop.
No change to the guard hunk or to the ten test files from v1. The commit
message is v1's with one paragraph added describing the above, and the
bfd/ ChangeLog entry extended to match.
Whole make check on microblaze-elf, pristine b7da195b94b versus this
patch, compared test by test:
baseline with patch
ld 473 pass, 4 fail 476 pass, 4 fail
gas 327 pass, 1 fail 327 pass, 1 fail
binutils 239 pass, 0 fail 239 pass, 0 fail
No test changes status in either direction and none disappears; the only
difference is the three new tests. The four ld failures and the one gas
failure are pre-existing on this target and unrelated to this patch.
contrib/check_GNU_style.py is clean, and the patch applies to b7da195b94b
producing the tested tree.
bfd/elf32-microblaze.c | 18 +++--
ld/testsuite/ld-microblaze/microblaze.exp | 28 +++++++
.../ld-microblaze/relax-addend-data.d | 17 +++++
.../ld-microblaze/relax-addend-eh-support.s | 16 ++++
ld/testsuite/ld-microblaze/relax-addend-eh.d | 27 +++++++
ld/testsuite/ld-microblaze/relax-addend-eh.ld | 9 +++
ld/testsuite/ld-microblaze/relax-addend-eh.s | 76 +++++++++++++++++++
.../ld-microblaze/relax-addend-support.s | 24 ++++++
ld/testsuite/ld-microblaze/relax-addend.d | 26 +++++++
ld/testsuite/ld-microblaze/relax-addend.ld | 23 ++++++
ld/testsuite/ld-microblaze/relax-addend.s | 55 ++++++++++++++
11 files changed, 312 insertions(+), 7 deletions(-)
create mode 100644 ld/testsuite/ld-microblaze/microblaze.exp
create mode 100644 ld/testsuite/ld-microblaze/relax-addend-data.d
create mode 100644 ld/testsuite/ld-microblaze/relax-addend-eh-support.s
create mode 100644 ld/testsuite/ld-microblaze/relax-addend-eh.d
create mode 100644 ld/testsuite/ld-microblaze/relax-addend-eh.ld
create mode 100644 ld/testsuite/ld-microblaze/relax-addend-eh.s
create mode 100644 ld/testsuite/ld-microblaze/relax-addend-support.s
create mode 100644 ld/testsuite/ld-microblaze/relax-addend.d
create mode 100644 ld/testsuite/ld-microblaze/relax-addend.ld
create mode 100644 ld/testsuite/ld-microblaze/relax-addend.s
diff --git a/bfd/elf32-microblaze.c b/bfd/elf32-microblaze.c
index a52358a9a12..9afb1ccfdeb 100644
--- a/bfd/elf32-microblaze.c
+++ b/bfd/elf32-microblaze.c
@@ -1862,11 +1862,13 @@ microblaze_elf_relax_section (bfd *abfd,
/* Get symbols for this section. */
symtab_hdr = &elf_symtab_hdr (abfd);
isymbuf = (Elf_Internal_Sym *) symtab_hdr->contents;
- symcount = symtab_hdr->sh_size / sizeof (Elf32_External_Sym);
- if (isymbuf == NULL)
- isymbuf = bfd_elf_get_elf_syms (abfd, symtab_hdr, symcount,
- 0, NULL, NULL, NULL);
- BFD_ASSERT (isymbuf != NULL);
+ if (isymbuf == NULL && symtab_hdr->sh_info != 0)
+ {
+ isymbuf = bfd_elf_get_elf_syms (abfd, symtab_hdr, symtab_hdr->sh_info,
+ 0, NULL, NULL, NULL);
+ if (isymbuf == NULL)
+ goto error_return;
+ }
internal_relocs = _bfd_elf_link_read_relocs (abfd, sec, NULL, NULL, link_info->keep_memory);
if (internal_relocs == NULL)
@@ -2084,6 +2086,9 @@ microblaze_elf_relax_section (bfd *abfd,
irelscanend = irelocs + o->reloc_count;
for (irelscan = irelocs; irelscan < irelscanend; irelscan++)
{
+ if (ELF32_R_SYM (irelscan->r_info) >= symtab_hdr->sh_info)
+ continue;
+
if ((ELF32_R_TYPE (irelscan->r_info) == (int) R_MICROBLAZE_32)
|| (ELF32_R_TYPE (irelscan->r_info) == (int) R_MICROBLAZE_32_NONE))
{
@@ -2285,7 +2290,7 @@ microblaze_elf_relax_section (bfd *abfd,
}
/* Adjust the local symbols defined in this section. */
- isymend = isymbuf + symtab_hdr->sh_info;
+ isymend = PTR_ADD (isymbuf, symtab_hdr->sh_info);
for (isym = isymbuf; isym < isymend; isym++)
{
if (isym->st_shndx == shndx)
@@ -2297,7 +2302,6 @@ microblaze_elf_relax_section (bfd *abfd,
}
/* Now adjust the global symbols defined in this section. */
- isym = isymbuf + symtab_hdr->sh_info;
symcount = (symtab_hdr->sh_size / sizeof (Elf32_External_Sym)) - symtab_hdr->sh_info;
for (sym_index = 0; sym_index < symcount; sym_index++)
{
diff --git a/ld/testsuite/ld-microblaze/microblaze.exp b/ld/testsuite/ld-microblaze/microblaze.exp
new file mode 100644
index 00000000000..919fc2c2a49
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/microblaze.exp
@@ -0,0 +1,28 @@
+# Expect script for MicroBlaze ELF linker tests.
+# Copyright (C) 2026 Free Software Foundation, Inc.
+#
+# This file is part of the GNU Binutils.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+if { ![istarget "microblaze*-*-*"] } {
+ return
+}
+
+foreach test [lsort [glob -nocomplain $srcdir/$subdir/*.d]] {
+ verbose [file rootname $test]
+ run_dump_test [file rootname $test]
+}
diff --git a/ld/testsuite/ld-microblaze/relax-addend-data.d b/ld/testsuite/ld-microblaze/relax-addend-data.d
new file mode 100644
index 00000000000..c4b5232d870
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend-data.d
@@ -0,0 +1,17 @@
+#source: relax-addend.s
+#source: relax-addend-support.s
+#as: -EL
+#ld: -EL -relax --gc-sections -T $srcdir/$subdir/relax-addend.ld
+#readelf: -x .checkdata
+#name: MicroBlaze relaxation preserves R_MICROBLAZE_32 addends
+
+# The same check for the R_MICROBLAZE_32 arm of the same loop, using a data
+# word rather than an instruction so that readelf alone can verify it.
+# .checkdata holds a single word initialised to gvar + 0x18; with .data pinned
+# by the linker script that is 0x90001018, little endian 18 10 00 90.
+# A linker which corrupts the addend stores 14 10 00 90.
+
+#...
+Hex dump of section '.checkdata':
+[ ]*0x90001100 18100090 .*
+#pass
diff --git a/ld/testsuite/ld-microblaze/relax-addend-eh-support.s b/ld/testsuite/ld-microblaze/relax-addend-eh-support.s
new file mode 100644
index 00000000000..d06df4b83e8
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend-eh-support.s
@@ -0,0 +1,16 @@
+ .section .text.zz_support,"ax",@progbits
+ .globl near_callee
+near_callee:
+ rtsd r15, 8
+ nop
+ .globl _start
+_start:
+ brlid r15, aaa_relaxed
+ nop
+ brlid r15, zzz_victim
+ nop
+ bri 0
+ .data
+ .globl gvar
+ .align 2
+gvar: .space 64
diff --git a/ld/testsuite/ld-microblaze/relax-addend-eh.d b/ld/testsuite/ld-microblaze/relax-addend-eh.d
new file mode 100644
index 00000000000..d061fd04fb1
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend-eh.d
@@ -0,0 +1,27 @@
+#source: relax-addend-eh.s
+#source: relax-addend-eh-support.s
+#as: -EL
+#ld: -EL -relax --gc-sections -T $srcdir/$subdir/relax-addend-eh.ld
+#objdump: -d
+#name: MicroBlaze relaxation preserves addends across .eh_frame editing
+
+# Same defect as relax-addend.d, reached the other way.
+#
+# _bfd_elf_discard_section_eh_frame installs a locals-only symbol cache in
+# symtab_hdr->contents when editing .eh_frame moves a local symbol defined
+# inside it; the generic ELF emulation then calls lang_relax_sections from the
+# same after_allocation. dead_fn is unreferenced, so --gc-sections drops it,
+# its FDE is removed, the section shrinks and ehlocal moves -- which is what
+# makes the cache appear. The linker script must KEEP .eh_frame or it is swept
+# and none of this happens.
+#
+# gvar lands at 0x90000074, so the reference to gvar + 0x18 must be 0x9000008c,
+# encoded as IMM 0x9000 followed by LWI with 0x008c. A linker which corrupts
+# the addend emits e8600088.
+
+.*: +file format .*
+#...
+9000001c <zzz_victim>:
+[ ]*9000001c:[ ]+b0009000[ ]+imm[ ]+-28672
+[ ]*90000020:[ ]+e860008c[ ]+lwi[ ]+r3, r0, 140
+#pass
diff --git a/ld/testsuite/ld-microblaze/relax-addend-eh.ld b/ld/testsuite/ld-microblaze/relax-addend-eh.ld
new file mode 100644
index 00000000000..c0d11cc9cb4
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend-eh.ld
@@ -0,0 +1,9 @@
+ENTRY(_start)
+SECTIONS
+{
+ . = 0x90000000;
+ .text : { *(.text) *(.text.*) }
+ .eh_frame : { KEEP (*(.eh_frame)) }
+ .data : { *(.data) *(.data.*) }
+ /DISCARD/ : { *(.comment) *(.note*) }
+}
diff --git a/ld/testsuite/ld-microblaze/relax-addend-eh.s b/ld/testsuite/ld-microblaze/relax-addend-eh.s
new file mode 100644
index 00000000000..5900f8cc956
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend-eh.s
@@ -0,0 +1,76 @@
+/* dead_fn is dropped by --gc-sections; its FDE is then removed from .eh_frame,
+ which shifts ehlocal and makes adjust_eh_frame_local_symbols() cache a
+ locals-only symbol buffer in symtab_hdr->contents. */
+ .section .text.dead,"ax",@progbits
+ .globl dead_fn
+ .type dead_fn,@function
+dead_fn:
+ rtsd r15, 8
+ nop
+ .size dead_fn, .-dead_fn
+
+ .section .text.aaa_relaxed,"ax",@progbits
+ .globl aaa_relaxed
+ .type aaa_relaxed,@function
+aaa_relaxed:
+ addik r1, r1, -28
+ swi r15, r1, 0
+ brlid r15, near_callee
+ nop
+ lwi r15, r1, 0
+ rtsd r15, 8
+ addik r1, r1, 28
+ .size aaa_relaxed, .-aaa_relaxed
+
+ .section .text.zzz_victim,"ax",@progbits
+ .globl zzz_victim
+ .type zzz_victim,@function
+zzz_victim:
+ lwi r3, r0, gvar+24
+ rtsd r15, 8
+ nop
+ .size zzz_victim, .-zzz_victim
+
+ .section .eh_frame,"a",@progbits
+/* Hand-assembled so that no label-difference expressions are used. MicroBlaze
+ GAS emits an R_MICROBLAZE_NONE marker for every resolved label difference,
+ which lands in .rela.eh_frame and trips the
+ BFD_ASSERT (cookie->rel->r_offset == ent->offset + 8) in
+ _bfd_elf_discard_section_eh_frame. Literal lengths and CIE pointers keep
+ .rela.eh_frame to just the two FDE initial-location relocations. */
+
+ /* CIE at 0x00, total 20 bytes */
+ .4byte 16 /* length */
+ .4byte 0 /* CIE id */
+ .byte 1 /* version */
+ .asciz "zR" /* augmentation */
+ .uleb128 1 /* code alignment factor */
+ .sleb128 -4 /* data alignment factor */
+ .byte 15 /* return address register */
+ .uleb128 1 /* augmentation data length */
+ .byte 0x00 /* FDE encoding: DW_EH_PE_absptr */
+ .byte 0x0c, 0x01, 0x00 /* DW_CFA_def_cfa r1, 0 */
+
+ /* FDE for dead_fn at 0x14, total 20 bytes. dead_fn is dropped by
+ --gc-sections, so this FDE is removed and everything after it moves. */
+ .4byte 16 /* length */
+ .4byte 0x18 /* CIE pointer: this field's offset - 0 */
+ .4byte dead_fn /* initial location <- the only reloc */
+ .4byte 8 /* address range */
+ .uleb128 0 /* augmentation data length */
+ .byte 0, 0, 0 /* padding to 20 bytes */
+
+ehlocal: /* local symbol inside .eh_frame; it is
+ this symbol moving that makes
+ adjust_eh_frame_local_symbols() cache
+ a locals-only symbol buffer */
+
+ /* FDE for aaa_relaxed at 0x28, total 20 bytes */
+ .4byte 16
+ .4byte 0x2c
+ .4byte aaa_relaxed /* <- the only other reloc */
+ .4byte 32
+ .uleb128 0
+ .byte 0, 0, 0
+
+ .4byte 0 /* terminator */
diff --git a/ld/testsuite/ld-microblaze/relax-addend-support.s b/ld/testsuite/ld-microblaze/relax-addend-support.s
new file mode 100644
index 00000000000..03d5eb0ffba
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend-support.s
@@ -0,0 +1,24 @@
+# Placed in .text.zz_support so that it is laid out after the sections of
+# relax-addend.s and the call from aaa_relaxed is a forward reference. A
+# backward reference is not relaxed and the test would not exercise anything.
+
+ .section .text.zz_support,"ax",@progbits
+ .globl near_callee
+ .type near_callee,@function
+near_callee:
+ rtsd r15, 8
+ nop
+ .size near_callee, .-near_callee
+
+ .globl _start
+_start:
+ brlid r15, aaa_relaxed
+ nop
+ brlid r15, zzz_victim
+ nop
+ bri 0
+
+ .data
+ .globl gvar
+ .align 2
+gvar: .space 64
diff --git a/ld/testsuite/ld-microblaze/relax-addend.d b/ld/testsuite/ld-microblaze/relax-addend.d
new file mode 100644
index 00000000000..951c8737696
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend.d
@@ -0,0 +1,26 @@
+#source: relax-addend.s
+#source: relax-addend-support.s
+#as: -EL
+#ld: -EL -relax --gc-sections -T $srcdir/$subdir/relax-addend.ld
+#objdump: -d
+#name: MicroBlaze relaxation preserves R_MICROBLAZE_64 addends
+
+# .text.aaa_relaxed contains an IMM that relaxation deletes. .text.zzz_victim,
+# a sibling section of the same object, refers to gvar + 0x18. The linker must
+# not let the deletion disturb that addend.
+#
+# The linker script pins .data, so gvar is at 0x90001000 and the reference must
+# resolve to 0x90001018, encoded as IMM 0x9000 followed by LWI with 0x1018.
+# A linker which corrupts the addend emits e8601014 instead.
+
+.*: +file format .*
+#...
+90000000 <aaa_relaxed>:
+[ ]*90000000:[ ]+3021ffe4[ ]+addik[ ]+r1, r1, -28
+[ ]*90000004:[ ]+f9e10000[ ]+swi[ ]+r15, r1, 0
+[ ]*90000008:[ ]+b9f40024[ ]+brlid[ ]+r15, 36
+#...
+9000001c <zzz_victim>:
+[ ]*9000001c:[ ]+b0009000[ ]+imm[ ]+-28672
+[ ]*90000020:[ ]+e8601018[ ]+lwi[ ]+r3, r0, 4120
+#pass
diff --git a/ld/testsuite/ld-microblaze/relax-addend.ld b/ld/testsuite/ld-microblaze/relax-addend.ld
new file mode 100644
index 00000000000..ca4d3603989
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend.ld
@@ -0,0 +1,23 @@
+/* Fixed layout so that the expected dump is deterministic. gvar is the first
+ thing in .data, which is pinned, so the reference to gvar + 0x18 has a known
+ value regardless of how the sections happen to be ordered. */
+ENTRY(_start)
+SECTIONS
+{
+ . = 0x90000000;
+ .text : {
+ *(.text.aaa_relaxed)
+ *(.text.zzz_victim)
+ *(.text.zz_support)
+ *(.text)
+ *(.text.*)
+ }
+ . = 0x90001000;
+ .data : {
+ *(.data)
+ *(.data.*)
+ }
+ . = 0x90001100;
+ .checkdata : { KEEP (*(.checkdata)) }
+ /DISCARD/ : { *(.comment) *(.note*) }
+}
diff --git a/ld/testsuite/ld-microblaze/relax-addend.s b/ld/testsuite/ld-microblaze/relax-addend.s
new file mode 100644
index 00000000000..16e500170fb
--- /dev/null
+++ b/ld/testsuite/ld-microblaze/relax-addend.s
@@ -0,0 +1,55 @@
+# Linker relaxation must not disturb the addend of an R_MICROBLAZE_64
+# relocation that refers to a symbol outside the section being relaxed.
+#
+# .text.aaa_relaxed contains an IMM that relaxation deletes. .text.zzz_victim,
+# a sibling section in the same object file, contains
+#
+# R_MICROBLAZE_64 gvar + 0x18
+#
+# While relaxing .text.aaa_relaxed, microblaze_elf_relax_section() walks the
+# relocations of every other section of the same BFD and, for R_MICROBLAZE_64,
+# indexes isymbuf with ELF32_R_SYM (irelscan->r_info) without checking it
+# against symtab_hdr->sh_info. gvar is global, so its symbol index is >=
+# sh_info and the read is out of bounds -- isymbuf holds only sh_info entries
+# once --gc-sections has installed the locals-only cache. If the bytes read
+# happen to satisfy
+#
+# isym->st_shndx == shndx && ELF32_ST_TYPE (isym->st_info) == STT_SECTION
+#
+# the addend is decremented by calc_fixup(), and gvar + 0x18 links as
+# gvar + 0x14.
+#
+# The out-of-bounds read itself is unconditional; run ld under ASan or
+# valgrind to observe it. Whether it corrupts the addend depends on what is
+# in the adjacent heap, so the check below is a correctness assertion rather
+# than a reliable trigger.
+
+ .section .text.aaa_relaxed,"ax",@progbits
+ .globl aaa_relaxed
+ .type aaa_relaxed,@function
+aaa_relaxed:
+ addik r1, r1, -28
+ swi r15, r1, 0
+ brlid r15, near_callee # IMM + BRLID; the IMM is deleted
+ nop
+ lwi r15, r1, 0
+ rtsd r15, 8
+ addik r1, r1, 28
+ .size aaa_relaxed, .-aaa_relaxed
+
+ .section .text.zzz_victim,"ax",@progbits
+ .globl zzz_victim
+ .type zzz_victim,@function
+zzz_victim:
+ lwi r3, r0, gvar+24 # IMM + LWI; R_MICROBLAZE_64 gvar+0x18
+ rtsd r15, 8
+ nop
+ .size zzz_victim, .-zzz_victim
+
+/* A data reference to the same symbol with the same non-zero addend. This
+ goes through the R_MICROBLAZE_32 arm of the same loop, and unlike the
+ instruction above it can be checked with readelf alone. */
+ .section .checkdata,"aw",@progbits
+ .globl check_word
+check_word:
+ .4byte gvar + 0x18
--
2.39.5 (Apple Git-154)