2022-05-26 18:04:30

by Peter Zijlstra

[permalink] [raw]
Subject: [PATCH 2/7] objtool: Extend UNWIND_HINT based ENDBR rules

Extend the UNWIND hint driven rules for ENDBR placement. Currently
objtool expects an ENDBR at any UNWINT_HINT_IRET_REGS that is at +0 of
an STB_GLOBAL symbol, with the expectation that this is an exception
entry point.

Extend this to also expect ENDBR at UNWIND_HINT_EMPTY at +0 for
STB_GLOBAL symbols, with the expectation that these are also machine
entry points (SYSCALL et. al.).

This guarantees all machine entry points are covered by objtool rules at
the expense of a few additional false positives:

vmlinux.o: warning: objtool: startup_64+0x0: UNWIND_HINT_EMPTY without ENDBR
vmlinux.o: warning: objtool: start_cpu0+0x0: UNWIND_HINT_EMPTY without ENDBR

Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/kernel/head_64.S | 2 ++
tools/objtool/check.c | 23 ++++++++++++++++++-----
2 files changed, 20 insertions(+), 5 deletions(-)

--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -43,6 +43,7 @@ L3_START_KERNEL = pud_index(__START_KERN
.code64
SYM_CODE_START_NOALIGN(startup_64)
UNWIND_HINT_EMPTY
+ ANNOTATE_NOENDBR
/*
* At this point the CPU runs in 64bit mode CS.L = 1 CS.D = 0,
* and someone has loaded an identity mapped page table
@@ -371,6 +372,7 @@ SYM_CODE_END(secondary_startup_64)
*/
SYM_CODE_START(start_cpu0)
UNWIND_HINT_EMPTY
+ ANNOTATE_NOENDBR
movq initial_stack(%rip), %rsp
jmp .Ljump_to_C_code
SYM_CODE_END(start_cpu0)
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1947,14 +1947,27 @@ static int read_unwind_hints(struct objt

insn->hint = true;

- if (opts.ibt && hint->type == UNWIND_HINT_TYPE_REGS_PARTIAL) {
+ if (opts.ibt) {
struct symbol *sym = find_symbol_by_offset(insn->sec, insn->offset);

- if (sym && sym->bind == STB_GLOBAL &&
- insn->type != INSN_ENDBR && !insn->noendbr) {
- WARN_FUNC("UNWIND_HINT_IRET_REGS without ENDBR",
- insn->sec, insn->offset);
+ if (!sym || sym->bind != STB_GLOBAL)
+ goto no_entry;
+
+ if (hint->type == UNWIND_HINT_TYPE_CALL && !hint->sp_reg) {
+ if (insn->type != INSN_ENDBR && !insn->noendbr) {
+ WARN_FUNC("UNWIND_HINT_EMPTY without ENDBR",
+ insn->sec, insn->offset);
+ }
+ }
+
+ if (hint->type == UNWIND_HINT_TYPE_REGS_PARTIAL) {
+ if (insn->type != INSN_ENDBR && !insn->noendbr) {
+ WARN_FUNC("UNWIND_HINT_IRET_REGS without ENDBR",
+ insn->sec, insn->offset);
+ }
}
+
+no_entry: ;
}

if (hint->type == UNWIND_HINT_TYPE_FUNC) {




2022-05-28 19:37:00

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [PATCH 2/7] objtool: Extend UNWIND_HINT based ENDBR rules

On Thu, May 26, 2022 at 12:52:54PM +0200, Peter Zijlstra wrote:
> Extend the UNWIND hint driven rules for ENDBR placement. Currently
> objtool expects an ENDBR at any UNWINT_HINT_IRET_REGS that is at +0 of
> an STB_GLOBAL symbol, with the expectation that this is an exception
> entry point.
>
> Extend this to also expect ENDBR at UNWIND_HINT_EMPTY at +0 for
> STB_GLOBAL symbols, with the expectation that these are also machine
> entry points (SYSCALL et. al.).
>
> This guarantees all machine entry points are covered by objtool rules at
> the expense of a few additional false positives:
>
> vmlinux.o: warning: objtool: startup_64+0x0: UNWIND_HINT_EMPTY without ENDBR
> vmlinux.o: warning: objtool: start_cpu0+0x0: UNWIND_HINT_EMPTY without ENDBR
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>

I can't remember if this was my bright idea, but it feels kind of
arbitrary. Hopefully there won't be a lot of false positives.

Anyway, won't SYSCALL-type symbols typically be referenced elsewhere in
the kernel and thus be found by the regular IBT validation?

Do you have any examples of where this warning would trigger if there
were a missing ENDBR?

--
Josh

2022-05-28 20:13:50

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH 2/7] objtool: Extend UNWIND_HINT based ENDBR rules

On Thu, May 26, 2022 at 08:05:49AM -0700, Josh Poimboeuf wrote:
> On Thu, May 26, 2022 at 12:52:54PM +0200, Peter Zijlstra wrote:
> > Extend the UNWIND hint driven rules for ENDBR placement. Currently
> > objtool expects an ENDBR at any UNWINT_HINT_IRET_REGS that is at +0 of
> > an STB_GLOBAL symbol, with the expectation that this is an exception
> > entry point.
> >
> > Extend this to also expect ENDBR at UNWIND_HINT_EMPTY at +0 for
> > STB_GLOBAL symbols, with the expectation that these are also machine
> > entry points (SYSCALL et. al.).
> >
> > This guarantees all machine entry points are covered by objtool rules at
> > the expense of a few additional false positives:
> >
> > vmlinux.o: warning: objtool: startup_64+0x0: UNWIND_HINT_EMPTY without ENDBR
> > vmlinux.o: warning: objtool: start_cpu0+0x0: UNWIND_HINT_EMPTY without ENDBR
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>
> I can't remember if this was my bright idea, but it feels kind of
> arbitrary. Hopefully there won't be a lot of false positives.

The existing UNWIND_HINT_IRET_REGS at +0 was your idea, I'm just trying
to cover more.

> Anyway, won't SYSCALL-type symbols typically be referenced elsewhere in
> the kernel and thus be found by the regular IBT validation?

They do indeed, and that's what we've been relying on. I just figured it
would be more consistent to have rules covering all machine entry
points.

(also all the Xen entry points are EMPTY like).

> Do you have any examples of where this warning would trigger if there
> were a missing ENDBR?

No.

Anyway, I can drop these first two patches for now.