Re: [PATCH] objtool: Detect non-relocated text references

[Ard Biesheuvel]

Hi Josh,

On Fri, 4 Oct 2024 at 02:31, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> When kernel IBT is enabled, objtool detects all text references in order
> to determine which functions can be indirectly branched to.
>
> In text, such references look like one of the following:
>
>    mov    $0x0,%rax        R_X86_64_32S     .init.text+0x7e0a0
>    lea    0x0(%rip),%rax   R_X86_64_PC32    autoremove_wake_function-0x4
>
> Either way the function pointer is denoted by a relocation, so objtool
> just reads that.
>
> However there are some "lea xxx(%rip)" cases which don't use relocations
> because they're referencing code in the same translation unit.

input section

> Objtool
> doesn't have visibility to those.
>
> The only currently known instances of that are a few hand-coded asm text
> references which don't actually need ENDBR.  So it's not actually a
> problem at the moment.
>
> However if we enable -fpie, the compiler would start generating them and
> there would definitely be bugs in the IBT sealing.
>

-fpie is guaranteed to break things, but even without it, Clang may
issue RIP-relative LEA instructions (or LLD when it performs
relaxations), so this is definitely worth addressing even if we don't
enable -fpie.

> Detect non-relocated text references and handle them appropriately.
>
> [ Note: I removed the manual static_call_tramp check -- that should
>   already be handled by the noendbr check. ]
>
> Reported-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>

Thanks for the quick fix. This addresses the issue I ran into, so

Tested-by: Ard Biesheuvel <ardb@xxxxxxxxxx>

> ---
>  arch/x86/kernel/acpi/wakeup_64.S     |   1 +
>  arch/x86/kernel/head_64.S            |   1 +
>  tools/objtool/arch/x86/decode.c      |  15 ++--
>  tools/objtool/check.c                | 112 +++++++++++++++------------
>  tools/objtool/include/objtool/arch.h |   1 +
>  5 files changed, 77 insertions(+), 53 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
> index af2f2ed57658..5e4472f788b3 100644
> --- a/arch/x86/kernel/acpi/wakeup_64.S
> +++ b/arch/x86/kernel/acpi/wakeup_64.S
> @@ -85,6 +85,7 @@ SYM_FUNC_START(do_suspend_lowlevel)
>
>         .align 4
>  .Lresume_point:
> +       ANNOTATE_NOENDBR
>         /* We don't restore %rax, it must be 0 anyway */
>         leaq    saved_context(%rip), %rax
>         movq    saved_context_cr4(%rax), %rbx
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 88cdc5a0c7a3..9e95599b58cf 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -88,6 +88,7 @@ SYM_CODE_START_NOALIGN(startup_64)
>         lretq
>
>  .Lon_kernel_cs:
> +       ANNOTATE_NOENDBR
>         UNWIND_HINT_END_OF_STACK
>
>  #ifdef CONFIG_AMD_MEM_ENCRYPT
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index ed6bff0e01dc..fe1362c34564 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -456,10 +456,6 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
>                 if (!rex_w)
>                         break;
>
> -               /* skip RIP relative displacement */
> -               if (is_RIP())
> -                       break;
> -
>                 /* skip nontrivial SIB */
>                 if (have_SIB()) {
>                         modrm_rm = sib_base;
> @@ -467,6 +463,12 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
>                                 break;
>                 }
>
> +               /* lea disp(%rip), %dst */
> +               if (is_RIP()) {
> +                       insn->type = INSN_LEA_RIP;
> +                       break;
> +               }
> +
>                 /* lea disp(%src), %dst */
>                 ADD_OP(op) {
>                         op->src.offset = ins.displacement.value;
> @@ -737,7 +739,10 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
>                 break;
>         }
>
> -       insn->immediate = ins.immediate.nbytes ? ins.immediate.value : 0;
> +       if (ins.immediate.nbytes)
> +               insn->immediate = ins.immediate.value;
> +       else if (ins.displacement.nbytes)
> +               insn->immediate = ins.displacement.value;
>
>         return 0;
>  }
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 94a56099e22d..d33bf36d36a3 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -4354,6 +4354,51 @@ static bool noendbr_range(struct objtool_file *file, struct instruction *insn)
>         return insn->offset == sym->offset + sym->len;
>  }
>
> +static int __validate_ibt_insn(struct objtool_file *file, struct instruction *insn,
> +                              struct instruction *dest)
> +{
> +       if (dest->type == INSN_ENDBR) {
> +               mark_endbr_used(dest);
> +               return 0;
> +       }
> +
> +       if (insn_func(dest) && insn_func(insn) &&
> +           insn_func(dest)->pfunc == insn_func(insn)->pfunc) {
> +               /*
> +                * Anything from->to self is either _THIS_IP_ or
> +                * IRET-to-self.
> +                *
> +                * There is no sane way to annotate _THIS_IP_ since the
> +                * compiler treats the relocation as a constant and is
> +                * happy to fold in offsets, skewing any annotation we
> +                * do, leading to vast amounts of false-positives.
> +                *
> +                * There's also compiler generated _THIS_IP_ through
> +                * KCOV and such which we have no hope of annotating.
> +                *
> +                * As such, blanket accept self-references without
> +                * issue.
> +                */
> +               return 0;
> +       }
> +
> +       /*
> +        * Accept anything ANNOTATE_NOENDBR.
> +        */
> +       if (dest->noendbr)
> +               return 0;
> +
> +       /*
> +        * Accept if this is the instruction after a symbol
> +        * that is (no)endbr -- typical code-range usage.
> +        */
> +       if (noendbr_range(file, dest))
> +               return 0;
> +
> +       WARN_INSN(insn, "relocation to !ENDBR: %s", offstr(dest->sec, dest->offset));
> +       return 1;
> +}
> +
>  static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
>  {
>         struct instruction *dest;
> @@ -4366,6 +4411,7 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
>          * direct/indirect branches:
>          */
>         switch (insn->type) {
> +
>         case INSN_CALL:
>         case INSN_CALL_DYNAMIC:
>         case INSN_JUMP_CONDITIONAL:
> @@ -4375,6 +4421,23 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
>         case INSN_RETURN:
>         case INSN_NOP:
>                 return 0;
> +
> +       case INSN_LEA_RIP:
> +               if (!insn_reloc(file, insn)) {
> +                       /* local function pointer reference without reloc */
> +
> +                       off = arch_jump_destination(insn);
> +
> +                       dest = find_insn(file, insn->sec, off);
> +                       if (!dest) {
> +                               WARN_INSN(insn, "corrupt function pointer reference");
> +                               return 1;
> +                       }
> +
> +                       return __validate_ibt_insn(file, insn, dest);
> +               }
> +               break;
> +
>         default:
>                 break;
>         }
> @@ -4385,13 +4448,6 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
>                                               reloc_offset(reloc) + 1,
>                                               (insn->offset + insn->len) - (reloc_offset(reloc) + 1))) {
>
> -               /*
> -                * static_call_update() references the trampoline, which
> -                * doesn't have (or need) ENDBR.  Skip warning in that case.
> -                */
> -               if (reloc->sym->static_call_tramp)
> -                       continue;
> -
>                 off = reloc->sym->offset;
>                 if (reloc_type(reloc) == R_X86_64_PC32 ||
>                     reloc_type(reloc) == R_X86_64_PLT32)
> @@ -4403,47 +4459,7 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
>                 if (!dest)
>                         continue;
>
> -               if (dest->type == INSN_ENDBR) {
> -                       mark_endbr_used(dest);
> -                       continue;
> -               }
> -
> -               if (insn_func(dest) && insn_func(insn) &&
> -                   insn_func(dest)->pfunc == insn_func(insn)->pfunc) {
> -                       /*
> -                        * Anything from->to self is either _THIS_IP_ or
> -                        * IRET-to-self.
> -                        *
> -                        * There is no sane way to annotate _THIS_IP_ since the
> -                        * compiler treats the relocation as a constant and is
> -                        * happy to fold in offsets, skewing any annotation we
> -                        * do, leading to vast amounts of false-positives.
> -                        *
> -                        * There's also compiler generated _THIS_IP_ through
> -                        * KCOV and such which we have no hope of annotating.
> -                        *
> -                        * As such, blanket accept self-references without
> -                        * issue.
> -                        */
> -                       continue;
> -               }
> -
> -               /*
> -                * Accept anything ANNOTATE_NOENDBR.
> -                */
> -               if (dest->noendbr)
> -                       continue;
> -
> -               /*
> -                * Accept if this is the instruction after a symbol
> -                * that is (no)endbr -- typical code-range usage.
> -                */
> -               if (noendbr_range(file, dest))
> -                       continue;
> -
> -               WARN_INSN(insn, "relocation to !ENDBR: %s", offstr(dest->sec, dest->offset));
> -
> -               warnings++;
> +               warnings += __validate_ibt_insn(file, insn, dest);
>         }
>
>         return warnings;
> diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
> index 0b303eba660e..d63b46a19f39 100644
> --- a/tools/objtool/include/objtool/arch.h
> +++ b/tools/objtool/include/objtool/arch.h
> @@ -28,6 +28,7 @@ enum insn_type {
>         INSN_CLD,
>         INSN_TRAP,
>         INSN_ENDBR,
> +       INSN_LEA_RIP,
>         INSN_OTHER,
>  };
>
> --
> 2.46.0
>