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: <20230810125005.GC220434@hirez.programming.kicks-ass.net>
Date:   Thu, 10 Aug 2023 14:50:05 +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: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.
> 
>