linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sami Tolvanen <samitolvanen@google.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: X86 ML <x86@kernel.org>, Kees Cook <keescook@chromium.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Sedat Dilek <sedat.dilek@gmail.com>,
	linux-hardening@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>,
	clang-built-linux <clang-built-linux@googlegroups.com>
Subject: Re: [PATCH v3 01/16] objtool: Add CONFIG_CFI_CLANG support
Date: Tue, 14 Sep 2021 14:01:30 -0700	[thread overview]
Message-ID: <CABCJKufA537qjWumDSeF6y0Ei5Ej=SXY-7r=Qyu3+VtVUALSCA@mail.gmail.com> (raw)
In-Reply-To: <CAKwvOd=pmcfJRkgBFnqePauMd67+eQ9=JAbSjxrWmBQY9zRveQ@mail.gmail.com>

On Tue, Sep 14, 2021 at 12:29 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Sep 14, 2021 at 12:10 PM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > With CONFIG_CFI_CLANG, the compiler replaces function references with
> > references to the CFI jump table, which confuses objtool. This change,
> > based on Josh's initial patch [1], goes through the list of relocations
> > and replaces jump table symbols with the actual function symbols.
> >
> > [1] https://lore.kernel.org/r/d743f4b36e120c06506567a9f87a062ae03da47f.1611263462.git.jpoimboe@redhat.com/
> >
> > Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> > Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> >  tools/objtool/arch/x86/decode.c      | 16 +++++++++
> >  tools/objtool/elf.c                  | 51 ++++++++++++++++++++++++++++
> >  tools/objtool/include/objtool/arch.h |  3 ++
> >  tools/objtool/include/objtool/elf.h  |  2 +-
> >  4 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> > index bc821056aba9..318189c8065e 100644
> > --- a/tools/objtool/arch/x86/decode.c
> > +++ b/tools/objtool/arch/x86/decode.c
> > @@ -62,6 +62,22 @@ bool arch_callee_saved_reg(unsigned char reg)
> >         }
> >  }
> >
> > +unsigned long arch_cfi_section_reloc_offset(struct reloc *reloc)
> > +{
> > +       if (!reloc->addend)
> > +               return 0;
> > +
> > +       if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
> > +               return reloc->addend + 4;
> > +
> > +       return reloc->addend;
> > +}
> > +
> > +unsigned long arch_cfi_jump_reloc_offset(unsigned long offset)
> > +{
> > +       return offset + 1;
> > +}
> > +
> >  unsigned long arch_dest_reloc_offset(int addend)
> >  {
> >         return addend + 4;
> > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > index 8676c7598728..05a5f51aad2c 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> > @@ -18,6 +18,7 @@
> >  #include <errno.h>
> >  #include <objtool/builtin.h>
> >
> > +#include <objtool/arch.h>
> >  #include <objtool/elf.h>
> >  #include <objtool/warn.h>
> >
> > @@ -291,6 +292,10 @@ static int read_sections(struct elf *elf)
> >                 if (sec->sh.sh_flags & SHF_EXECINSTR)
> >                         elf->text_size += sec->len;
> >
> > +               /* Detect -fsanitize=cfi jump table sections */
> > +               if (!strncmp(sec->name, ".text..L.cfi.jumptable", 22))
> > +                       sec->cfi_jt = true;
> > +
> >                 list_add_tail(&sec->list, &elf->sections);
> >                 elf_hash_add(section, &sec->hash, sec->idx);
> >                 elf_hash_add(section_name, &sec->name_hash, str_hash(sec->name));
> > @@ -576,6 +581,49 @@ static int read_rela_reloc(struct section *sec, int i, struct reloc *reloc, unsi
> >         return 0;
> >  }
> >
> > +/*
> > + * CONFIG_CFI_CLANG replaces function relocations to refer to an intermediate
> > + * jump table. Undo the conversion so objtool can make sense of things.
> > + */
> > +static int fix_cfi_relocs(const struct elf *elf)
> > +{
> > +       struct section *sec;
> > +       struct reloc *reloc;
> > +
> > +       list_for_each_entry(sec, &elf->sections, list) {
> > +               list_for_each_entry(reloc, &sec->reloc_list, list) {
> > +                       struct reloc *cfi_reloc;
> > +                       unsigned long offset;
> > +
> > +                       if (!reloc->sym->sec->cfi_jt)
> > +                               continue;
> > +
> > +                       if (reloc->sym->type == STT_SECTION)
> > +                               offset = arch_cfi_section_reloc_offset(reloc);
> > +                       else
> > +                               offset = reloc->sym->offset;
> > +
> > +                       /*
> > +                        * The jump table immediately jumps to the actual function,
> > +                        * so look up the relocation there.
> > +                        */
> > +                       offset = arch_cfi_jump_reloc_offset(offset);
>
> Sorry, this comment is curious to me, it looks like we jump to the
> offset+1, not directly to the actual function?  Perhaps a comment
> above arch_cfi_jump_reloc_offset() and/or amending this comment might
> make it clearer? Sorry if this is obvious to others?  Perhaps comments
> can be cleaned up in a follow up, if this is not a bug?

It looks like my response was sent only to Nick, so to summarize the
brief off-list discussion:

arch_cfi_jump_reloc_offset() returns the offset to a relocation when
given the address of a jmp instruction in the CFI jump table. Here's
an example:

Disassembly of section .text..L.cfi.jumptable:

0000000000000000 <_printk.cfi_jt>:
       0: e9 00 00 00 00                jmp     0x5 <_printk.cfi_jt+0x5>
                0000000000000001:  R_X86_64_PLT32       _printk-0x4
       5: cc                            int3
       6: cc                            int3
       7: cc                            int3

We look at the relocation in the jump table to figure out the actual
target function, in this case, _printk. Alternatively, we could look
at the jump table symbol instead (i.e., _printk.cfi_jt) and use the
name to figure out the target. Unfortunately, LLVM doesn't generate
symbols for all the jump table entries, so we can't rely on them in
objtool.

What comes to the magic offset value, it's one because the first byte
of the jmp instruction is the opcode and the relocation only applies
to the rest of the instruction.

I'll add a comment to arch_cfi_jump_reloc_offset() in v4 to clarify this.

Sami

  reply	other threads:[~2021-09-14 21:01 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 19:10 [PATCH v3 00/16] x86: Add support for Clang CFI Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 01/16] objtool: Add CONFIG_CFI_CLANG support Sami Tolvanen
2021-09-14 19:29   ` Nick Desaulniers
2021-09-14 21:01     ` Sami Tolvanen [this message]
2021-09-14 19:10 ` [PATCH v3 02/16] objtool: Add ASM_STACK_FRAME_NON_STANDARD Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 03/16] linkage: Add DECLARE_ASM_FUNC_SYMBOL Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 04/16] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB Sami Tolvanen
2021-09-14 19:36   ` Nick Desaulniers
2021-09-14 20:32     ` Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 05/16] tracepoint: Exclude tp_stub_func from CFI checking Sami Tolvanen
2021-09-14 19:39   ` Nick Desaulniers
2021-09-14 19:10 ` [PATCH v3 06/16] ftrace: Use an opaque type for functions not callable from C Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 07/16] lkdtm: Disable UNSET_SMEP with CFI Sami Tolvanen
2021-09-14 19:30   ` Kees Cook
2021-09-14 19:10 ` [PATCH v3 08/16] lkdtm: Use an opaque type for lkdtm_rodata_do_nothing Sami Tolvanen
2021-09-14 19:32   ` Kees Cook
2021-09-14 19:10 ` [PATCH v3 09/16] x86: Use an opaque type for functions not callable from C Sami Tolvanen
2021-09-14 19:33   ` Kees Cook
2021-09-14 19:10 ` [PATCH v3 10/16] x86/extable: Mark handlers __cficanonical Sami Tolvanen
2021-09-14 19:37   ` Kees Cook
2021-09-14 20:38     ` Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 11/16] x86/purgatory: Disable CFI Sami Tolvanen
2021-09-14 20:02   ` Nick Desaulniers
2021-09-14 20:30     ` Sami Tolvanen
2021-09-14 22:31       ` Nick Desaulniers
2021-09-15  6:24         ` Kees Cook
2021-09-14 19:10 ` [PATCH v3 12/16] x86, relocs: Ignore __typeid__ relocations Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 13/16] x86, module: " Sami Tolvanen
2021-09-14 19:10 ` [PATCH v3 14/16] x86, cpu: Use LTO for cpu.c with CFI Sami Tolvanen
2021-09-14 19:44   ` Kees Cook
2021-09-14 19:46   ` Nick Desaulniers
2021-09-14 19:10 ` [PATCH v3 15/16] x86, kprobes: Fix optprobe_template_func type mismatch Sami Tolvanen
2021-09-14 19:40   ` Kees Cook
2021-09-14 19:10 ` [PATCH v3 16/16] x86, build: Allow CONFIG_CFI_CLANG to be selected Sami Tolvanen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABCJKufA537qjWumDSeF6y0Ei5Ej=SXY-7r=Qyu3+VtVUALSCA@mail.gmail.com' \
    --to=samitolvanen@google.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=jpoimboe@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=peterz@infradead.org \
    --cc=sedat.dilek@gmail.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v3 01/16] objtool: Add CONFIG_CFI_CLANG support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).