* [PATCH] objtool: Detect non-relocated text references
@ 2024-10-04 0:31 Josh Poimboeuf
2024-10-04 6:54 ` Ard Biesheuvel
2024-10-04 8:21 ` Peter Zijlstra
0 siblings, 2 replies; 7+ messages in thread
From: Josh Poimboeuf @ 2024-10-04 0:31 UTC (permalink / raw)
To: x86; +Cc: linux-kernel, Peter Zijlstra, Ard Biesheuvel
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. 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.
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@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] objtool: Detect non-relocated text references
2024-10-04 0:31 [PATCH] objtool: Detect non-relocated text references Josh Poimboeuf
@ 2024-10-04 6:54 ` Ard Biesheuvel
2024-10-04 7:28 ` Josh Poimboeuf
2024-10-04 8:21 ` Peter Zijlstra
1 sibling, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2024-10-04 6:54 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Peter Zijlstra
Hi Josh,
On Fri, 4 Oct 2024 at 02:31, Josh Poimboeuf <jpoimboe@kernel.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@kernel.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Thanks for the quick fix. This addresses the issue I ran into, so
Tested-by: Ard Biesheuvel <ardb@kernel.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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] objtool: Detect non-relocated text references
2024-10-04 6:54 ` Ard Biesheuvel
@ 2024-10-04 7:28 ` Josh Poimboeuf
2024-10-04 7:59 ` Ard Biesheuvel
2024-10-04 8:20 ` Peter Zijlstra
0 siblings, 2 replies; 7+ messages in thread
From: Josh Poimboeuf @ 2024-10-04 7:28 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: x86, linux-kernel, Peter Zijlstra
On Fri, Oct 04, 2024 at 08:54:16AM +0200, Ard Biesheuvel wrote:
> On Fri, 4 Oct 2024 at 02:31, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > 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
"in the same translation unit and section" ?
> > 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.
I haven't seen this with Clang either. Also, objtool runs before the
linker so LLD relaxations shouldn't matter.
--
Josh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] objtool: Detect non-relocated text references
2024-10-04 7:28 ` Josh Poimboeuf
@ 2024-10-04 7:59 ` Ard Biesheuvel
2024-10-04 8:20 ` Peter Zijlstra
1 sibling, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2024-10-04 7:59 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Peter Zijlstra
On Fri, 4 Oct 2024 at 09:28, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Fri, Oct 04, 2024 at 08:54:16AM +0200, Ard Biesheuvel wrote:
> > On Fri, 4 Oct 2024 at 02:31, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > 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
>
> "in the same translation unit and section" ?
>
Yeah, something like that. The point is really that the only way we
might end up in this case is when the LEA offset is known at assembly
time.
> > > 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.
>
> I haven't seen this with Clang either. Also, objtool runs before the
> linker so LLD relaxations shouldn't matter.
>
Fair enough.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] objtool: Detect non-relocated text references
2024-10-04 7:28 ` Josh Poimboeuf
2024-10-04 7:59 ` Ard Biesheuvel
@ 2024-10-04 8:20 ` Peter Zijlstra
2024-10-04 16:46 ` Josh Poimboeuf
1 sibling, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2024-10-04 8:20 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: Ard Biesheuvel, x86, linux-kernel
On Fri, Oct 04, 2024 at 12:28:47AM -0700, Josh Poimboeuf wrote:
> On Fri, Oct 04, 2024 at 08:54:16AM +0200, Ard Biesheuvel wrote:
> > On Fri, 4 Oct 2024 at 02:31, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > 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
>
> "in the same translation unit and section" ?
>
> > > 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.
>
> I haven't seen this with Clang either. Also, objtool runs before the
> linker so LLD relaxations shouldn't matter.
LTO might have a few more cases, the input sections are bigger there.
But even there we run before the final link stage.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] objtool: Detect non-relocated text references
2024-10-04 8:20 ` Peter Zijlstra
@ 2024-10-04 16:46 ` Josh Poimboeuf
0 siblings, 0 replies; 7+ messages in thread
From: Josh Poimboeuf @ 2024-10-04 16:46 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ard Biesheuvel, x86, linux-kernel
On Fri, Oct 04, 2024 at 10:20:29AM +0200, Peter Zijlstra wrote:
> On Fri, Oct 04, 2024 at 12:28:47AM -0700, Josh Poimboeuf wrote:
> > On Fri, Oct 04, 2024 at 08:54:16AM +0200, Ard Biesheuvel wrote:
> > > On Fri, 4 Oct 2024 at 02:31, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > > 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
> >
> > "in the same translation unit and section" ?
> >
> > > > 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.
> >
> > I haven't seen this with Clang either. Also, objtool runs before the
> > linker so LLD relaxations shouldn't matter.
>
> LTO might have a few more cases, the input sections are bigger there.
> But even there we run before the final link stage.
Clang LTO uses -ffunction-sections so it shouldn't be possible.
--
Josh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] objtool: Detect non-relocated text references
2024-10-04 0:31 [PATCH] objtool: Detect non-relocated text references Josh Poimboeuf
2024-10-04 6:54 ` Ard Biesheuvel
@ 2024-10-04 8:21 ` Peter Zijlstra
1 sibling, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2024-10-04 8:21 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Ard Biesheuvel
On Thu, Oct 03, 2024 at 05:31:10PM -0700, Josh Poimboeuf 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. 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.
>
> 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@kernel.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-04 16:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-04 0:31 [PATCH] objtool: Detect non-relocated text references Josh Poimboeuf
2024-10-04 6:54 ` Ard Biesheuvel
2024-10-04 7:28 ` Josh Poimboeuf
2024-10-04 7:59 ` Ard Biesheuvel
2024-10-04 8:20 ` Peter Zijlstra
2024-10-04 16:46 ` Josh Poimboeuf
2024-10-04 8:21 ` Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®