Extricate ibt from validate_branch() so they can be executed (or ported)
independently from each other.
While shuffling code around, simplify and improve the ibt logic:
- Ignore an explicit list of known sections which reference functions
for reasons other than indirect branching to them. This helps prevent
unnnecesary sealing.
- Warn on missing !ENDBR for all other sections, not just .data and
.rodata. This finds additional warnings, because there are sections
other than .[ro]data which reference function pointers. For example,
the ksymtab sections which are used for exporting symbols.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
tools/objtool/check.c | 280 ++++++++++++++++++++++--------------------
1 file changed, 147 insertions(+), 133 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8dc6035fabd2..0059b592195e 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));
-}
-
-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,173 @@ 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))
+ 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;
+
+ dest = find_insn(file, reloc->sym->sec, off);
+ if (!dest)
continue;
- if (!strncmp(sec->name, ".discard", 8))
+ if (dest->type == INSN_ENDBR) {
+ mark_endbr_used(dest);
continue;
+ }
- if (!strncmp(sec->name, ".debug", 6))
+ 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, "_error_injection_whitelist"))
+ if (dest->noendbr)
continue;
- if (!strcmp(sec->name, "_kprobe_blacklist"))
+ WARN_FUNC("relocation to !ENDBR: %s",
+ insn->sec, insn->offset,
+ offstr(dest->sec, dest->offset));
+
+ warnings++;
+ }
+
+ 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;
+ }
+
+ if (dest->noendbr)
+ return 0;
+
+ WARN_FUNC("data relocation to !ENDBR: %s",
+ reloc->sec->base, reloc->offset,
+ offstr(dest->sec, dest->offset));
+
+ return 1;
+}
+
+/*
+ * Validate IBT rules and remove used ENDBR instructions from the seal list.
+ * Unused ENDBR instructions will be annotated for sealing (i.e., replaced with
+ * NOPs) later, in create_ibt_endbr_seal_sections().
+ */
+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) {
+
+ /* Already done by validate_ibt_insn() */
+ if (sec->sh.sh_flags & SHF_EXECINSTR)
continue;
- is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
+ if (!sec->reloc)
+ continue;
- list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
- struct instruction *dest;
+ /*
+ * 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, "_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;
- dest = validate_ibt_reloc(file, reloc);
- if (is_data && dest && !dest->noendbr)
- warn_noendbr("data ", sec, reloc->offset, dest);
- }
+ list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
+ warnings += validate_ibt_data_reloc(file, reloc);
}
- return 0;
+ return warnings;
}
static int validate_reachable_instructions(struct objtool_file *file)
@@ -3900,7 +3914,7 @@ int check(struct objtool_file *file)
warnings += ret;
}
- if (opts.stackval || opts.orc || opts.uaccess || opts.ibt || opts.sls) {
+ if (opts.stackval || opts.orc || opts.uaccess || opts.sls) {
ret = validate_functions(file);
if (ret < 0)
goto out;
--
2.34.1
A nit and it was there even before this patch...
> -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);
here...
> +static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
> +{
...
> + 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;
it looks kind of strange to have arch_dest_reloc_offset() and still
reference arch-specific relocation types here. On the other hand it seems
difficult to achieve complete arch-agnostic code, so take it just as a
note and maybe someone porting objtool to a different architecture will
split the code, make it all arch-independent and all will be nice and
shiny.
Miroslav
On Wed, Apr 20, 2022 at 07:25:16PM +0200, Miroslav Benes wrote:
> A nit and it was there even before this patch...
>
> > -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);
>
> here...
>
> > +static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
> > +{
>
> ...
> > + 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;
>
> it looks kind of strange to have arch_dest_reloc_offset() and still
> reference arch-specific relocation types here. On the other hand it seems
> difficult to achieve complete arch-agnostic code, so take it just as a
> note and maybe someone porting objtool to a different architecture will
> split the code, make it all arch-independent and all will be nice and
> shiny.
Something like so perhaps? Seems to build and boot x86_64-defconfig.
---
tools/objtool/arch/x86/decode.c | 9 +++++++--
tools/objtool/check.c | 18 +++++++-----------
tools/objtool/include/objtool/arch.h | 2 +-
3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 8b990a52aada..775e1963ecfc 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -63,9 +63,14 @@ bool arch_callee_saved_reg(unsigned char reg)
}
}
-unsigned long arch_dest_reloc_offset(int addend)
+unsigned long arch_dest_reloc_offset(struct reloc *reloc)
{
- return addend + 4;
+ unsigned long offset = reloc->sym->offset + reloc->addend;
+
+ if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
+ offset += 4;
+
+ return offset;
}
unsigned long arch_jump_destination(struct instruction *insn)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2063f9fea1a2..5752013dd6e8 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1295,7 +1295,7 @@ static int add_jump_destinations(struct objtool_file *file)
dest_off = arch_jump_destination(insn);
} else if (reloc->sym->type == STT_SECTION) {
dest_sec = reloc->sym->sec;
- dest_off = arch_dest_reloc_offset(reloc->addend);
+ dest_off = arch_dest_reloc_offset(reloc);
} else if (reloc->sym->retpoline_thunk) {
add_retpoline_call(file, insn);
continue;
@@ -1308,8 +1308,7 @@ static int add_jump_destinations(struct objtool_file *file)
continue;
} else if (reloc->sym->sec->idx) {
dest_sec = reloc->sym->sec;
- dest_off = reloc->sym->sym.st_value +
- arch_dest_reloc_offset(reloc->addend);
+ dest_off = arch_dest_reloc_offset(reloc);
} else {
/* non-func asm code jumping to another file */
continue;
@@ -1413,7 +1412,7 @@ static int add_call_destinations(struct objtool_file *file)
}
} else if (reloc->sym->type == STT_SECTION) {
- dest_off = arch_dest_reloc_offset(reloc->addend);
+ dest_off = arch_dest_reloc_offset(reloc);
dest = find_call_destination(reloc->sym->sec, dest_off);
if (!dest) {
WARN_FUNC("can't find call dest symbol at %s+0x%lx",
@@ -3031,6 +3030,7 @@ static inline const char *call_dest_name(struct instruction *insn)
static bool pv_call_dest(struct objtool_file *file, struct instruction *insn)
{
struct symbol *target;
+ unsigned long offset;
struct reloc *rel;
int idx;
@@ -3038,7 +3038,8 @@ static bool pv_call_dest(struct objtool_file *file, struct instruction *insn)
if (!rel || strcmp(rel->sym->name, "pv_ops"))
return false;
- idx = (arch_dest_reloc_offset(rel->addend) / sizeof(void *));
+ offset = arch_dest_reloc_offset(rel) - rel->sym->offset;
+ idx = offset / sizeof(void *);
if (file->pv_ops[idx].clean)
return true;
@@ -3709,12 +3710,7 @@ static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn
if (reloc->sym->static_call_tramp)
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;
-
+ off = arch_dest_reloc_offset(reloc);
dest = find_insn(file, reloc->sym->sec, off);
if (!dest)
continue;
diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
index 9b19cc304195..57562eaa0967 100644
--- a/tools/objtool/include/objtool/arch.h
+++ b/tools/objtool/include/objtool/arch.h
@@ -81,7 +81,7 @@ bool arch_callee_saved_reg(unsigned char reg);
unsigned long arch_jump_destination(struct instruction *insn);
-unsigned long arch_dest_reloc_offset(int addend);
+unsigned long arch_dest_reloc_offset(struct reloc *reloc);
const char *arch_nop_insn(int len);
const char *arch_ret_insn(int len);
On Fri, Apr 22, 2022 at 12:50:37PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 20, 2022 at 07:25:16PM +0200, Miroslav Benes wrote:
> > A nit and it was there even before this patch...
> >
> > > -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);
> >
> > here...
> >
> > > +static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
> > > +{
> >
> > ...
> > > + 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;
> >
> > it looks kind of strange to have arch_dest_reloc_offset() and still
> > reference arch-specific relocation types here. On the other hand it seems
> > difficult to achieve complete arch-agnostic code, so take it just as a
> > note and maybe someone porting objtool to a different architecture will
> > split the code, make it all arch-independent and all will be nice and
> > shiny.
>
> Something like so perhaps? Seems to build and boot x86_64-defconfig.
Looks good...
--
Josh
The following commit has been merged into the objtool/core branch of tip:
Commit-ID: 3c6f9f77e6188ca4d283633d66e91b3821a505ae
Gitweb: https://git.kernel.org/tip/3c6f9f77e6188ca4d283633d66e91b3821a505ae
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Mon, 18 Apr 2022 09:50:34 -07:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Fri, 22 Apr 2022 12:32:02 +02:00
objtool: Rework ibt and extricate from stack validation
Extricate ibt from validate_branch() so they can be executed (or ported)
independently from each other.
While shuffling code around, simplify and improve the ibt logic:
- Ignore an explicit list of known sections which reference functions
for reasons other than indirect branching to them. This helps prevent
unnnecesary sealing.
- Warn on missing !ENDBR for all other sections, not just .data and
.rodata. This finds additional warnings, because there are sections
other than .[ro]data which reference function pointers. For example,
the ksymtab sections which are used for exporting symbols.
Signed-off-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Miroslav Benes <[email protected]>
Link: https://lkml.kernel.org/r/fd1435e46bb95f81031b8fb1fa360f5f787e4316.1650300597.git.jpoimboe@redhat.com
---
tools/objtool/check.c | 280 +++++++++++++++++++++--------------------
1 file changed, 147 insertions(+), 133 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3456eb9..85c2888 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3182,114 +3182,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));
-}
-
-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
@@ -3499,9 +3391,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;
@@ -3787,48 +3676,173 @@ 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))
+ 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;
+
+ dest = find_insn(file, reloc->sym->sec, off);
+ if (!dest)
continue;
- if (!strncmp(sec->name, ".discard", 8))
+ if (dest->type == INSN_ENDBR) {
+ mark_endbr_used(dest);
continue;
+ }
- if (!strncmp(sec->name, ".debug", 6))
+ 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, "_error_injection_whitelist"))
+ if (dest->noendbr)
continue;
- if (!strcmp(sec->name, "_kprobe_blacklist"))
+ WARN_FUNC("relocation to !ENDBR: %s",
+ insn->sec, insn->offset,
+ offstr(dest->sec, dest->offset));
+
+ warnings++;
+ }
+
+ 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;
+ }
+
+ if (dest->noendbr)
+ return 0;
+
+ WARN_FUNC("data relocation to !ENDBR: %s",
+ reloc->sec->base, reloc->offset,
+ offstr(dest->sec, dest->offset));
+
+ return 1;
+}
+
+/*
+ * Validate IBT rules and remove used ENDBR instructions from the seal list.
+ * Unused ENDBR instructions will be annotated for sealing (i.e., replaced with
+ * NOPs) later, in create_ibt_endbr_seal_sections().
+ */
+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) {
+
+ /* Already done by validate_ibt_insn() */
+ if (sec->sh.sh_flags & SHF_EXECINSTR)
continue;
- is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
+ if (!sec->reloc)
+ continue;
- list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
- struct instruction *dest;
+ /*
+ * 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, "_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;
- dest = validate_ibt_reloc(file, reloc);
- if (is_data && dest && !dest->noendbr)
- warn_noendbr("data ", sec, reloc->offset, dest);
- }
+ list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
+ warnings += validate_ibt_data_reloc(file, reloc);
}
- return 0;
+ return warnings;
}
static int validate_reachable_instructions(struct objtool_file *file)
@@ -3899,7 +3913,7 @@ int check(struct objtool_file *file)
warnings += ret;
}
- if (opts.stackval || opts.orc || opts.uaccess || opts.ibt || opts.sls) {
+ if (opts.stackval || opts.orc || opts.uaccess || opts.sls) {
ret = validate_functions(file);
if (ret < 0)
goto out;
On Fri, 22 Apr 2022, Peter Zijlstra wrote:
> On Wed, Apr 20, 2022 at 07:25:16PM +0200, Miroslav Benes wrote:
> > A nit and it was there even before this patch...
> >
> > > -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);
> >
> > here...
> >
> > > +static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
> > > +{
> >
> > ...
> > > + 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;
> >
> > it looks kind of strange to have arch_dest_reloc_offset() and still
> > reference arch-specific relocation types here. On the other hand it seems
> > difficult to achieve complete arch-agnostic code, so take it just as a
> > note and maybe someone porting objtool to a different architecture will
> > split the code, make it all arch-independent and all will be nice and
> > shiny.
>
> Something like so perhaps? Seems to build and boot x86_64-defconfig.
Yes, that looks good. Thanks.
Miroslav