linux-kernel - Re: [PATCH 12/21] objtool: Add CONFIG_CFI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABCJKudFxiYbuHN+NVJ76QfHCky80nvsb_J08THmMmmHuy0vLA@mail.gmail.com>
Date:   Thu, 14 Jan 2021 12:49:28 -0800
From:   Sami Tolvanen <samitolvanen@...gle.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Sedat Dilek <sedat.dilek@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        Miroslav Benes <mbenes@...e.cz>
Subject: Re: [PATCH 12/21] objtool: Add CONFIG_CFI_CLANG support

Hi Josh,

On Thu, Jan 14, 2021 at 11:41 AM Josh Poimboeuf <jpoimboe@...hat.com> 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@...il.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> ---
>  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?

Sami