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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230810124859.GZ212435@hirez.programming.kicks-ass.net>
Date:   Thu, 10 Aug 2023 14:48:59 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, David.Kaplan@....com,
        Andrew.Cooper3@...rix.com, jpoimboe@...nel.org,
        gregkh@...uxfoundation.org
Subject: Re: [RFC][PATCH 04/17] objtool/x86: Fix SRSO mess

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.