[binutils-gdb] ld: microblaze: don't index the local symbol cache
Alan Modra via Binutils-cvs <[email protected]>
| Newsgroups | gmane.comp.gnu.binutils.cvs |
|---|---|
| Message-ID | <[email protected]> |
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a639e0adaf2dc8f0bcb2b55715e7abebbe39bc7f commit a639e0adaf2dc8f0bcb2b55715e7abebbe39bc7f Author: Samuel Price <[email protected]> Date: Wed Aug 12 22:18:26 2026 -0400 ld: microblaze: don't index the local symbol cache 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) Diff: --- bfd/elf32-microblaze.c | 18 +++-- ld/testsuite/ld-microblaze/microblaze.exp | 28 ++++++++ ld/testsuite/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/testsuite/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(-) diff --git a/bfd/elf32-microblaze.c b/bfd/elf32-microblaze.c index 5300f8d4d87..bf02aca2652 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