Re: [PATCH v2 02/14] objtool: Fix ORC vs alternatives

[Josh Poimboeuf]

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 <jannh@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  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 <jpoimboe@xxxxxxxxxx>

-- 
Josh