microblaze: don't index the local symbol cache with a global symbol index
Reviewer suggests a tidy-up to a patch that fixes an out-of-bounds read in microblaze ELF relaxation by ensuring global symbols are not indexed.
Alan Modra reviews a patch that fixes an out-of-bounds read in microblaze ELF relaxation by adding a missing bounds check. He agrees with the fix, noting that the symtab_hdr->contents buffer should only contain local symbols. Modra suggests a further tidy-up to the patch, removing the need to re-read global symbols entirely, as they are not used within the microblaze_elf_relax_section function.
- reviewer
Approves the patch and suggests a code cleanup to remove unnecessary global symbol lookups.
“The patch looks good to me. As you correctly point out, the symtab_hdr->contents buffer is supposed to only contain local syms, so indexing at symtab_hdr->sh_info or above is wrong. What's more, none of the code in microblaze_elf_relax_section needs to re-read global syms so you might like to add the following tidy-up to your patch.”
- proposer
Submitted a patch to fix an out-of-bounds read in microblaze_elf_relax_section by adding a missing bounds check.
“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.”
Technical Tradeoffs
- Fixes an out-of-bounds read, preventing silent corruption of relocation addends.
- Simplifies the `microblaze_elf_relax_section` function by removing unnecessary global symbol lookups.
In Details
This discussion pertains to the microblaze_elf_relax_section function within binutils' BFD library. A patch corrects an out-of-bounds read caused by incorrect indexing into the local symbol table (symtab_hdr->contents). The reviewer suggests an additional cleanup to prevent the function from accessing global symbols altogether, as they are not required for the relaxation process on microblaze.
- microblaze
- A configurable soft processor core from Xilinx, often used in embedded systems and FPGAs.
- ELF
- Executable and Linkable Format. A standard file format for executables, object code, shared libraries, and core dumps, commonly used on Unix-like systems.
- Relaxation
- A linking process where the linker adjusts relocation entries based on the final placement of code and data. This can involve changing instruction encoding or operand values.
- Relocation Addend
- A value stored in a relocation entry that is added to the symbol's address to form the final address. In this context, it's being corrupted during relaxation.
- BFD
- Binary File Descriptor. A library used by GNU binary utilities to provide a common interface for handling various executable file formats.
- Symbol Cache
- An internal data structure used by the linker or assembler to store information about symbols (variables, functions, etc.) for quick lookup.