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

[Peter Zijlstra <peterz@infradead.org>]

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.

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.