* x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)
@ 2020-03-01 6:02 Jann Horn
2020-03-02 14:58 ` Peter Zijlstra
2020-03-02 15:18 ` Josh Poimboeuf
0 siblings, 2 replies; 18+ messages in thread
From: Jann Horn @ 2020-03-01 6:02 UTC (permalink / raw)
To: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, the arch/x86 maintainers
Cc: kernel list, Josh Poimboeuf
Hi!
With the latest kernel from Linus' tree (fb279f4e2386), on a box that
needs KPTI, as soon as I run "sudo <path>/perf top -g", I see this
warning (if I modify the unwinding code to use %px there instead of
%p):
[ 83.716065] WARNING: stack recursion on stack type 4
[ 83.716070] WARNING: can't dereference registers at
fffffe0000002190 for ip
swapgs_restore_regs_and_return_to_usermode+0x79/0x80
So apparently some ORC annotations are a bit off. I decided to write a
little helper script that can interleave "objtool orc dump" and
"objdump -d" output to make it easier for me to understand what's
going on; you can save it to tools/objtool and run it from there:
==============
#!/usr/bin/env python3
import subprocess
import sys
import re
objfile = sys.argv[1]
orc_dump = subprocess.run([sys.path[0]+'/objtool', 'orc', 'dump',
objfile], stdout=subprocess.PIPE).stdout.decode('utf-8')
orc_map = {}
for line in orc_dump.split('\n'):
parts = line.split(': ', 1)
if len(parts) != 2:
continue
orc_map[parts[0]] = parts[1]
objdump = subprocess.run(['objdump', '-d', objfile],
stdout=subprocess.PIPE).stdout.decode('utf-8')
cur_section = None
hex_re = re.compile('^[a-f0-9]+$')
for line in objdump.split('\n'):
if line.startswith('Disassembly of section '):
cur_section = line.split('of section ')[1].split(':')[0]
if line.find(':') != -1:
hex_prefix = line.split(':')[0].lstrip()
if hex_re.match(hex_prefix):
orc_val = orc_map.get(cur_section+'+'+hex_prefix)
if orc_val != None:
print('\033[92m#######' + orc_val + '\033[0m')
print(line)
==============
swapgs_restore_regs_and_return_to_usermode+0x79/0x80 is offset 0xaa8
in the object file. Here's the interleaved assembly and ORC unwind
info (with ORC info appearing above the line it refers to):
==============
0000000000000a00 <common_interrupt>:
#######sp:sp+8 bp:(und) type:iret end:0
a00: 48 83 04 24 80 addq $0xffffffffffffff80,(%rsp)
a05: e8 00 00 00 00 callq a0a <common_interrupt+0xa>
#######sp:(sp+0) bp:(und) type:regs end:0
a0a: e8 00 00 00 00 callq a0f <ret_from_intr>
0000000000000a0f <ret_from_intr>:
a0f: fa cli
a10: 5c pop %rsp
#######sp:sp+0 bp:(und) type:regs end:0
a11: 65 ff 0c 25 00 00 00 decl %gs:0x0
a18: 00
a19: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp)
a20: 03
a21: 0f 84 88 00 00 00 je aaf <retint_kernel>
a27: 48 89 e7 mov %rsp,%rdi
a2a: e8 00 00 00 00 callq a2f
<swapgs_restore_regs_and_return_to_usermode>
0000000000000a2f <swapgs_restore_regs_and_return_to_usermode>:
a2f: 41 5f pop %r15
#######sp:sp-8 bp:(und) type:regs end:0
a31: 41 5e pop %r14
#######sp:sp-16 bp:(und) type:regs end:0
a33: 41 5d pop %r13
#######sp:sp-24 bp:(und) type:regs end:0
a35: 41 5c pop %r12
#######sp:sp-32 bp:(und) type:regs end:0
a37: 5d pop %rbp
#######sp:sp-40 bp:(und) type:regs end:0
a38: 5b pop %rbx
#######sp:sp-48 bp:(und) type:regs end:0
a39: 41 5b pop %r11
#######sp:sp-56 bp:(und) type:regs end:0
a3b: 41 5a pop %r10
#######sp:sp-64 bp:(und) type:regs end:0
a3d: 41 59 pop %r9
#######sp:sp-72 bp:(und) type:regs end:0
a3f: 41 58 pop %r8
#######sp:sp-80 bp:(und) type:regs end:0
a41: 58 pop %rax
#######sp:sp-88 bp:(und) type:regs end:0
a42: 59 pop %rcx
#######sp:sp-96 bp:(und) type:regs end:0
a43: 5a pop %rdx
#######sp:sp-104 bp:(und) type:regs end:0
a44: 5e pop %rsi
#######sp:sp-112 bp:(und) type:regs end:0
a45: 48 89 e7 mov %rsp,%rdi
a48: 65 48 8b 24 25 00 00 mov %gs:0x0,%rsp
a4f: 00 00
a51: ff 77 30 pushq 0x30(%rdi)
#######sp:sp-104 bp:(und) type:regs end:0
a54: ff 77 28 pushq 0x28(%rdi)
#######sp:sp-96 bp:(und) type:regs end:0
a57: ff 77 20 pushq 0x20(%rdi)
#######sp:sp-88 bp:(und) type:regs end:0
a5a: ff 77 18 pushq 0x18(%rdi)
#######sp:sp-80 bp:(und) type:regs end:0
a5d: ff 77 10 pushq 0x10(%rdi)
#######sp:sp-72 bp:(und) type:regs end:0
a60: ff 37 pushq (%rdi)
#######sp:sp-64 bp:(und) type:regs end:0
a62: 50 push %rax
#######sp:sp-56 bp:(und) type:regs end:0
a63: eb 43 jmp aa8
<swapgs_restore_regs_and_return_to_usermode+0x79>
a65: 0f 20 df mov %cr3,%rdi
a68: eb 34 jmp a9e
<swapgs_restore_regs_and_return_to_usermode+0x6f>
a6a: 48 89 f8 mov %rdi,%rax
a6d: 48 81 e7 ff 07 00 00 and $0x7ff,%rdi
a74: 65 48 0f a3 3c 25 00 bt %rdi,%gs:0x0
a7b: 00 00 00
a7e: 73 0f jae a8f
<swapgs_restore_regs_and_return_to_usermode+0x60>
a80: 65 48 0f b3 3c 25 00 btr %rdi,%gs:0x0
a87: 00 00 00
a8a: 48 89 c7 mov %rax,%rdi
a8d: eb 08 jmp a97
<swapgs_restore_regs_and_return_to_usermode+0x68>
a8f: 48 89 c7 mov %rax,%rdi
a92: 48 0f ba ef 3f bts $0x3f,%rdi
a97: 48 81 cf 00 08 00 00 or $0x800,%rdi
a9e: 48 81 cf 00 10 00 00 or $0x1000,%rdi
aa5: 0f 22 df mov %rdi,%cr3
aa8: 58 pop %rax
#######sp:sp-64 bp:(und) type:regs end:0
aa9: 5f pop %rdi
#######sp:sp-72 bp:(und) type:regs end:0
aaa: 0f 01 f8 swapgs
aad: eb 41 jmp af0 <native_iret>
==============
It looks to me like things go wrong at the point where we switch over
to the trampoline stack? The ORC info claims that we have full user
registers on the trampoline stack (and that we're clobbering them with
our pushes - apparently objtool is not smart enough to realize that
that looks bogus), but at that point we should probably actually use
something like UNWIND_HINT_IRET_REGS, right?
By the way, looking through the rest of the entry stuff, there's some
other funny-looking stuff, too:
============
0000000000000f40 <general_protection>:
#######sp:sp+8 bp:(und) type:iret end:0
f40: 90 nop
#######sp:(und) bp:(und) type:call end:0
f41: 90 nop
f42: 90 nop
#######sp:sp+8 bp:(und) type:iret end:0
f43: e8 a8 01 00 00 callq 10f0 <error_entry>
#######sp:sp+0 bp:(und) type:regs end:0
f48: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp)
f4f: 03
f50: 74 00 je f52 <general_protection+0x12>
f52: 48 89 e7 mov %rsp,%rdi
f55: 48 8b 74 24 78 mov 0x78(%rsp),%rsi
f5a: 48 c7 44 24 78 ff ff movq $0xffffffffffffffff,0x78(%rsp)
f61: ff ff
f63: e8 00 00 00 00 callq f68 <general_protection+0x28>
f68: e9 73 02 00 00 jmpq 11e0 <error_exit>
#######sp:(und) bp:(und) type:call end:0
f6d: 0f 1f 00 nopl (%rax)
============
So I think on machines without X86_FEATURE_SMAP, trying to unwind from
the two NOPs at f41 and f42 will cause the unwinder to report an
error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1"
marker seems to be reserved for errors.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-03-01 6:02 x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) Jann Horn @ 2020-03-02 14:58 ` Peter Zijlstra 2020-03-02 15:18 ` Josh Poimboeuf 1 sibling, 0 replies; 18+ messages in thread From: Peter Zijlstra @ 2020-03-02 14:58 UTC (permalink / raw) To: Jann Horn Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, Josh Poimboeuf On Sun, Mar 01, 2020 at 07:02:15AM +0100, Jann Horn wrote: > 0000000000000a2f <swapgs_restore_regs_and_return_to_usermode>: > a2f: 41 5f pop %r15 > #######sp:sp-8 bp:(und) type:regs end:0 > a31: 41 5e pop %r14 > #######sp:sp-16 bp:(und) type:regs end:0 > a33: 41 5d pop %r13 > #######sp:sp-24 bp:(und) type:regs end:0 > a35: 41 5c pop %r12 > #######sp:sp-32 bp:(und) type:regs end:0 > a37: 5d pop %rbp > #######sp:sp-40 bp:(und) type:regs end:0 > a38: 5b pop %rbx > #######sp:sp-48 bp:(und) type:regs end:0 > a39: 41 5b pop %r11 > #######sp:sp-56 bp:(und) type:regs end:0 > a3b: 41 5a pop %r10 > #######sp:sp-64 bp:(und) type:regs end:0 > a3d: 41 59 pop %r9 > #######sp:sp-72 bp:(und) type:regs end:0 > a3f: 41 58 pop %r8 > #######sp:sp-80 bp:(und) type:regs end:0 > a41: 58 pop %rax > #######sp:sp-88 bp:(und) type:regs end:0 > a42: 59 pop %rcx > #######sp:sp-96 bp:(und) type:regs end:0 > a43: 5a pop %rdx > #######sp:sp-104 bp:(und) type:regs end:0 > a44: 5e pop %rsi > #######sp:sp-112 bp:(und) type:regs end:0 > a45: 48 89 e7 mov %rsp,%rdi > a48: 65 48 8b 24 25 00 00 mov %gs:0x0,%rsp > a4f: 00 00 Right, so here we flip stacks, > a51: ff 77 30 pushq 0x30(%rdi) > #######sp:sp-104 bp:(und) type:regs end:0 > a54: ff 77 28 pushq 0x28(%rdi) > #######sp:sp-96 bp:(und) type:regs end:0 > a57: ff 77 20 pushq 0x20(%rdi) > #######sp:sp-88 bp:(und) type:regs end:0 > a5a: ff 77 18 pushq 0x18(%rdi) > #######sp:sp-80 bp:(und) type:regs end:0 > a5d: ff 77 10 pushq 0x10(%rdi) And here we've pushed an IRET frame > #######sp:sp-72 bp:(und) type:regs end:0 > a60: ff 37 pushq (%rdi) > It looks to me like things go wrong at the point where we switch over > to the trampoline stack? The ORC info claims that we have full user > registers on the trampoline stack (and that we're clobbering them with > our pushes - apparently objtool is not smart enough to realize that > that looks bogus), but at that point we should probably actually use > something like UNWIND_HINT_IRET_REGS, right? I _think_ you've nailed it, but I'm somewhat new to this part of objtool. Josh? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-03-01 6:02 x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) Jann Horn 2020-03-02 14:58 ` Peter Zijlstra @ 2020-03-02 15:18 ` Josh Poimboeuf 2020-03-02 15:52 ` Josh Poimboeuf 1 sibling, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-03-02 15:18 UTC (permalink / raw) To: Jann Horn Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list On Sun, Mar 01, 2020 at 07:02:15AM +0100, Jann Horn wrote: > It looks to me like things go wrong at the point where we switch over > to the trampoline stack? The ORC info claims that we have full user > registers on the trampoline stack (and that we're clobbering them with > our pushes - apparently objtool is not smart enough to realize that > that looks bogus), but at that point we should probably actually use > something like UNWIND_HINT_IRET_REGS, right? Good timing. I have a patch set coming in a few days which fixes several ORC issues, and this was one of them. > By the way, looking through the rest of the entry stuff, there's some > other funny-looking stuff, too: > > ============ > 0000000000000f40 <general_protection>: > #######sp:sp+8 bp:(und) type:iret end:0 > f40: 90 nop > #######sp:(und) bp:(und) type:call end:0 > f41: 90 nop > f42: 90 nop > #######sp:sp+8 bp:(und) type:iret end:0 > f43: e8 a8 01 00 00 callq 10f0 <error_entry> > #######sp:sp+0 bp:(und) type:regs end:0 > f48: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp) > f4f: 03 > f50: 74 00 je f52 <general_protection+0x12> > f52: 48 89 e7 mov %rsp,%rdi > f55: 48 8b 74 24 78 mov 0x78(%rsp),%rsi > f5a: 48 c7 44 24 78 ff ff movq $0xffffffffffffffff,0x78(%rsp) > f61: ff ff > f63: e8 00 00 00 00 callq f68 <general_protection+0x28> > f68: e9 73 02 00 00 jmpq 11e0 <error_exit> > #######sp:(und) bp:(und) type:call end:0 > f6d: 0f 1f 00 nopl (%rax) > ============ > > So I think on machines without X86_FEATURE_SMAP, trying to unwind from > the two NOPs at f41 and f42 will cause the unwinder to report an > error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1" > marker seems to be reserved for errors. Hm... good catch. Not sure why objtool is doing that but I'll look into it. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-03-02 15:18 ` Josh Poimboeuf @ 2020-03-02 15:52 ` Josh Poimboeuf 2020-04-28 7:04 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-03-02 15:52 UTC (permalink / raw) To: Jann Horn Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list On Mon, Mar 02, 2020 at 09:18:29AM -0600, Josh Poimboeuf wrote: > > So I think on machines without X86_FEATURE_SMAP, trying to unwind from > > the two NOPs at f41 and f42 will cause the unwinder to report an > > error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1" > > marker seems to be reserved for errors. I think we can blame this one on Peter ;-) 764eef4b109a ("objtool: Rewrite alt->skip_orig") With X86_FEATURE_SMAP, alt->skip_orig gets set, which tells objtool to skip validation of the NOPs. That has the side effect of not propagating the ORC state to the NOPs as well. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-03-02 15:52 ` Josh Poimboeuf @ 2020-04-28 7:04 ` Josh Poimboeuf 2020-04-28 12:46 ` Peter Zijlstra 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-04-28 7:04 UTC (permalink / raw) To: Jann Horn Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, Peter Zijlstra On Mon, Mar 02, 2020 at 09:52:40AM -0600, Josh Poimboeuf wrote: > On Mon, Mar 02, 2020 at 09:18:29AM -0600, Josh Poimboeuf wrote: > > > So I think on machines without X86_FEATURE_SMAP, trying to unwind from > > > the two NOPs at f41 and f42 will cause the unwinder to report an > > > error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1" > > > marker seems to be reserved for errors. > > I think we can blame this one on Peter ;-) > > 764eef4b109a ("objtool: Rewrite alt->skip_orig") > > With X86_FEATURE_SMAP, alt->skip_orig gets set, which tells objtool to > skip validation of the NOPs. That has the side effect of not > propagating the ORC state to the NOPs as well. I almost forgot about this one, until I rediscovered it just now... Peter, I just realized you weren't CCed on the original email, oops. I'm thinking something like this should fix it. Peter, does this look ok? (I should probably split it into two patches: STAC/CLAC fix and then revert 764eef4b109a.) From: Josh Poimboeuf <jpoimboe@redhat.com> Subject: [PATCH] objtool: Fix ORC gap in STAC/CLAC alternatives Since commit 764eef4b109a ("objtool: Rewrite alt->skip_orig"), the new skip_orig actually skips objtool validation of the original instruction, resulting in an empty gap in the ORC data. Revert the skip_orig logic to how it was before, which is to convert the original instruction to a NOP. For the SMAP case, convert the original instruction to a matching STAC/CLAC, so that it doesn't matter which path is taken. Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig") Reported-by: Jann Horn <jannh@google.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- tools/objtool/check.c | 60 ++++++++++++++++++++++++----------------- tools/objtool/special.c | 16 ----------- tools/objtool/special.h | 1 - 3 files changed, 35 insertions(+), 42 deletions(-) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 0c732d586924..541fcf16283c 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -23,7 +23,6 @@ struct alternative { struct list_head list; struct instruction *insn; - bool skip_orig; }; const char *objname; @@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file, struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL; unsigned long dest_off; + /* + * For uaccess checking, propagate the STAC/CLAC from the alternative + * to the original insn to avoid paths where we see the STAC but then + * take the NOP instead of CLAC (and vice versa). + */ + if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP && + *new_insn && + ((*new_insn)->type == INSN_STAC || + (*new_insn)->type == INSN_CLAC)) + orig_insn->type = (*new_insn)->type; + last_orig_insn = NULL; insn = orig_insn; sec_for_each_insn_from(file, insn) { if (insn->offset >= special_alt->orig_off + special_alt->orig_len) break; + if (special_alt->skip_orig) + insn->type = INSN_NOP; + insn->alt_group = true; last_orig_insn = insn; } @@ -970,8 +983,6 @@ static int add_special_section_alts(struct objtool_file *file) } alt->insn = new_insn; - alt->skip_orig = special_alt->skip_orig; - orig_insn->ignore_alts |= special_alt->skip_alt; list_add_tail(&alt->list, &orig_insn->alts); list_del(&special_alt->list); @@ -2221,12 +2232,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, insn->visited |= visited; if (!insn->ignore_alts) { - bool skip_orig = false; - list_for_each_entry(alt, &insn->alts, list) { - if (alt->skip_orig) - skip_orig = true; - ret = validate_branch(file, func, alt->insn, state); if (ret) { if (backtrace) @@ -2234,9 +2240,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, return ret; } } - - if (skip_orig) - return 0; } switch (insn->type) { @@ -2325,26 +2328,33 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, break; case INSN_STAC: - if (state.uaccess) { - WARN_FUNC("recursive UACCESS enable", sec, insn->offset); - return 1; - } + if (uaccess) { + if (state.uaccess) { + WARN_FUNC("recursive UACCESS enable", + sec, insn->offset); + return 1; + } - state.uaccess = true; + state.uaccess = true; + } break; case INSN_CLAC: - if (!state.uaccess && func) { - WARN_FUNC("redundant UACCESS disable", sec, insn->offset); - return 1; - } + if (uaccess) { + if (!state.uaccess && func) { + WARN_FUNC("redundant UACCESS disable", + sec, insn->offset); + return 1; + } - if (func_uaccess_safe(func) && !state.uaccess_stack) { - WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset); - return 1; - } + if (func_uaccess_safe(func) && !state.uaccess_stack) { + WARN_FUNC("UACCESS-safe disables UACCESS", + sec, insn->offset); + return 1; + } - state.uaccess = false; + state.uaccess = false; + } break; case INSN_STD: diff --git a/tools/objtool/special.c b/tools/objtool/special.c index e74e0189de22..ec7adbd32d57 100644 --- a/tools/objtool/special.c +++ b/tools/objtool/special.c @@ -100,22 +100,6 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry, */ if (feature == X86_FEATURE_POPCNT) alt->skip_orig = true; - - /* - * If UACCESS validation is enabled; force that alternative; - * otherwise force it the other way. - * - * What we want to avoid is having both the original and the - * alternative code flow at the same time, in that case we can - * find paths that see the STAC but take the NOP instead of - * CLAC and the other way around. - */ - if (feature == X86_FEATURE_SMAP) { - if (uaccess) - alt->skip_orig = true; - else - alt->skip_alt = true; - } } orig_rela = find_rela_by_dest(elf, sec, offset + entry->orig); diff --git a/tools/objtool/special.h b/tools/objtool/special.h index 35061530e46e..ac729a20042c 100644 --- a/tools/objtool/special.h +++ b/tools/objtool/special.h @@ -14,7 +14,6 @@ struct special_alt { bool group; bool skip_orig; - bool skip_alt; bool jump_or_nop; struct section *orig_sec; -- 2.21.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 7:04 ` Josh Poimboeuf @ 2020-04-28 12:46 ` Peter Zijlstra 2020-04-28 14:14 ` Josh Poimboeuf 2020-04-28 14:16 ` Peter Zijlstra 0 siblings, 2 replies; 18+ messages in thread From: Peter Zijlstra @ 2020-04-28 12:46 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 02:04:50AM -0500, Josh Poimboeuf wrote: > On Mon, Mar 02, 2020 at 09:52:40AM -0600, Josh Poimboeuf wrote: > > On Mon, Mar 02, 2020 at 09:18:29AM -0600, Josh Poimboeuf wrote: > > > > So I think on machines without X86_FEATURE_SMAP, trying to unwind from > > > > the two NOPs at f41 and f42 will cause the unwinder to report an > > > > error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1" > > > > marker seems to be reserved for errors. > > > > I think we can blame this one on Peter ;-) > > > > 764eef4b109a ("objtool: Rewrite alt->skip_orig") > > > > With X86_FEATURE_SMAP, alt->skip_orig gets set, which tells objtool to > > skip validation of the NOPs. That has the side effect of not > > propagating the ORC state to the NOPs as well. > > I almost forgot about this one, until I rediscovered it just now... > Peter, I just realized you weren't CCed on the original email, oops. Nah, I got them (through x86@); but I too lost track of it :/ > I'm thinking something like this should fix it. Peter, does this look > ok? Unfortunate. But also, I fear, insufficient. Specifically consider things like: ALTERNATIVE "jmp 1f", "alt... "..." "...insn", X86_FEAT_foo 1: This results in something like: .text .altinstr_replacement e8 xx ... 90 90 ... 90 Where all our normal single byte nops (0x90) are unreachable with undefined CFI, but the alternative might have CFI, which is never propagated. We ran into this with the validate_alternative stuff from Alexandre. > @@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file, > struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL; > unsigned long dest_off; > > + /* > + * For uaccess checking, propagate the STAC/CLAC from the alternative > + * to the original insn to avoid paths where we see the STAC but then > + * take the NOP instead of CLAC (and vice versa). > + */ > + if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP && > + *new_insn && > + ((*new_insn)->type == INSN_STAC || > + (*new_insn)->type == INSN_CLAC)) > + orig_insn->type = (*new_insn)->type; Also, this generates a mis-match between actual instruction text and type. We now have a single byte instruction (0x90) with the type of a 3 byte (SLAC/CLAC). Which currently isn't a problem, but I'm looking at adding infrastructure for having objtool rewrite instructions. So rather than hacking around this issue, should we not make create_orc() smarter? I'm trying to come up with something, but so far I'm just making a mess. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 12:46 ` Peter Zijlstra @ 2020-04-28 14:14 ` Josh Poimboeuf 2020-04-28 14:35 ` Peter Zijlstra 2020-04-28 14:16 ` Peter Zijlstra 1 sibling, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-04-28 14:14 UTC (permalink / raw) To: Peter Zijlstra Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote: > > I'm thinking something like this should fix it. Peter, does this look > > ok? > > Unfortunate. But also, I fear, insufficient. Specifically consider > things like: > > ALTERNATIVE "jmp 1f", > "alt... > "..." > "...insn", X86_FEAT_foo > 1: > > This results in something like: > > > .text .altinstr_replacement > e8 xx ... > 90 > 90 > ... > 90 > > Where all our normal single byte nops (0x90) are unreachable with > undefined CFI, but the alternative might have CFI, which is never > propagated. > > We ran into this with the validate_alternative stuff from Alexandre. I don't get what you're saying. We decided not to allow CFI changes in alternatives. And how does this relate to my patch? > > @@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file, > > struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL; > > unsigned long dest_off; > > > > + /* > > + * For uaccess checking, propagate the STAC/CLAC from the alternative > > + * to the original insn to avoid paths where we see the STAC but then > > + * take the NOP instead of CLAC (and vice versa). > > + */ > > + if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP && > > + *new_insn && > > + ((*new_insn)->type == INSN_STAC || > > + (*new_insn)->type == INSN_CLAC)) > > + orig_insn->type = (*new_insn)->type; > > Also, this generates a mis-match between actual instruction text and > type. We now have a single byte instruction (0x90) with the type of a 3 > byte (SLAC/CLAC). Which currently isn't a problem, but I'm looking at > adding infrastructure for having objtool rewrite instructions. But it doesn't actually change the original instruction bytes, it just changes the decoding. Is that really going to be a problem? We do that in other places as well, and it helps simplify code flow. Also might I ask why you're going to be rewriting instructions? That sounds scary. > So rather than hacking around this issue, should we not make > create_orc() smarter? Maybe, though I don't see how that logic belongs in create_orc(). It might be tricky distinguishing between normal undefined and "undefined because of a skip_orig". Right now create_orc() is blissfully ignorant. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 14:14 ` Josh Poimboeuf @ 2020-04-28 14:35 ` Peter Zijlstra 0 siblings, 0 replies; 18+ messages in thread From: Peter Zijlstra @ 2020-04-28 14:35 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 09:14:57AM -0500, Josh Poimboeuf wrote: > On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote: > > > I'm thinking something like this should fix it. Peter, does this look > > > ok? > > > > Unfortunate. But also, I fear, insufficient. Specifically consider > > things like: > > > > ALTERNATIVE "jmp 1f", > > "alt... > > "..." > > "...insn", X86_FEAT_foo > > 1: > > > > This results in something like: > > > > > > .text .altinstr_replacement > > e8 xx ... > > 90 > > 90 > > ... > > 90 > > > > Where all our normal single byte nops (0x90) are unreachable with > > undefined CFI, but the alternative might have CFI, which is never > > propagated. > > > > We ran into this with the validate_alternative stuff from Alexandre. > > I don't get what you're saying. We decided not to allow CFI changes in > alternatives. And how does this relate to my patch? Ah, I went with a slightly looser invariant rule that allows CFI but ensures they're the same for all alternatives, and the above orig text has a giant unreachable hole (that we don't report because NOP), I'm allowing the alternative CFI changes in that. Maybe that's too much leaway, but I'm thinking it ought to work. > > > @@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file, > > > struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL; > > > unsigned long dest_off; > > > > > > + /* > > > + * For uaccess checking, propagate the STAC/CLAC from the alternative > > > + * to the original insn to avoid paths where we see the STAC but then > > > + * take the NOP instead of CLAC (and vice versa). > > > + */ > > > + if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP && > > > + *new_insn && > > > + ((*new_insn)->type == INSN_STAC || > > > + (*new_insn)->type == INSN_CLAC)) > > > + orig_insn->type = (*new_insn)->type; > > > > Also, this generates a mis-match between actual instruction text and > > type. We now have a single byte instruction (0x90) with the type of a 3 > > byte (SLAC/CLAC). Which currently isn't a problem, but I'm looking at > > adding infrastructure for having objtool rewrite instructions. > > But it doesn't actually change the original instruction bytes, it just > changes the decoding. Is that really going to be a problem? We do that > in other places as well, and it helps simplify code flow. It will probably work just fine, it just feels off to me. > Also might I ask why you're going to be rewriting instructions? That > sounds scary. Variable length jump label support, I can't make gnu-as (I so hate that thing) emit the right instruction at compile-time :/ > > So rather than hacking around this issue, should we not make > > create_orc() smarter? > > Maybe, though I don't see how that logic belongs in create_orc(). It > might be tricky distinguishing between normal undefined and "undefined > because of a skip_orig". Right now create_orc() is blissfully ignorant. Yeah, you're right. I'll look for a better place to stick it. Perhaps I can frob it in validate_branch() somewhere. And you're also right on the unreachable because of skip_orig thing, I'll thnk about that. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 12:46 ` Peter Zijlstra 2020-04-28 14:14 ` Josh Poimboeuf @ 2020-04-28 14:16 ` Peter Zijlstra 2020-04-28 14:31 ` Josh Poimboeuf 1 sibling, 1 reply; 18+ messages in thread From: Peter Zijlstra @ 2020-04-28 14:16 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote: > On Tue, Apr 28, 2020 at 02:04:50AM -0500, Josh Poimboeuf wrote: > > I'm thinking something like this should fix it. Peter, does this look > > ok? > > Unfortunate. But also, I fear, insufficient. Specifically consider > things like: > > ALTERNATIVE "jmp 1f", > "alt... > "..." > "...insn", X86_FEAT_foo > 1: > > This results in something like: > > > .text .altinstr_replacement > e8 xx ... > 90 > 90 > ... > 90 > > Where all our normal single byte nops (0x90) are unreachable with > undefined CFI, but the alternative might have CFI, which is never > propagated. > > We ran into this with the validate_alternative stuff from Alexandre. > So rather than hacking around this issue, should we not make > create_orc() smarter? > > I'm trying to come up with something, but so far I'm just making a mess. Like this, it's horrid, but it seems to work. What do you think of the approach? I'll work on cleaning it up if you don't hate it too much ;-) --- diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c index 9d2bf2daaaa6..2a853ae994ea 100644 --- a/tools/objtool/orc_gen.c +++ b/tools/objtool/orc_gen.c @@ -10,17 +10,129 @@ #include "check.h" #include "warn.h" -int create_orc(struct objtool_file *file) +static bool same_cfi(struct cfi_state *a, struct cfi_state *b) { + return memcmp(a, b, sizeof(*a)); +} + +static struct instruction *next_insn_same_sec(struct objtool_file *file, + struct instruction *insn) +{ + struct instruction *next = list_next_entry(insn, list); + + if (!next || &next->list == &file->insn_list || next->sec != insn->sec) + return NULL; + + return next; +} + +struct alternative { + struct list_head list; struct instruction *insn; + bool skip_orig; +}; + +static int alt_cfi(struct objtool_file *file, + struct instruction *prev_insn, + struct instruction *orig_insn, + struct instruction *alt_insn) +{ + unsigned long orig_offset = orig_insn->offset; + unsigned long alt_offset = alt_insn->offset; + struct instruction *next_insn; + bool orig_orc, alt_orc; + + orig_orc = !prev_insn || same_cfi(&orig_insn->cfi, &prev_insn->cfi); + alt_orc = !prev_insn || same_cfi(&alt_insn->cfi, &prev_insn->cfi); + +again: + if ((orig_orc || alt_orc)) { + if (orig_insn->offset - orig_offset != alt_insn->offset - alt_offset) { + WARN_FUNC("alternative has unaligned ORC", orig_insn->sec, orig_insn->offset); + return -1; + } + + if (orig_insn->visited) { + if (same_cfi(&orig_insn->cfi, &alt_insn->cfi)) { + WARN_FUNC("alternative violates ORC invariance", orig_insn->sec, orig_insn->offset); + return -1; + } + } else { + /* + * We're in unreachable NOPs, allow the alternative to + * override the CFI/ORC data. + */ + orig_insn->cfi = alt_insn->cfi; + } + } + + next_insn = next_insn_same_sec(file, alt_insn); + if (!next_insn) + return 0; + + if (next_insn->offset == -1 /*FAKE_JUMP_OFFSET*/) + return 0; + + alt_orc = same_cfi(&alt_insn->cfi, &next_insn->cfi); + alt_insn = next_insn; + + do { + next_insn = next_insn_same_sec(file, orig_insn); + if (!next_insn) + return 0; + if (!next_insn->alt_group) + return 0; + + orig_orc = next_insn->visited && same_cfi(&orig_insn->cfi, &next_insn->cfi); + orig_insn = next_insn; + } while (orig_insn->offset - orig_offset < alt_insn->offset - alt_offset); + + goto again; +} + +static void orig_fill(struct objtool_file *file, + struct instruction *prev_insn, + struct instruction *insn) +{ + for (;;) { + if (!insn->visited) + insn->cfi = prev_insn->cfi; + + prev_insn = insn; + insn = next_insn_same_sec(file, insn); + if (!insn) + return; + if (!insn->alt_group) + return; + } +} + +int create_orc(struct objtool_file *file) +{ + struct instruction *insn, *prev_insn = NULL; for_each_insn(file, insn) { struct orc_entry *orc = &insn->orc; struct cfi_reg *cfa = &insn->cfi.cfa; struct cfi_reg *bp = &insn->cfi.regs[CFI_BP]; + int ret; orc->end = insn->cfi.end; + if (insn->alt_group && !insn->ignore_alts) { + struct alternative *alt; + + list_for_each_entry(alt, &insn->alts, list) { + if (alt->insn->offset == -1 /*FAKE_JUMP_OFFSET*/) + continue; + ret = alt_cfi(file, prev_insn, insn, alt->insn); + if (ret) + return ret; + } + + orig_fill(file, prev_insn, insn); + } + if (cfa->base == CFI_UNDEFINED) { orc->sp_reg = ORC_REG_UNDEFINED; continue; @@ -76,6 +188,8 @@ int create_orc(struct objtool_file *file) orc->sp_offset = cfa->offset; orc->bp_offset = bp->offset; orc->type = insn->cfi.type; + + prev_insn = insn; } return 0; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 14:16 ` Peter Zijlstra @ 2020-04-28 14:31 ` Josh Poimboeuf 2020-04-28 15:25 ` Peter Zijlstra 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-04-28 14:31 UTC (permalink / raw) To: Peter Zijlstra Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 04:16:14PM +0200, Peter Zijlstra wrote: > On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote: > > On Tue, Apr 28, 2020 at 02:04:50AM -0500, Josh Poimboeuf wrote: > > > > I'm thinking something like this should fix it. Peter, does this look > > > ok? > > > > Unfortunate. But also, I fear, insufficient. Specifically consider > > things like: > > > > ALTERNATIVE "jmp 1f", > > "alt... > > "..." > > "...insn", X86_FEAT_foo > > 1: > > > > This results in something like: > > > > > > .text .altinstr_replacement > > e8 xx ... > > 90 > > 90 > > ... > > 90 > > > > Where all our normal single byte nops (0x90) are unreachable with > > undefined CFI, but the alternative might have CFI, which is never > > propagated. > > > > We ran into this with the validate_alternative stuff from Alexandre. > > > So rather than hacking around this issue, should we not make > > create_orc() smarter? > > > > I'm trying to come up with something, but so far I'm just making a mess. > > Like this, it's horrid, but it seems to work. > > What do you think of the approach? I'll work on cleaning it up if you > don't hate it too much ;-) How'd you know I'd hate it ;-) That's quite the monstrosity, and I still don't see the point. I thought we decided to just disallow CFI changes in alternatives anyway? That can be done much simpler. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 14:31 ` Josh Poimboeuf @ 2020-04-28 15:25 ` Peter Zijlstra 2020-04-28 15:49 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: Peter Zijlstra @ 2020-04-28 15:25 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote: > That's quite the monstrosity, and I still don't see the point. I > thought we decided to just disallow CFI changes in alternatives anyway? > That can be done much simpler. Something like so then ? --- diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 8443ec690051..d14d83e6edb0 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file, last_new_insn = insn; + insn->alt_group = true; insn->ignore = orig_insn->ignore_alts; insn->func = orig_insn->func; @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state) list_for_each_entry(op, &insn->stack_ops, list) { int res; + if (insn->alt_group) { + WARN_FUNC("alternative has CFI", insn->sec, insn->offset); + return 1; + } + res = update_cfi_state(insn, &state->cfi, op); if (res) return res; @@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, sec = insn->sec; - if (insn->alt_group && list_empty(&insn->alts)) { - WARN_FUNC("don't know how to handle branch to middle of alternative instruction group", - sec, insn->offset); - return 1; - } - while (1) { next_insn = next_insn_same_sec(file, insn); @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, } } - if (skip_orig) + if (skip_orig) { + struct instruction *prev_insn = insn; + sec_for_each_insn_continue(file, insn) { + if (!insn->alt_group) + break; + if (!insn->visited) + insn->cfi = prev_insn->cfi; + } return 0; + } } if (handle_insn_ops(insn, &state)) ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 15:25 ` Peter Zijlstra @ 2020-04-28 15:49 ` Josh Poimboeuf 2020-04-28 15:54 ` Josh Poimboeuf 2020-04-28 16:44 ` Peter Zijlstra 0 siblings, 2 replies; 18+ messages in thread From: Josh Poimboeuf @ 2020-04-28 15:49 UTC (permalink / raw) To: Peter Zijlstra Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote: > On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote: > > That's quite the monstrosity, and I still don't see the point. I > > thought we decided to just disallow CFI changes in alternatives anyway? > > That can be done much simpler. > > Something like so then ? > > --- > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 8443ec690051..d14d83e6edb0 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file, > > last_new_insn = insn; > > + insn->alt_group = true; > insn->ignore = orig_insn->ignore_alts; > insn->func = orig_insn->func; > > @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state) > list_for_each_entry(op, &insn->stack_ops, list) { > int res; > > + if (insn->alt_group) { > + WARN_FUNC("alternative has CFI", insn->sec, insn->offset); > + return 1; > + } > + ACK (separate patch) > res = update_cfi_state(insn, &state->cfi, op); > if (res) > return res; > @@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > > sec = insn->sec; > > - if (insn->alt_group && list_empty(&insn->alts)) { > - WARN_FUNC("don't know how to handle branch to middle of alternative instruction group", > - sec, insn->offset); > - return 1; > - } > - ACK (separate patch) > while (1) { > next_insn = next_insn_same_sec(file, insn); > > @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > } > } > > - if (skip_orig) > + if (skip_orig) { > + struct instruction *prev_insn = insn; > + sec_for_each_insn_continue(file, insn) { > + if (!insn->alt_group) > + break; > + if (!insn->visited) > + insn->cfi = prev_insn->cfi; > + } > return 0; > + } NACK :-) What happens if you have two alternatives adjacent to each other (which can definitely happen in this scenario)? I still like my patch, at least the hack is done before the validate code, so validate_branch() itself is simpler. -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 15:49 ` Josh Poimboeuf @ 2020-04-28 15:54 ` Josh Poimboeuf 2020-04-28 16:25 ` Sean Christopherson 2020-04-28 16:44 ` Peter Zijlstra 1 sibling, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-04-28 15:54 UTC (permalink / raw) To: Peter Zijlstra Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote: > On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote: > > On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote: > > > That's quite the monstrosity, and I still don't see the point. I > > > thought we decided to just disallow CFI changes in alternatives anyway? > > > That can be done much simpler. > > > > Something like so then ? > > > > --- > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > > index 8443ec690051..d14d83e6edb0 100644 > > --- a/tools/objtool/check.c > > +++ b/tools/objtool/check.c > > @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file, > > > > last_new_insn = insn; > > > > + insn->alt_group = true; > > insn->ignore = orig_insn->ignore_alts; > > insn->func = orig_insn->func; > > > > @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state) > > list_for_each_entry(op, &insn->stack_ops, list) { > > int res; > > > > + if (insn->alt_group) { > > + WARN_FUNC("alternative has CFI", insn->sec, insn->offset); > > + return 1; > > + } > > + > > ACK (separate patch) BTW, since most people don't know what CFI is, how about something like "unsupported stack change in alternatives code" -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 15:54 ` Josh Poimboeuf @ 2020-04-28 16:25 ` Sean Christopherson 2020-04-28 16:33 ` Josh Poimboeuf 0 siblings, 1 reply; 18+ messages in thread From: Sean Christopherson @ 2020-04-28 16:25 UTC (permalink / raw) To: Josh Poimboeuf Cc: Peter Zijlstra, Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 10:54:13AM -0500, Josh Poimboeuf wrote: > On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote: > > On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote: > > > On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote: > > > > That's quite the monstrosity, and I still don't see the point. I > > > > thought we decided to just disallow CFI changes in alternatives anyway? > > > > That can be done much simpler. > > > > > > Something like so then ? > > > > > > --- > > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > > > index 8443ec690051..d14d83e6edb0 100644 > > > --- a/tools/objtool/check.c > > > +++ b/tools/objtool/check.c > > > @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file, > > > > > > last_new_insn = insn; > > > > > > + insn->alt_group = true; > > > insn->ignore = orig_insn->ignore_alts; > > > insn->func = orig_insn->func; > > > > > > @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state) > > > list_for_each_entry(op, &insn->stack_ops, list) { > > > int res; > > > > > > + if (insn->alt_group) { > > > + WARN_FUNC("alternative has CFI", insn->sec, insn->offset); > > > + return 1; > > > + } > > > + > > > > ACK (separate patch) > > BTW, since most people don't know what CFI is, how about something like > > "unsupported stack change in alternatives code" Would it be accurate to print "unsupported CFI stack change in alternatives code"? to give the developer something more explicit to plug into their search engine? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 16:25 ` Sean Christopherson @ 2020-04-28 16:33 ` Josh Poimboeuf 2020-04-28 18:28 ` Peter Zijlstra 0 siblings, 1 reply; 18+ messages in thread From: Josh Poimboeuf @ 2020-04-28 16:33 UTC (permalink / raw) To: Sean Christopherson Cc: Peter Zijlstra, Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 09:25:51AM -0700, Sean Christopherson wrote: > On Tue, Apr 28, 2020 at 10:54:13AM -0500, Josh Poimboeuf wrote: > > On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote: > > > On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote: > > > > On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote: > > > > > That's quite the monstrosity, and I still don't see the point. I > > > > > thought we decided to just disallow CFI changes in alternatives anyway? > > > > > That can be done much simpler. > > > > > > > > Something like so then ? > > > > > > > > --- > > > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > > > > index 8443ec690051..d14d83e6edb0 100644 > > > > --- a/tools/objtool/check.c > > > > +++ b/tools/objtool/check.c > > > > @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file, > > > > > > > > last_new_insn = insn; > > > > > > > > + insn->alt_group = true; > > > > insn->ignore = orig_insn->ignore_alts; > > > > insn->func = orig_insn->func; > > > > > > > > @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state) > > > > list_for_each_entry(op, &insn->stack_ops, list) { > > > > int res; > > > > > > > > + if (insn->alt_group) { > > > > + WARN_FUNC("alternative has CFI", insn->sec, insn->offset); > > > > + return 1; > > > > + } > > > > + > > > > > > ACK (separate patch) > > > > BTW, since most people don't know what CFI is, how about something like > > > > "unsupported stack change in alternatives code" > > Would it be accurate to print > > "unsupported CFI stack change in alternatives code"? > > to give the developer something more explicit to plug into their search > engine? I don't have a strong opinion either way, though this warning is going to be documented in stack-validation.txt anyway right Peter? :-) -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 16:33 ` Josh Poimboeuf @ 2020-04-28 18:28 ` Peter Zijlstra 0 siblings, 0 replies; 18+ messages in thread From: Peter Zijlstra @ 2020-04-28 18:28 UTC (permalink / raw) To: Josh Poimboeuf Cc: Sean Christopherson, Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 11:33:27AM -0500, Josh Poimboeuf wrote: > I don't have a strong opinion either way, though this warning is going > to be documented in stack-validation.txt anyway right Peter? :-) Sure... here goes (on top of Alexandre's alt_group patch). --- Subject: objtool: Fix ORC vs alternatives From: Peter Zijlstra <peterz@infradead.org> Date: Tue Apr 28 19:37:01 CEST 2020 Jann reported that (for instance) entry_64.o:general_protection has very odd ORC data: 0000000000000f40 <general_protection>: #######sp:sp+8 bp:(und) type:iret end:0 f40: 90 nop #######sp:(und) bp:(und) type:call end:0 f41: 90 nop f42: 90 nop #######sp:sp+8 bp:(und) type:iret end:0 f43: e8 a8 01 00 00 callq 10f0 <error_entry> #######sp:sp+0 bp:(und) type:regs end:0 f48: f6 84 24 88 00 00 00 testb $0x3,0x88(%rsp) f4f: 03 f50: 74 00 je f52 <general_protection+0x12> f52: 48 89 e7 mov %rsp,%rdi f55: 48 8b 74 24 78 mov 0x78(%rsp),%rsi f5a: 48 c7 44 24 78 ff ff movq $0xffffffffffffffff,0x78(%rsp) f61: ff ff f63: e8 00 00 00 00 callq f68 <general_protection+0x28> f68: e9 73 02 00 00 jmpq 11e0 <error_exit> #######sp:(und) bp:(und) type:call end:0 f6d: 0f 1f 00 nopl (%rax) Note the entry at 0xf41. Josh found this was the result of commit: 764eef4b109a ("objtool: Rewrite alt->skip_orig") Due to the early return in validate_branch() we no longer set insn->cfi of the original instruction stream (the NOPs at 0xf41 and 0xf42) and we'll end up with the above weirdness. In other discussions we realized alternatives should be ORC invariant; that is, due to there being only a single ORC table, it must be valid for all alternatives. The easiest way to ensure this is to not allow any stack modifications in alternatives. When we enforce this latter observation, we get the property that the whole alternative must have the same CFI, which we can employ to fix the former report. Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig") Reported-by: Jann Horn <jannh@google.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- tools/objtool/Documentation/stack-validation.txt | 7 ++++ tools/objtool/check.c | 34 ++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) --- a/tools/objtool/Documentation/stack-validation.txt +++ b/tools/objtool/Documentation/stack-validation.txt @@ -315,6 +315,13 @@ they mean, and suggestions for how to fi function tracing inserts additional calls, which is not obvious from the sources). +10. file.o: warning: func()+0x5c: alternative modifies stack + + This means that an alternative includes instructions that modify the + stack. The is that there is only one ORC unwind table, this means that the + ORC unwind entries must be valid for each of the alternatives. The easiest + way to enforce this is to ensure alternative do not contain any ORC + entries, which in turn implies the above constraint. If the error doesn't seem to make sense, it could be a bug in objtool. Feel free to ask the objtool maintainer for help. --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -2001,6 +2001,11 @@ static int handle_insn_ops(struct instru list_for_each_entry(op, &insn->stack_ops, list) { int res; + if (insn->alt_group) { + WARN_FUNC("alternative modifies stack", insn->sec, insn->offset); + return -1; + } + res = update_cfi_state(insn, &state->cfi, op); if (res) return res; @@ -2177,6 +2182,30 @@ static bool is_branch_to_alternative(str } /* + * Alternatives should not contain any ORC entries, this in turn means they + * should not contain any CFI ops, which implies all instructions should have + * the same same CFI state. + * + * It is possible to constuct alternatives that have unreachable holes that go + * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED + * states which then results in ORC entries, which we just said we didn't want. + * + * Avoid them by copying the CFI entry of the first instruction into the whole + * alternative. + */ +static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn) +{ + struct instruction *first_insn = insn; + int alt_group = insn->alt_group; + + sec_for_each_insn_continue(file, insn) { + if (insn->alt_group != alt_group) + break; + insn->cfi = first_insn->cfi; + } +} + +/* * Follow the branch starting at the given instruction, and recursively follow * any other branches (jumps). Meanwhile, track the frame pointer state at * each instruction and validate all the rules described in @@ -2234,7 +2263,7 @@ static int validate_branch(struct objtoo insn->visited |= visited; - if (!insn->ignore_alts) { + if (!insn->ignore_alts && !list_empty(&insn->alts)) { bool skip_orig = false; list_for_each_entry(alt, &insn->alts, list) { @@ -2249,6 +2278,9 @@ static int validate_branch(struct objtoo } } + if (insn->alt_group) + fill_alternative_cfi(file, insn); + if (skip_orig) return 0; } ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 15:49 ` Josh Poimboeuf 2020-04-28 15:54 ` Josh Poimboeuf @ 2020-04-28 16:44 ` Peter Zijlstra 2020-04-28 17:01 ` Josh Poimboeuf 1 sibling, 1 reply; 18+ messages in thread From: Peter Zijlstra @ 2020-04-28 16:44 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote: > > @@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > > > > sec = insn->sec; > > > > - if (insn->alt_group && list_empty(&insn->alts)) { > > - WARN_FUNC("don't know how to handle branch to middle of alternative instruction group", > > - sec, insn->offset); > > - return 1; > > - } > > - > > ACK (separate patch) > > > while (1) { > > next_insn = next_insn_same_sec(file, insn); > > Yeah, there is one from Julien that does this: 20200327152847.15294-6-jthierry@redhat.com > > @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > > } > > } > > > > - if (skip_orig) > > + if (skip_orig) { > > + struct instruction *prev_insn = insn; > > + sec_for_each_insn_continue(file, insn) { > > + if (!insn->alt_group) > > + break; > > + if (!insn->visited) > > + insn->cfi = prev_insn->cfi; > > + } > > return 0; > > + } > > NACK :-) > > What happens if you have two alternatives adjacent to each other (which > can definitely happen in this scenario)? Alexandre's alt_group would help: 20200414103618.12657-3-alexandre.chartre@oracle.com Then we can do something like: static void fill_alternative(struct instruction *insn) { struct instruction *first_insn = insn; int alt_group = insn->alt_group; sec_for_each_insn_continue(file, insn) { if (insn->alt_group != alt_group) break; if (!insn->visited) insn->cfi = first_insn->cfi; } } > I still like my patch, at least the hack is done before the validate > code, so validate_branch() itself is simpler. But it doesn't handle the case where the alternatives themselves have unreachable holes in them, if that happens we'll generate spurious ORC entries for them. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) 2020-04-28 16:44 ` Peter Zijlstra @ 2020-04-28 17:01 ` Josh Poimboeuf 0 siblings, 0 replies; 18+ messages in thread From: Josh Poimboeuf @ 2020-04-28 17:01 UTC (permalink / raw) To: Peter Zijlstra Cc: Jann Horn, Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers, kernel list, alexandre.chartre On Tue, Apr 28, 2020 at 06:44:44PM +0200, Peter Zijlstra wrote: > > > @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > > > } > > > } > > > > > > - if (skip_orig) > > > + if (skip_orig) { > > > + struct instruction *prev_insn = insn; > > > + sec_for_each_insn_continue(file, insn) { > > > + if (!insn->alt_group) > > > + break; > > > + if (!insn->visited) > > > + insn->cfi = prev_insn->cfi; > > > + } > > > return 0; > > > + } > > > > NACK :-) > > > > What happens if you have two alternatives adjacent to each other (which > > can definitely happen in this scenario)? > > Alexandre's alt_group would help: > > 20200414103618.12657-3-alexandre.chartre@oracle.com > > Then we can do something like: > > static void fill_alternative(struct instruction *insn) > { > struct instruction *first_insn = insn; > int alt_group = insn->alt_group; > > sec_for_each_insn_continue(file, insn) { > if (insn->alt_group != alt_group) > break; > if (!insn->visited) > insn->cfi = first_insn->cfi; > } > } Ugh... > > I still like my patch, at least the hack is done before the validate > > code, so validate_branch() itself is simpler. > > But it doesn't handle the case where the alternatives themselves have > unreachable holes in them, if that happens we'll generate spurious ORC > entries for them. Ah, I see what you mean. I need to think about it... -- Josh ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-04-28 18:29 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-01 6:02 x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?) Jann Horn 2020-03-02 14:58 ` Peter Zijlstra 2020-03-02 15:18 ` Josh Poimboeuf 2020-03-02 15:52 ` Josh Poimboeuf 2020-04-28 7:04 ` Josh Poimboeuf 2020-04-28 12:46 ` Peter Zijlstra 2020-04-28 14:14 ` Josh Poimboeuf 2020-04-28 14:35 ` Peter Zijlstra 2020-04-28 14:16 ` Peter Zijlstra 2020-04-28 14:31 ` Josh Poimboeuf 2020-04-28 15:25 ` Peter Zijlstra 2020-04-28 15:49 ` Josh Poimboeuf 2020-04-28 15:54 ` Josh Poimboeuf 2020-04-28 16:25 ` Sean Christopherson 2020-04-28 16:33 ` Josh Poimboeuf 2020-04-28 18:28 ` Peter Zijlstra 2020-04-28 16:44 ` Peter Zijlstra 2020-04-28 17:01 ` Josh Poimboeuf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).