On Fri, Apr 16, 2021 at 01:38:31PM -0700, Sami Tolvanen wrote:
> +static int fix_cfi_relocs(const struct elf *elf)
> +{
> + struct section *sec, *tmpsec;
> + struct reloc *reloc, *tmpreloc;
> +
> + list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
> + list_for_each_entry_safe(reloc, tmpreloc, &sec->reloc_list,
> list) {
These loops don't remove structs from the lists, so are the "safe"
iterators really needed?
> + struct symbol *sym;
> + struct reloc *func_reloc;
> +
> + /*
> + * CONFIG_CFI_CLANG replaces function relocations to
> refer
> + * to an intermediate jump table. Undo the conversion so
> + * objtool can make sense of things.
> + */
I think this comment could be clearer if it were placed above the
function.
> + if (!reloc->sym->sec->cfi_jt)
> + continue;
> +
> + if (reloc->sym->type == STT_SECTION)
> + sym = find_func_by_offset(reloc->sym->sec,
> + reloc->addend);
> + else
> + sym = reloc->sym;
> +
> + if (!sym || !sym->sec)
> + continue;
This should be a fatal error, it probably means something's gone wrong
with the reading of the jump table.
> +
> + /*
> + * The jump table immediately jumps to the actual
> function,
> + * so look up the relocation there.
> + */
> + func_reloc = find_reloc_by_dest_range(elf, sym->sec,
> sym->offset, 5);
The jump instruction's reloc is always at offset 1, so this can maybe be
optimized to:
func_reloc = find_reloc_by_dest(elf, sym->sec,
sym->offset+1);
though of course that will probably break (future) arm64 objtool. Maybe
an arch-specific offset from the start of the insn would be good.
> + if (!func_reloc || !func_reloc->sym)
> + continue;
This should also be an error.
> +
> + reloc->sym = func_reloc->sym;
I think we should also do 'reloc->addend = 0', because of the
STT_SECTION case:
0000000000000025 0000259e0000000b R_X86_64_32S 0000000000000000
.text..L.cfi.jumptable.8047 + 8
which then translates to
0000000000000009 0000063200000004 R_X86_64_PLT32 0000000000000000
x2apic_prepare_cpu - 4
so the original addend of '8' is no longer valid. Copying the '-4'
wouldn't be right either, because that only applies to a PLT32 text
reloc. A '0' should be appropriate for the 32S data reloc.
This addend might not actually be read by any code, so it may not
currently be an actual bug, but better safe than sorry.
--
Josh
