Re: [PATCH 12/21] objtool: Add CONFIG_CFI_CLANG support

[Josh Poimboeuf]

On Thu, Jan 14, 2021 at 12:49:28PM -0800, Sami Tolvanen wrote:
> Hi Josh,
> 
> On Thu, Jan 14, 2021 at 11:41 AM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> >
> > The upcoming CONFIG_CFI_CLANG support uses -fsanitize=cfi, the
> > non-canonical version of which hijacks function entry by changing
> > function relocation references to point to an intermediary jump table.
> >
> > For example:
> >
> >   Relocation section '.rela.discard.func_stack_frame_non_standard' at offset 0x37e018 contains 6 entries:
> >       Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
> >   0000000000000000  0002944700000002 R_X86_64_PC32          00000000000023f0 do_suspend_lowlevel + 0
> >   0000000000000008  0003c11900000001 R_X86_64_64            0000000000000008 xen_cpuid$e69bc59f4fade3b6f2b579b3934137df.cfi_jt + 0
> >   0000000000000010  0003980900000001 R_X86_64_64            0000000000000060 machine_real_restart.cfi_jt + 0
> >   0000000000000018  0003962b00000001 R_X86_64_64            0000000000000e18 kretprobe_trampoline.cfi_jt + 0
> >   0000000000000020  000028f300000001 R_X86_64_64            0000000000000000 .rodata + 12
> >   0000000000000028  000349f400000001 R_X86_64_64            0000000000000018 __crash_kexec.cfi_jt + 0
> >
> >   0000000000000060 <machine_real_restart.cfi_jt>:
> >     60: e9 00 00 00 00          jmpq   65 <machine_real_restart.cfi_jt+0x5>
> >                         61: R_X86_64_PLT32      machine_real_restart-0x4
> >     65: cc                      int3
> >     66: cc                      int3
> >     67: cc                      int3
> >
> > This breaks objtool vmlinux validation in many ways, including static
> > call site detection and the STACK_FRAME_NON_STANDARD() macro.
> >
> > Fix it by converting those relocations' symbol references back to their
> > original non-jump-table versions.  Note this doesn't change the actual
> > relocations in the object itself, it just changes objtool's view of
> > them.
> >
> > Reported-by: Sedat Dilek <sedat.dilek@xxxxxxxxx>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > ---
> >  tools/objtool/elf.c | 28 ++++++++++++++++++++++++++++
> >  tools/objtool/elf.h |  2 +-
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > index 292f015f7ec6..e357dc34cd7a 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> > @@ -382,6 +382,11 @@ static int read_sections(struct elf *elf)
> >                 }
> >                 sec->len = sec->sh.sh_size;
> >
> > +               /* Detect -fsanitize=cfi related sections */
> > +               if (!strcmp(sec->name, ".text.__cfi_check") ||
> > +                   !strncmp(sec->name, ".text..L.cfi.jumptable", 22))
> > +                       sec->cfi_jt = true;
> > +
> >                 list_add_tail(&sec->list, &elf->sections);
> >                 elf_hash_add(elf->section_hash, &sec->hash, sec->idx);
> >                 elf_hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
> > @@ -614,6 +619,29 @@ static int read_relocs(struct elf *elf)
> >                                 return -1;
> >                         }
> >
> > +                       /*
> > +                        * Deal with -fsanitize=cfi (CONFIG_CFI_CLANG), which
> > +                        * hijacks function entry by arbitrarily changing a lot
> > +                        * of relocation symbol references to refer to an
> > +                        * intermediate jump table.  Undo that conversion so
> > +                        * objtool can make sense of things.
> > +                        */
> > +                       if (reloc->sym->sec->cfi_jt) {
> > +                               struct symbol *func, *sym;
> > +
> > +                               if (sym->type == STT_SECTION)
> > +                                       sym = find_func_by_offset(sym->sec,
> > +                                                                 reloc->addend);
> 
> Clang points out that sym is uninitialized here. Should these be
> reloc->sym instead?

Yup, I somehow managed to bork this patch (and the one before it) right
before I sent it.  Will send updated versions of these two.

-- 
Josh