Find all unused ENDBR instructions and stick them in a section such
that the kernel can poison them.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/kernel/vmlinux.lds.S | 9 ++++
tools/objtool/builtin-check.c | 3 -
tools/objtool/check.c | 72 +++++++++++++++++++++++++++++++-
tools/objtool/include/objtool/builtin.h | 2
tools/objtool/include/objtool/objtool.h | 1
tools/objtool/objtool.c | 1
6 files changed, 85 insertions(+), 3 deletions(-)
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -285,6 +285,15 @@ SECTIONS
}
#endif
+#ifdef CONFIG_X86_KERNEL_IBT
+ . = ALIGN(8);
+ .ibt_endbr_sites : AT(ADDR(.ibt_endbr_sites) - LOAD_OFFSET) {
+ __ibt_endbr_sites = .;
+ *(.ibt_endbr_sites)
+ __ibt_endbr_sites_end = .;
+ }
+#endif
+
/*
* struct alt_inst entries. From the header (alternative.h):
* "Alternative instructions for different CPU types or capabilities"
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -21,7 +21,7 @@
bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
lto, vmlinux, mcount, noinstr, backup, sls, dryrun,
- ibt, ibt_fix_direct;
+ ibt, ibt_fix_direct, ibt_seal;
static const char * const check_usage[] = {
"objtool check [<options>] file.o",
@@ -50,6 +50,7 @@ const struct option check_options[] = {
OPT_BOOLEAN(0, "dry-run", &dryrun, "don't write the modifications"),
OPT_BOOLEAN(0, "ibt", &ibt, "validate ENDBR placement"),
OPT_BOOLEAN(0, "ibt-fix-direct", &ibt_fix_direct, "fixup direct jmp/call to ENDBR"),
+ OPT_BOOLEAN(0, "ibt-seal", &ibt_seal, "list superfluous ENDBR instructions"),
OPT_END(),
};
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -732,6 +732,58 @@ static int create_retpoline_sites_sectio
return 0;
}
+static int create_ibt_endbr_sites_sections(struct objtool_file *file)
+{
+ struct instruction *insn;
+ struct section *sec;
+ int idx;
+
+ sec = find_section_by_name(file->elf, ".ibt_endbr_sites");
+ if (sec) {
+ WARN("file already has .ibt_endbr_sites, skipping");
+ return 0;
+ }
+
+ idx = 0;
+ list_for_each_entry(insn, &file->endbr_list, call_node)
+ idx++;
+
+ if (stats) {
+ printf("ibt: ENDBR at function start: %d\n", file->nr_endbr);
+ printf("ibt: ENDBR inside functions: %d\n", file->nr_endbr_int);
+ printf("ibt: superfluous ENDBR: %d\n", idx);
+ }
+
+ if (!idx)
+ return 0;
+
+ sec = elf_create_section(file->elf, ".ibt_endbr_sites", 0,
+ sizeof(int), idx);
+ if (!sec) {
+ WARN("elf_create_section: .ibt_endbr_sites");
+ return -1;
+ }
+
+ idx = 0;
+ list_for_each_entry(insn, &file->endbr_list, call_node) {
+
+ int *site = (int *)sec->data->d_buf + idx;
+ *site = 0;
+
+ if (elf_add_reloc_to_insn(file->elf, sec,
+ idx * sizeof(int),
+ R_X86_64_PC32,
+ insn->sec, insn->offset)) {
+ WARN("elf_add_reloc_to_insn: .ibt_endbr_sites");
+ return -1;
+ }
+
+ idx++;
+ }
+
+ return 0;
+}
+
static int create_mcount_loc_sections(struct objtool_file *file)
{
struct section *sec;
@@ -1179,6 +1231,7 @@ static int add_jump_destinations(struct
for_each_insn(file, insn) {
if (insn->type == INSN_ENDBR && insn->func) {
if (insn->offset == insn->func->offset) {
+ list_add_tail(&insn->call_node, &file->endbr_list);
file->nr_endbr++;
} else {
file->nr_endbr_int++;
@@ -3633,8 +3687,12 @@ validate_ibt_reloc(struct objtool_file *
if (!dest)
return NULL;
- if (dest->type == INSN_ENDBR)
+ 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;
@@ -3777,6 +3835,11 @@ int check(struct objtool_file *file)
return 1;
}
+ if (ibt_seal && !ibt_fix_direct) {
+ fprintf(stderr, "--ibt-seal requires: --ibt-fix-direct\n");
+ return 1;
+ }
+
arch_initial_func_cfi_state(&initial_func_cfi);
init_cfi_state(&init_cfi);
init_cfi_state(&func_cfi);
@@ -3854,6 +3917,13 @@ int check(struct objtool_file *file)
if (ret < 0)
goto out;
warnings += ret;
+ }
+
+ if (ibt_seal) {
+ ret = create_ibt_endbr_sites_sections(file);
+ if (ret < 0)
+ goto out;
+ warnings += ret;
}
if (stats) {
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -10,7 +10,7 @@
extern const struct option check_options[];
extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
lto, vmlinux, mcount, noinstr, backup, sls, dryrun,
- ibt, ibt_fix_direct;
+ ibt, ibt_fix_direct, ibt_seal;
extern int cmd_parse_options(int argc, const char **argv, const char * const usage[]);
--- a/tools/objtool/include/objtool/objtool.h
+++ b/tools/objtool/include/objtool/objtool.h
@@ -26,6 +26,7 @@ struct objtool_file {
struct list_head retpoline_call_list;
struct list_head static_call_list;
struct list_head mcount_loc_list;
+ struct list_head endbr_list;
bool ignore_unreachables, c_file, hints, rodata;
unsigned int nr_endbr;
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -128,6 +128,7 @@ struct objtool_file *objtool_open_read(c
INIT_LIST_HEAD(&file.retpoline_call_list);
INIT_LIST_HEAD(&file.static_call_list);
INIT_LIST_HEAD(&file.mcount_loc_list);
+ INIT_LIST_HEAD(&file.endbr_list);
file.c_file = !vmlinux && find_section_by_name(file.elf, ".comment");
file.ignore_unreachables = no_unreachable;
file.hints = false;
On Thu, Feb 24, 2022 at 03:52:14PM +0100, Peter Zijlstra wrote:
> +#ifdef CONFIG_X86_KERNEL_IBT
> + . = ALIGN(8);
> + .ibt_endbr_sites : AT(ADDR(.ibt_endbr_sites) - LOAD_OFFSET) {
> + __ibt_endbr_sites = .;
> + *(.ibt_endbr_sites)
> + __ibt_endbr_sites_end = .;
> + }
> +#endif
".ibt_endbr_superfluous" maybe? It's not *all* the endbr sites.
> +
> /*
> * struct alt_inst entries. From the header (alternative.h):
> * "Alternative instructions for different CPU types or capabilities"
> --- a/tools/objtool/builtin-check.c
> +++ b/tools/objtool/builtin-check.c
> @@ -21,7 +21,7 @@
>
> bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
> lto, vmlinux, mcount, noinstr, backup, sls, dryrun,
> - ibt, ibt_fix_direct;
> + ibt, ibt_fix_direct, ibt_seal;
>
> static const char * const check_usage[] = {
> "objtool check [<options>] file.o",
> @@ -50,6 +50,7 @@ const struct option check_options[] = {
> OPT_BOOLEAN(0, "dry-run", &dryrun, "don't write the modifications"),
> OPT_BOOLEAN(0, "ibt", &ibt, "validate ENDBR placement"),
> OPT_BOOLEAN(0, "ibt-fix-direct", &ibt_fix_direct, "fixup direct jmp/call to ENDBR"),
> + OPT_BOOLEAN(0, "ibt-seal", &ibt_seal, "list superfluous ENDBR instructions"),
s/list/annotate/ ?
Not sure "ibt-seal" is the appropriate name since the "seal" is done at
boot time.
Do we really need a separate option anyway? To get the full benefits of
IBT you might as well enable it... And always enabling it helps flush
out bugs quicker.
--
Josh
On Sat, Feb 26, 2022 at 07:46:13PM -0800, Josh Poimboeuf wrote:
> On Thu, Feb 24, 2022 at 03:52:14PM +0100, Peter Zijlstra wrote:
> > +#ifdef CONFIG_X86_KERNEL_IBT
> > + . = ALIGN(8);
> > + .ibt_endbr_sites : AT(ADDR(.ibt_endbr_sites) - LOAD_OFFSET) {
> > + __ibt_endbr_sites = .;
> > + *(.ibt_endbr_sites)
> > + __ibt_endbr_sites_end = .;
> > + }
> > +#endif
>
> ".ibt_endbr_superfluous" maybe? It's not *all* the endbr sites.
Since I like seals, I'll make it .ibt_endbr_seal :-) Also goes well with
the option at hand.
> > +
> > /*
> > * struct alt_inst entries. From the header (alternative.h):
> > * "Alternative instructions for different CPU types or capabilities"
> > --- a/tools/objtool/builtin-check.c
> > +++ b/tools/objtool/builtin-check.c
> > @@ -21,7 +21,7 @@
> >
> > bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
> > lto, vmlinux, mcount, noinstr, backup, sls, dryrun,
> > - ibt, ibt_fix_direct;
> > + ibt, ibt_fix_direct, ibt_seal;
> >
> > static const char * const check_usage[] = {
> > "objtool check [<options>] file.o",
> > @@ -50,6 +50,7 @@ const struct option check_options[] = {
> > OPT_BOOLEAN(0, "dry-run", &dryrun, "don't write the modifications"),
> > OPT_BOOLEAN(0, "ibt", &ibt, "validate ENDBR placement"),
> > OPT_BOOLEAN(0, "ibt-fix-direct", &ibt_fix_direct, "fixup direct jmp/call to ENDBR"),
> > + OPT_BOOLEAN(0, "ibt-seal", &ibt_seal, "list superfluous ENDBR instructions"),
>
> s/list/annotate/ ?
Done :-)
> Not sure "ibt-seal" is the appropriate name since the "seal" is done at
> boot time.
It allows sealing; it finds the locations to seal, whatever :-)
> Do we really need a separate option anyway? To get the full benefits of
> IBT you might as well enable it... And always enabling it helps flush
> out bugs quicker.
Are you asking about --ibt and --ibt-seal or about the existence of
X86_KERNEL_IBT_SEAL here?
The Makefiles will only ever use --ibt and --ibt-seal together for the
reason you state. The reason they're two separate objtool arguments is
because it's stictly speaking two different things being done. Also
--ibt as such is invariant, while --ibt-seal causes modifications to the
object file (which can be discarded using the new --dry-run I suppose).
The reason X86_KERNEL_IBT_SEAL exists is because that requires objtool
while X86_KERNEL_IBT does not -- you seemed to favour not hard relying
on having objtool present.
On Mon, Feb 28, 2022 at 01:41:13PM +0100, Peter Zijlstra wrote:
> On Sat, Feb 26, 2022 at 07:46:13PM -0800, Josh Poimboeuf wrote:
> > On Thu, Feb 24, 2022 at 03:52:14PM +0100, Peter Zijlstra wrote:
> > > +#ifdef CONFIG_X86_KERNEL_IBT
> > > + . = ALIGN(8);
> > > + .ibt_endbr_sites : AT(ADDR(.ibt_endbr_sites) - LOAD_OFFSET) {
> > > + __ibt_endbr_sites = .;
> > > + *(.ibt_endbr_sites)
> > > + __ibt_endbr_sites_end = .;
> > > + }
> > > +#endif
> >
> > ".ibt_endbr_superfluous" maybe? It's not *all* the endbr sites.
>
> Since I like seals, I'll make it .ibt_endbr_seal :-) Also goes well with
> the option at hand.
Sounds good.
>
> > > +
> > > /*
> > > * struct alt_inst entries. From the header (alternative.h):
> > > * "Alternative instructions for different CPU types or capabilities"
> > > --- a/tools/objtool/builtin-check.c
> > > +++ b/tools/objtool/builtin-check.c
> > > @@ -21,7 +21,7 @@
> > >
> > > bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
> > > lto, vmlinux, mcount, noinstr, backup, sls, dryrun,
> > > - ibt, ibt_fix_direct;
> > > + ibt, ibt_fix_direct, ibt_seal;
> > >
> > > static const char * const check_usage[] = {
> > > "objtool check [<options>] file.o",
> > > @@ -50,6 +50,7 @@ const struct option check_options[] = {
> > > OPT_BOOLEAN(0, "dry-run", &dryrun, "don't write the modifications"),
> > > OPT_BOOLEAN(0, "ibt", &ibt, "validate ENDBR placement"),
> > > OPT_BOOLEAN(0, "ibt-fix-direct", &ibt_fix_direct, "fixup direct jmp/call to ENDBR"),
> > > + OPT_BOOLEAN(0, "ibt-seal", &ibt_seal, "list superfluous ENDBR instructions"),
> >
> > s/list/annotate/ ?
>
> Done :-)
>
> > Not sure "ibt-seal" is the appropriate name since the "seal" is done at
> > boot time.
>
> It allows sealing; it finds the locations to seal, whatever :-)
Fair enough :-)
> > Do we really need a separate option anyway? To get the full benefits of
> > IBT you might as well enable it... And always enabling it helps flush
> > out bugs quicker.
>
> Are you asking about --ibt and --ibt-seal or about the existence of
> X86_KERNEL_IBT_SEAL here?
Both.
> The Makefiles will only ever use --ibt and --ibt-seal together for the
> reason you state. The reason they're two separate objtool arguments is
> because it's stictly speaking two different things being done. Also
> --ibt as such is invariant, while --ibt-seal causes modifications to the
> object file (which can be discarded using the new --dry-run I suppose).
Ok, but I wanted to avoid option sprawl. I don't see a reason to
separate them.
> The reason X86_KERNEL_IBT_SEAL exists is because that requires objtool
> while X86_KERNEL_IBT does not -- you seemed to favour not hard relying
> on having objtool present.
Hm, either you misunderstood, I misspoke, or I have short term memory
loss. Objtool is already hopelessly intertwined with x86. I'd rather
not have the extra option.
--
Josh