Re: [RFC][PATCH 04/17] objtool/x86: Fix SRSO mess

[Peter Zijlstra]

On Thu, Aug 10, 2023 at 02:48:59PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 10, 2023 at 02:06:15PM +0200, Borislav Petkov wrote:
> > On Wed, Aug 09, 2023 at 09:12:22AM +0200, Peter Zijlstra wrote:
> > > arch_is_rethunk() indicates those functions that will be considered
> > > equivalent to INSN_RETURN and any callsite will be added to the
> > > .return_sites section.
> > > 
> > > Notably: srso_untrain_ret, srso_safe_ret and __ret do not qualify.
> > 
> > They do not qualify *anymore*. Before your changes, the last two would
> > call RET.

Notably 'call RET' is not what this is about. I hope the below
clarifies.

> They were wrong too before too. From the previous email (slightly
> refined), you had:
> 
> SYNC_FUNC_START(foo)
> 	...
> 	ALTERNATIVE "jmp __x86_return_thunk",
> 		    "ret; int3" ALT_NOT(X86_FEATURE_RETHUNK)
> SYM_FUNC_END(foo)
> 
> SYM_FUNC_START(__x86_return_thunk)
> 	ALTERNATIVE "jmp __ret",
> 		    "call srso_safe_ret", X86_FEATURE_SRSO,
> 		    "call srso_alias_safe_ret", X86_FEATURE_SRSO_ALIAS
> 	int3
> SYM_FUNC_END(__x86_return_thunk)
> 
> 
> so we want foo's "jmp __x86_return_thunk" to end up in .return_sites.
> 
> But what you had would also add __x86_return_thunk's 'jmp __ret' to
> .return_sites. And .altins_replacement's 'call srso_safe_ret' in
> .return_sites if we're unlucky.
> 
> You do *NOT* want those in .return_sites.
> 
> 
> objtool did two things:
> 
>  - generate .return_sites to rewrite all the compiler generated 'jmp
>    __x86_return_thunk' sites to either 'ret; int3' or a jump to your
>    function of choice (default __x86_return_thunk).
> 
>  - not get confused by the fact that __x86_return_thunk is in the middle
>    of another instruction and zen_untrain_ret's validation fails to find
>    the jump target because it doesn't know this instruction -- because
>    it's inside another instruction.
> 
> Before all this they were both __x86_return_thunk, there was no
> distinction needed.
> 
> But now there is, you still only want 'jmp __x86_return_thunk' to end up
> in .return_sites. As per the above, you very much do *NOT* want any
> other things on there.
> 
> But you added one more magic function inside another instruction
> instance and had it identified at return_thunk, which is a fail.
> 
> Worse, you also added srso_untrain_ret, even though that really
> shouldn't have been added since it doesn't suffer any of the above two
> things.
> 
>