Re: [PATCH 02/15] objtool: Add CONFIG_CFI_CLANG support

[Sami Tolvanen]

On Tue, Apr 20, 2021 at 12:48 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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