| 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
| ||
|
Message-ID: <CAMj1kXHt3GTVx-Ow0OZaP4WW7k=RVc+jgtC-4qOSZM3js4jo0g@mail.gmail.com>
Date: Fri, 4 Oct 2024 08:54:16 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] objtool: Detect non-relocated text references
Hi Josh,
On Fri, 4 Oct 2024 at 02:31, Josh Poimboeuf <jpoimboe@...nel.org> 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@...nel.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
Thanks for the quick fix. This addresses the issue I ran into, so
Tested-by: Ard Biesheuvel <ardb@...nel.org>
> ---
> 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
>
Powered by blists - more mailing lists