linux-kernel - Re: [PATCH v2 10/11] x86/alternatives: Simplify ALTERNATIVE

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: <20230907150632.GAZPnm+Ly+Vyt8VPYr@fat_crate.local>
Date:   Thu, 7 Sep 2023 17:06:32 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, David.Kaplan@....com,
        Andrew.Cooper3@...rix.com, jpoimboe@...nel.org,
        gregkh@...uxfoundation.org, nik.borisov@...e.com
Subject: Re: [PATCH v2 10/11] x86/alternatives: Simplify ALTERNATIVE_n()

On Thu, Sep 07, 2023 at 01:09:17PM +0200, Peter Zijlstra wrote:
> If you flip the 3 and 2 byte instructions the result is the same. No
> extra padding.
>
> And no, I had not actually tested this before, because clearly this is
> all obvious ;-)

IKR.

So I take that extra padding thing back - we actually *must* have that
padding so that it actually works correctly. I just did a silly example
but nothing says one cannot do one like that today:

		alternative_2("", "pop %%rax", X86_FEATURE_ALWAYS,
			      "call clear_page_orig", X86_FEATURE_ALWAYS);

An order of insns which grows in size: 0, then 1, then 5.

It turns into:

> # arch/x86/mm/init.c:163: 		alternative_2("", "pop %%rax", X86_FEATURE_ALWAYS,
> # 163 "arch/x86/mm/init.c" 1
> 	# ALT: oldnstr
> 661:
> 	# ALT: oldnstr
> 661:
> 	
> 662:
> # ALT: padding
> .skip -(((665f-664f)-(662b-661b)) > 0) * ((665f-664f)-(662b-661b)),0x90

IINM, this turns into:

.skip 1 * (1 - 0) = 1.

because "pop %rax" is one byte. The original insn is of size 0.

So we end up with a 0x90 here.

> 663:
> .pushsection .altinstructions,"a"
>  .long 661b - .
>  .long 664f - .
>  .4byte ( 3*32+21)
>  .byte 663b-661b
>  .byte 665f-664f
> .popsection
> .pushsection .altinstr_replacement, "ax"
> # ALT: replacement 
> 664:
> 	pop %rax
>  665:
> .popsection
> 
> 662:

<--- X

> # ALT: padding
> .skip -(((665f-664f)-(662b-661b)) > 0) * ((665f-664f)-(662b-661b)),0x90

Now the second guy comes in. That turns into:

.skip 1 * (5 - 1) = 4

Because, IINM, the 662 label above is the *second* one at marker X (we
go backwards) and the 661 is the second one too.

So between those two labels you have the 0x90 - one byte padding from
the first .skip.

And now it adds 4 more bytes to accomodate the CALL.

So we need to have that padding back-to-back in case the second
replacement is longer.

Ok, I guess the only thing that's bothering me is:

>       # ALT: oldnstr
> 661:
>       # ALT: oldnstr 
> 661:

I'll keep on playing with this.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette