linux-kernel - Re: New objtool warning..

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Dec 2020 14:01:58 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Peter Zijlstra <peterz@...radead.org>, linux-kernel@...r.kernel.org
Subject: Re: New objtool warning..

On Tue, Dec 15, 2020 at 09:32:29PM -0800, Linus Torvalds wrote:
> > The asm code looks like this:
> >
> >         cmpb    $4, %al #, _30
> >         jne     .L176   #,
> > ...
> >         cmpb    $12, %al        #, _30
> >         jne     .L176   #,
> > ...
> > .L176:
> > # drivers/gpu/drm/drm_edid.c:3118:                      unreachable();
> > #APP
> > # 3118 "drivers/gpu/drm/drm_edid.c" 1
> >         320:    #
> >         .pushsection .discard.unreachable
> >         .long 320b - .  #
> >         .popsection
> > # 0 "" 2
> > #NO_APP
> >    .. this falls through..
> >
> > So you *should* find that label that then falls through in that
> > ".discard.unreachable" section, and so it should be possible to teach
> > objdump about that (insane) unreachable code that way. No?

So this is kind of tricky, because the unreachable() annotation usually
means "the previous instruction is a dead end".  Most of the time, the
next instruction -- the one actually pointed to by the annotation -- is
actually reachable from another path.

For example, here's a typical usage of unreachable():

	je not_a_bug
	ud2

	.pushsection .discard.unreachable
	.long not_a_bug - .
	.popsection

not_a_bug:
	... normal non-buggy code ...

The 'not_a_bug' label is pointed to by the unreachable annotation, but
it's actually reachable.

In your .o, .discard.unreachable points to 0xbb3, so objtool marks the
previous instruction (0xbae) as a dead end:

     bae:       e9 30 ff ff ff          jmpq   ae3 <do_cvt_mode+0xd3>
     bb3:       66 66 2e 0f 1f 84 00    data16 nopw %cs:0x0(%rax,%rax,1)
     bba:       00 00 00 00
     bbe:       66 90                   xchg   %ax,%ax

And there's another path to 0xbb3 from the switch statement, so objtool
assumes it's reachable.

So maybe we need to make objtool's unreachable logic a little more
nuanced: If the previous instruction is an unconditional jump, then
consider *the annotated instruction itself* to be a dead end.

I'm not quite able to convince myself this wouldn't produce false
positives.  It did immediately produce one false positive in
no_context(), but that should be easily fixable (see patch).

I can run it through more testing, if you don't see any obvious problems
with it.

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index f1f1b5a0956a..c888821bb40c 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -699,7 +699,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 		 */
 		asm volatile ("movq %[stack], %%rsp\n\t"
 			      "call handle_stack_overflow\n\t"
-			      "1: jmp 1b"
 			      : ASM_CALL_CONSTRAINT
 			      : "D" ("kernel stack overflow (page fault)"),
 				"S" (regs), "d" (address),
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c6ab44543c92..267e8b88ca3a 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -370,9 +370,12 @@ static int add_dead_ends(struct objtool_file *file)
 			return -1;
 		}
 		insn = find_insn(file, reloc->sym->sec, reloc->addend);
-		if (insn)
-			insn = list_prev_entry(insn, list);
-		else if (reloc->addend == reloc->sym->sec->len) {
+		if (insn) {
+			struct instruction *prev = list_prev_entry(insn, list);
+			if (prev->type != INSN_JUMP_UNCONDITIONAL)
+				insn = prev;
+
+		} else if (reloc->addend == reloc->sym->sec->len) {
 			insn = find_last_insn(file, reloc->sym->sec);
 			if (!insn) {
 				WARN("can't find unreachable insn at %s+0x%x",