Extricate ibt from validate_branch() in preparation for making stack
validation optional.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 253 ++++++++++++++++++++----------------------
1 file changed, 120 insertions(+), 133 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index bb25937b2d1c..1b1e7a4ae18b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3183,114 +3183,6 @@ static struct instruction *next_insn_to_validate(struct objtool_file *file,
return next_insn_same_sec(file, insn);
}
-static struct instruction *
-validate_ibt_reloc(struct objtool_file *file, struct reloc *reloc)
-{
- struct instruction *dest;
- struct section *sec;
- unsigned long off;
-
- sec = reloc->sym->sec;
- off = reloc->sym->offset;
-
- if ((reloc->sec->base->sh.sh_flags & SHF_EXECINSTR) &&
- (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32))
- off += arch_dest_reloc_offset(reloc->addend);
- else
- off += reloc->addend;
-
- dest = find_insn(file, sec, off);
- if (!dest)
- return NULL;
-
- if (dest->type == INSN_ENDBR) {
- if (!list_empty(&dest->call_node))
- list_del_init(&dest->call_node);
-
- return NULL;
- }
-
- if (reloc->sym->static_call_tramp)
- return NULL;
-
- return dest;
-}
-
-static void warn_noendbr(const char *msg, struct section *sec, unsigned long offset,
- struct instruction *dest)
-{
- WARN_FUNC("%srelocation to !ENDBR: %s", sec, offset, msg,
- offstr(dest->sec, dest->offset, false));
-}
-
-static void validate_ibt_dest(struct objtool_file *file, struct instruction *insn,
- struct instruction *dest)
-{
- if (dest->func && dest->func == insn->func) {
- /*
- * 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;
- }
-
- if (dest->noendbr)
- return;
-
- warn_noendbr("", insn->sec, insn->offset, dest);
-}
-
-static void validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
-{
- struct instruction *dest;
- struct reloc *reloc;
-
- switch (insn->type) {
- case INSN_CALL:
- case INSN_CALL_DYNAMIC:
- case INSN_JUMP_CONDITIONAL:
- case INSN_JUMP_UNCONDITIONAL:
- case INSN_JUMP_DYNAMIC:
- case INSN_JUMP_DYNAMIC_CONDITIONAL:
- case INSN_RETURN:
- /*
- * We're looking for code references setting up indirect code
- * flow. As such, ignore direct code flow and the actual
- * dynamic branches.
- */
- return;
-
- case INSN_NOP:
- /*
- * handle_group_alt() will create INSN_NOP instruction that
- * don't belong to any section, ignore all NOP since they won't
- * carry a (useful) relocation anyway.
- */
- return;
-
- default:
- break;
- }
-
- for (reloc = insn_reloc(file, insn);
- reloc;
- reloc = find_reloc_by_dest_range(file->elf, insn->sec,
- reloc->offset + 1,
- (insn->offset + insn->len) - (reloc->offset + 1))) {
- dest = validate_ibt_reloc(file, reloc);
- if (dest)
- validate_ibt_dest(file, insn, dest);
- }
-}
-
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -3500,9 +3392,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
}
- if (opts.ibt)
- validate_ibt_insn(file, insn);
-
if (insn->dead_end)
return 0;
@@ -3788,48 +3677,146 @@ static int validate_functions(struct objtool_file *file)
return warnings;
}
-static int validate_ibt(struct objtool_file *file)
+static void mark_endbr_used(struct instruction *insn)
{
- struct section *sec;
+ if (!list_empty(&insn->call_node))
+ list_del_init(&insn->call_node);
+}
+
+static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
+{
+ struct instruction *dest;
struct reloc *reloc;
+ unsigned long off;
+ int warnings = 0;
- for_each_sec(file, sec) {
- bool is_data;
+ /*
+ * Looking for function pointer load relocations. Ignore
+ * direct/indirect branches:
+ */
+ switch (insn->type) {
+ case INSN_CALL:
+ case INSN_CALL_DYNAMIC:
+ case INSN_JUMP_CONDITIONAL:
+ case INSN_JUMP_UNCONDITIONAL:
+ case INSN_JUMP_DYNAMIC:
+ case INSN_JUMP_DYNAMIC_CONDITIONAL:
+ case INSN_RETURN:
+ case INSN_NOP:
+ return 0;
+ default:
+ break;
+ }
- /* already done in validate_branch() */
- if (sec->sh.sh_flags & SHF_EXECINSTR)
- continue;
+ for (reloc = insn_reloc(file, insn);
+ reloc;
+ reloc = find_reloc_by_dest_range(file->elf, insn->sec,
+ reloc->offset + 1,
+ (insn->offset + insn->len) - (reloc->offset + 1))) {
- if (!sec->reloc)
+ /*
+ * 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;
- if (!strncmp(sec->name, ".orc", 4))
- continue;
+ off = reloc->sym->offset;
+ if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
+ off += arch_dest_reloc_offset(reloc->addend);
+ else
+ off += reloc->addend;
- if (!strncmp(sec->name, ".discard", 8))
+ dest = find_insn(file, reloc->sym->sec, off);
+ if (!dest)
continue;
- if (!strncmp(sec->name, ".debug", 6))
+ if (dest->type == INSN_ENDBR) {
+ mark_endbr_used(dest);
continue;
+ }
- if (!strcmp(sec->name, "_error_injection_whitelist"))
+ if (dest->func && dest->func == insn->func) {
+ /*
+ * 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;
+ }
- if (!strcmp(sec->name, "_kprobe_blacklist"))
+ if (dest->noendbr)
continue;
- is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
+ WARN_FUNC("relocation to !ENDBR: %s",
+ insn->sec, insn->offset,
+ offstr(dest->sec, dest->offset, false));
- list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
- struct instruction *dest;
+ warnings++;
+ }
- dest = validate_ibt_reloc(file, reloc);
- if (is_data && dest && !dest->noendbr)
- warn_noendbr("data ", sec, reloc->offset, dest);
- }
+ return warnings;
+}
+
+static int validate_ibt_data_reloc(struct objtool_file *file,
+ struct reloc *reloc)
+{
+ struct instruction *dest;
+
+ dest = find_insn(file, reloc->sym->sec,
+ reloc->sym->offset + reloc->addend);
+ if (!dest)
+ return 0;
+
+ if (dest->type == INSN_ENDBR) {
+ mark_endbr_used(dest);
+ return 0;
}
- return 0;
+ if (dest->noendbr)
+ return 0;
+
+ WARN_FUNC("data relocation to !ENDBR: %s",
+ reloc->sec->base, reloc->offset,
+ offstr(dest->sec, dest->offset, false));
+
+ return 1;
+}
+
+
+static int validate_ibt(struct objtool_file *file)
+{
+ struct section *sec;
+ struct reloc *reloc;
+ struct instruction *insn;
+ int warnings = 0;
+
+ for_each_insn(file, insn)
+ warnings += validate_ibt_insn(file, insn);
+
+ for_each_sec(file, sec) {
+
+ if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
+ continue;
+
+ if (!sec->reloc)
+ continue;
+
+ list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
+ warnings += validate_ibt_data_reloc(file, reloc);
+ }
+
+ return warnings;
}
static int validate_reachable_instructions(struct objtool_file *file)
--
2.34.1
On Wed, Apr 13, 2022 at 04:19:45PM -0700, Josh Poimboeuf wrote:
> Extricate ibt from validate_branch() in preparation for making stack
> validation optional.
It does a bit more..
> - /* already done in validate_branch() */
> - if (sec->sh.sh_flags & SHF_EXECINSTR)
> - continue;
>
> - if (!sec->reloc)
> continue;
>
> - if (!strncmp(sec->name, ".orc", 4))
> - continue;
>
> - if (!strncmp(sec->name, ".discard", 8))
> continue;
>
> - if (!strncmp(sec->name, ".debug", 6))
> continue;
>
> - if (!strcmp(sec->name, "_error_injection_whitelist"))
> continue;
>
> - if (!strcmp(sec->name, "_kprobe_blacklist"))
> continue;
>
> - is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
>
> - list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
> - struct instruction *dest;
>
> - dest = validate_ibt_reloc(file, reloc);
> - if (is_data && dest && !dest->noendbr)
> - warn_noendbr("data ", sec, reloc->offset, dest);
> - }
So this iterates all sections and excludes a bunch, and only reports
fail for .data/.rodata.
> +static int validate_ibt(struct objtool_file *file)
> +{
> + struct section *sec;
> + struct reloc *reloc;
> + struct instruction *insn;
> + int warnings = 0;
> +
> + for_each_insn(file, insn)
> + warnings += validate_ibt_insn(file, insn);
So I specifically didn't do this because I wanted to reduce the amount
of loops we do over those instructions. But yeah, if you really want to
allow --ibt without --stack-validate (but why?) then I suppose so.
Esp. for the vmlinux.o case, iterating all insn can quickly add up to
significant time.
> + for_each_sec(file, sec) {
> +
> + if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
> + continue;
But this only iterates .data/.rodata.
That's not the same, specifically, it'll not iterate stuff like ksymtab
that contains the EXPORT_SYMBOL* crud. The result being that we can now
seal EXPORT'ed symbols, which will make modules really sad.
There's also the .initcall sections, sealing initcalls typcally ends really
badly.
And there might be a few others I forgot about.
> + if (!sec->reloc)
> + continue;
> +
> + list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
> + warnings += validate_ibt_data_reloc(file, reloc);
> + }
> +
> + return warnings;
> }
>
> static int validate_reachable_instructions(struct objtool_file *file)
> --
> 2.34.1
>
On Thu, Apr 14, 2022 at 11:25:05AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 10:05:53AM -0700, Josh Poimboeuf wrote:
> > On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> > >
> > > > Ok. That was subtle, it needs a comment or two. I had the distinct
> > > > feeling I was introducing a bug, then I got distracted ;-)
> > >
> > > Right, lemme try and not forget to write one ;-)
> >
> > I'm rewriting the code anyway, I'll add some comments.
> >
> > > > Doesn't the compiler give those special cases ENDBR anyway? Just
> > > > wondering why we avoid the warning for those.
> > >
> > > Sure, but this is about not scribbling that ENDBR with a NOP.
> >
> > Right, but it only prints warnings for data sections, despite marking
> > others:
> >
> > - dest = validate_ibt_reloc(file, reloc);
> > - if (is_data && dest && !dest->noendbr)
> > - warn_noendbr("data ", sec, reloc->offset, dest);
>
>
> How about this? This way it doesn't silence warnings for non
> .data/.rodata, as other sections (including ksymtab) can also have valid
> function pointers. It also caught a bug(?) in putuser.S, as some of the
> exported inner labels didn't have ENDBR.
>
>
> diff --git a/arch/x86/include/asm/static_call.h b/arch/x86/include/asm/static_call.h
> index 2455d721503e..2d8dacd02643 100644
> --- a/arch/x86/include/asm/static_call.h
> +++ b/arch/x86/include/asm/static_call.h
> @@ -26,6 +26,7 @@
> ".align 4 \n" \
> ".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
> STATIC_CALL_TRAMP_STR(name) ": \n" \
> + ANNOTATE_NOENDBR \
> insns " \n" \
> ".byte 0x53, 0x43, 0x54 \n" \
> ".type " STATIC_CALL_TRAMP_STR(name) ", @function \n" \
Right, that makes more sense than hard-coding that exclusion, no idea
what I was thinking ;-)
> diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
> index ecb2049c1273..b7dfd60243b7 100644
> --- a/arch/x86/lib/putuser.S
> +++ b/arch/x86/lib/putuser.S
> @@ -48,6 +48,7 @@ SYM_FUNC_START(__put_user_1)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_1, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 1: movb %al,(%_ASM_CX)
> xor %ecx,%ecx
> @@ -62,6 +63,7 @@ SYM_FUNC_START(__put_user_2)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_2, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 2: movw %ax,(%_ASM_CX)
> xor %ecx,%ecx
> @@ -76,6 +78,7 @@ SYM_FUNC_START(__put_user_4)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_4, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 3: movl %eax,(%_ASM_CX)
> xor %ecx,%ecx
> @@ -90,6 +93,7 @@ SYM_FUNC_START(__put_user_8)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_8, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 4: mov %_ASM_AX,(%_ASM_CX)
> #ifdef CONFIG_X86_32
Hmm, how did those go missing?
> diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> index 5f87bab4fb8d..b2b2366885a2 100644
> --- a/arch/x86/lib/retpoline.S
> +++ b/arch/x86/lib/retpoline.S
> @@ -31,6 +31,7 @@
> .align RETPOLINE_THUNK_SIZE
> SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
> UNWIND_HINT_EMPTY
> + ANNOTATE_NOENDBR
>
> ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
> __stringify(RETPOLINE \reg), X86_FEATURE_RETPOLINE, \
> @@ -55,7 +56,6 @@ SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
>
> .align RETPOLINE_THUNK_SIZE
> SYM_CODE_START(__x86_indirect_thunk_array)
> - ANNOTATE_NOENDBR // apply_retpolines
>
> #define GEN(reg) THUNK reg
> #include <asm/GEN-for-each-reg.h>
Hmm, where does that come from? Do you have commit be8a096521ca
("x86,bpf: Avoid IBT objtool warning")?
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index ac17196e2518..3a2cd93bf059 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -45,6 +45,7 @@ SYM_CODE_END(hypercall_page)
> __INIT
> SYM_CODE_START(startup_xen)
> UNWIND_HINT_EMPTY
> + ANNOTATE_NOENDBR
> cld
>
> /* Clear .bss */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index b09c416432b6..10e375c84088 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -3794,7 +3794,10 @@ static int validate_ibt_data_reloc(struct objtool_file *file,
> return 1;
> }
>
> -
> +/*
> + * Validate IBT rules and mark used ENDBR instructions so the non-used ones can
> + * be sealed later by create_ibt_endbr_seal_sections().
> + */
> static int validate_ibt(struct objtool_file *file)
> {
> struct section *sec;
> @@ -3807,12 +3810,36 @@ static int validate_ibt(struct objtool_file *file)
>
> for_each_sec(file, sec) {
>
> - if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
> + /* Already done by validate_ibt_insn() */
> + if (sec->sh.sh_flags & SHF_EXECINSTR)
> continue;
>
> if (!sec->reloc)
> continue;
>
> + /*
> + * These sections can reference text addresses, but not with
> + * the intent to indirect branch to them.
> + */
> + if (!strncmp(sec->name, ".discard", 8) ||
> + !strncmp(sec->name, ".debug", 6) ||
> + !strcmp(sec->name, ".altinstructions") ||
> + !strcmp(sec->name, ".ibt_endbr_seal") ||
> + !strcmp(sec->name, ".orc_unwind_ip") ||
> + !strcmp(sec->name, ".parainstructions") ||
> + !strcmp(sec->name, ".retpoline_sites") ||
> + !strcmp(sec->name, ".smp_locks") ||
> + !strcmp(sec->name, ".static_call_sites") ||
> + !strcmp(sec->name, ".static_call_tramp_key") ||
> + !strcmp(sec->name, "_error_injection_whitelist") ||
> + !strcmp(sec->name, "_kprobe_blacklist") ||
> + !strcmp(sec->name, "__bug_table") ||
> + !strcmp(sec->name, "__ex_table") ||
> + !strcmp(sec->name, "__jump_table") ||
> + !strcmp(sec->name, "__mcount_loc") ||
> + !strcmp(sec->name, "__tracepoints"))
> + continue;
> +
> list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
> warnings += validate_ibt_data_reloc(file, reloc);
> }
>
Yes, looks good, Thanks!
On Thu, Apr 14, 2022 at 10:05:53AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> >
> > > Ok. That was subtle, it needs a comment or two. I had the distinct
> > > feeling I was introducing a bug, then I got distracted ;-)
> >
> > Right, lemme try and not forget to write one ;-)
>
> I'm rewriting the code anyway, I'll add some comments.
>
> > > Doesn't the compiler give those special cases ENDBR anyway? Just
> > > wondering why we avoid the warning for those.
> >
> > Sure, but this is about not scribbling that ENDBR with a NOP.
>
> Right, but it only prints warnings for data sections, despite marking
> others:
>
> - dest = validate_ibt_reloc(file, reloc);
> - if (is_data && dest && !dest->noendbr)
> - warn_noendbr("data ", sec, reloc->offset, dest);
How about this? This way it doesn't silence warnings for non
.data/.rodata, as other sections (including ksymtab) can also have valid
function pointers. It also caught a bug(?) in putuser.S, as some of the
exported inner labels didn't have ENDBR.
diff --git a/arch/x86/include/asm/static_call.h b/arch/x86/include/asm/static_call.h
index 2455d721503e..2d8dacd02643 100644
--- a/arch/x86/include/asm/static_call.h
+++ b/arch/x86/include/asm/static_call.h
@@ -26,6 +26,7 @@
".align 4 \n" \
".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
STATIC_CALL_TRAMP_STR(name) ": \n" \
+ ANNOTATE_NOENDBR \
insns " \n" \
".byte 0x53, 0x43, 0x54 \n" \
".type " STATIC_CALL_TRAMP_STR(name) ", @function \n" \
diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
index ecb2049c1273..b7dfd60243b7 100644
--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -48,6 +48,7 @@ SYM_FUNC_START(__put_user_1)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_1, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
1: movb %al,(%_ASM_CX)
xor %ecx,%ecx
@@ -62,6 +63,7 @@ SYM_FUNC_START(__put_user_2)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_2, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
2: movw %ax,(%_ASM_CX)
xor %ecx,%ecx
@@ -76,6 +78,7 @@ SYM_FUNC_START(__put_user_4)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_4, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
3: movl %eax,(%_ASM_CX)
xor %ecx,%ecx
@@ -90,6 +93,7 @@ SYM_FUNC_START(__put_user_8)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_8, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
4: mov %_ASM_AX,(%_ASM_CX)
#ifdef CONFIG_X86_32
diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
index 5f87bab4fb8d..b2b2366885a2 100644
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -31,6 +31,7 @@
.align RETPOLINE_THUNK_SIZE
SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
UNWIND_HINT_EMPTY
+ ANNOTATE_NOENDBR
ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
__stringify(RETPOLINE \reg), X86_FEATURE_RETPOLINE, \
@@ -55,7 +56,6 @@ SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
.align RETPOLINE_THUNK_SIZE
SYM_CODE_START(__x86_indirect_thunk_array)
- ANNOTATE_NOENDBR // apply_retpolines
#define GEN(reg) THUNK reg
#include <asm/GEN-for-each-reg.h>
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index ac17196e2518..3a2cd93bf059 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -45,6 +45,7 @@ SYM_CODE_END(hypercall_page)
__INIT
SYM_CODE_START(startup_xen)
UNWIND_HINT_EMPTY
+ ANNOTATE_NOENDBR
cld
/* Clear .bss */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index b09c416432b6..10e375c84088 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3794,7 +3794,10 @@ static int validate_ibt_data_reloc(struct objtool_file *file,
return 1;
}
-
+/*
+ * Validate IBT rules and mark used ENDBR instructions so the non-used ones can
+ * be sealed later by create_ibt_endbr_seal_sections().
+ */
static int validate_ibt(struct objtool_file *file)
{
struct section *sec;
@@ -3807,12 +3810,36 @@ static int validate_ibt(struct objtool_file *file)
for_each_sec(file, sec) {
- if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
+ /* Already done by validate_ibt_insn() */
+ if (sec->sh.sh_flags & SHF_EXECINSTR)
continue;
if (!sec->reloc)
continue;
+ /*
+ * These sections can reference text addresses, but not with
+ * the intent to indirect branch to them.
+ */
+ if (!strncmp(sec->name, ".discard", 8) ||
+ !strncmp(sec->name, ".debug", 6) ||
+ !strcmp(sec->name, ".altinstructions") ||
+ !strcmp(sec->name, ".ibt_endbr_seal") ||
+ !strcmp(sec->name, ".orc_unwind_ip") ||
+ !strcmp(sec->name, ".parainstructions") ||
+ !strcmp(sec->name, ".retpoline_sites") ||
+ !strcmp(sec->name, ".smp_locks") ||
+ !strcmp(sec->name, ".static_call_sites") ||
+ !strcmp(sec->name, ".static_call_tramp_key") ||
+ !strcmp(sec->name, "_error_injection_whitelist") ||
+ !strcmp(sec->name, "_kprobe_blacklist") ||
+ !strcmp(sec->name, "__bug_table") ||
+ !strcmp(sec->name, "__ex_table") ||
+ !strcmp(sec->name, "__jump_table") ||
+ !strcmp(sec->name, "__mcount_loc") ||
+ !strcmp(sec->name, "__tracepoints"))
+ continue;
+
list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
warnings += validate_ibt_data_reloc(file, reloc);
}
On Thu, Apr 14, 2022 at 09:01:56PM +0200, Peter Zijlstra wrote:
> > diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
> > index ecb2049c1273..b7dfd60243b7 100644
> > --- a/arch/x86/lib/putuser.S
> > +++ b/arch/x86/lib/putuser.S
> > @@ -48,6 +48,7 @@ SYM_FUNC_START(__put_user_1)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_1, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 1: movb %al,(%_ASM_CX)
> > xor %ecx,%ecx
> > @@ -62,6 +63,7 @@ SYM_FUNC_START(__put_user_2)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_2, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 2: movw %ax,(%_ASM_CX)
> > xor %ecx,%ecx
> > @@ -76,6 +78,7 @@ SYM_FUNC_START(__put_user_4)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_4, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 3: movl %eax,(%_ASM_CX)
> > xor %ecx,%ecx
> > @@ -90,6 +93,7 @@ SYM_FUNC_START(__put_user_8)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_8, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 4: mov %_ASM_AX,(%_ASM_CX)
> > #ifdef CONFIG_X86_32
>
> Hmm, how did those go missing?
Because the current code only warns about references from .data/.rodata.
This patch changes that.
> > diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> > index 5f87bab4fb8d..b2b2366885a2 100644
> > --- a/arch/x86/lib/retpoline.S
> > +++ b/arch/x86/lib/retpoline.S
> > @@ -31,6 +31,7 @@
> > .align RETPOLINE_THUNK_SIZE
> > SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
> > UNWIND_HINT_EMPTY
> > + ANNOTATE_NOENDBR
> >
> > ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
> > __stringify(RETPOLINE \reg), X86_FEATURE_RETPOLINE, \
> > @@ -55,7 +56,6 @@ SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
> >
> > .align RETPOLINE_THUNK_SIZE
> > SYM_CODE_START(__x86_indirect_thunk_array)
> > - ANNOTATE_NOENDBR // apply_retpolines
> >
> > #define GEN(reg) THUNK reg
> > #include <asm/GEN-for-each-reg.h>
>
> Hmm, where does that come from? Do you have commit be8a096521ca
> ("x86,bpf: Avoid IBT objtool warning")?
Same answer as above, this patch now warns about exported symbols with
missing ENDBR.
--
Josh
On Thu, Apr 14, 2022 at 10:05:50AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> >
> > > Ok. That was subtle, it needs a comment or two. I had the distinct
> > > feeling I was introducing a bug, then I got distracted ;-)
> >
> > Right, lemme try and not forget to write one ;-)
>
> I'm rewriting the code anyway, I'll add some comments.
>
> > > Doesn't the compiler give those special cases ENDBR anyway? Just
> > > wondering why we avoid the warning for those.
> >
> > Sure, but this is about not scribbling that ENDBR with a NOP.
>
> Right, but it only prints warnings for data sections, despite marking
> others:
>
> - dest = validate_ibt_reloc(file, reloc);
> - if (is_data && dest && !dest->noendbr)
> - warn_noendbr("data ", sec, reloc->offset, dest);
>
Right, so validate_ibt_reloc() does two things:
- it removes the instruction from the seal list (if it was still on it)
- it returns the dest instruction if it isn't ENDBR (or a static call
trampoline, which we know will never be indirectly called).
So that first thing is always important; we should not seal too many
things (.initcall and .ksymtab would be bad etc..), that second thing is
only useful if there is potential control flow.
That is, .ksymtab is not used for control flow (directly), it's the
symbol table for the module linker. OTOH .initcall does have control
flow, but it is 100% control flow.
In neither case does it really make sense to warn (then again, they also
should not trigger warns I suppose).
.data/.rodata otoh is more questionable, typically a code reference
there is a function pointer and we really want an ENDBR there, but it's
not always the case, hence the warns from there.
Now, I have some vague memories of getting a lot of noise from some
other sections, but I can't really remember now :-/
On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> Ok. That was subtle, it needs a comment or two. I had the distinct
> feeling I was introducing a bug, then I got distracted ;-)
Right, lemme try and not forget to write one ;-)
> Doesn't the compiler give those special cases ENDBR anyway? Just
> wondering why we avoid the warning for those.
Sure, but this is about not scribbling that ENDBR with a NOP.
On Thu, Apr 14, 2022 at 09:53:18AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:45PM -0700, Josh Poimboeuf wrote:
> > Extricate ibt from validate_branch() in preparation for making stack
> > validation optional.
>
> It does a bit more..
Indeed.
> > - /* already done in validate_branch() */
> > - if (sec->sh.sh_flags & SHF_EXECINSTR)
> > - continue;
> >
> > - if (!sec->reloc)
> > continue;
> >
> > - if (!strncmp(sec->name, ".orc", 4))
> > - continue;
> >
> > - if (!strncmp(sec->name, ".discard", 8))
> > continue;
> >
> > - if (!strncmp(sec->name, ".debug", 6))
> > continue;
> >
> > - if (!strcmp(sec->name, "_error_injection_whitelist"))
> > continue;
> >
> > - if (!strcmp(sec->name, "_kprobe_blacklist"))
> > continue;
> >
> > - is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
> >
> > - list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
> > - struct instruction *dest;
> >
> > - dest = validate_ibt_reloc(file, reloc);
> > - if (is_data && dest && !dest->noendbr)
> > - warn_noendbr("data ", sec, reloc->offset, dest);
> > - }
>
> So this iterates all sections and excludes a bunch, and only reports
> fail for .data/.rodata.
Oops.
> > +static int validate_ibt(struct objtool_file *file)
> > +{
> > + struct section *sec;
> > + struct reloc *reloc;
> > + struct instruction *insn;
> > + int warnings = 0;
> > +
> > + for_each_insn(file, insn)
> > + warnings += validate_ibt_insn(file, insn);
>
> So I specifically didn't do this because I wanted to reduce the amount
> of loops we do over those instructions. But yeah, if you really want to
> allow --ibt without --stack-validate (but why?) then I suppose so.
>
> Esp. for the vmlinux.o case, iterating all insn can quickly add up to
> significant time.
I didn't look at the performance, but if it's a problem then we can
eventually look at combining several of the for_each_insn() features
into a single loop: retpolines, ibt, reachability check, maybe even some
of decode_sections().
>
> > + for_each_sec(file, sec) {
> > +
> > + if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
> > + continue;
>
> But this only iterates .data/.rodata.
>
> That's not the same, specifically, it'll not iterate stuff like ksymtab
> that contains the EXPORT_SYMBOL* crud. The result being that we can now
> seal EXPORT'ed symbols, which will make modules really sad.
>
> There's also the .initcall sections, sealing initcalls typcally ends really
> badly.
>
> And there might be a few others I forgot about.
Ok. That was subtle, it needs a comment or two. I had the distinct
feeling I was introducing a bug, then I got distracted ;-)
Doesn't the compiler give those special cases ENDBR anyway? Just
wondering why we avoid the warning for those.
--
Josh
On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
>
> > Ok. That was subtle, it needs a comment or two. I had the distinct
> > feeling I was introducing a bug, then I got distracted ;-)
>
> Right, lemme try and not forget to write one ;-)
I'm rewriting the code anyway, I'll add some comments.
> > Doesn't the compiler give those special cases ENDBR anyway? Just
> > wondering why we avoid the warning for those.
>
> Sure, but this is about not scribbling that ENDBR with a NOP.
Right, but it only prints warnings for data sections, despite marking
others:
- dest = validate_ibt_reloc(file, reloc);
- if (is_data && dest && !dest->noendbr)
- warn_noendbr("data ", sec, reloc->offset, dest);
--
Josh