| 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
| ||
|
Message-ID: <20201216200158.akf356yrw44o2rlb@treble>
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",
Powered by blists - more mailing lists