On Tue, Apr 20, 2021 at 12:48 PM Josh Poimboeuf <jpoim...@redhat.com> wrote:
>
> 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.
Thank you for taking a look, Josh! I'll fix these in the next version.
Sami
