On Tue, Apr 28, 2020 at 09:11:03PM +0200, Peter Zijlstra wrote:
> 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 <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> 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 problem 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
"alternatives"
Acked-by: Josh Poimboeuf <[email protected]>
--
Josh
