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: <20230907083158.GBZPmKfjarnaQk1ofB@fat_crate.local>
Date:   Thu, 7 Sep 2023 10:31:58 +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 Mon, Aug 14, 2023 at 01:44:36PM +0200, Peter Zijlstra wrote:
> Instead of making increasingly complicated ALTERNATIVE_n()
> implementations, use a nested alternative expression.
> 
> The only difference between:
> 
>   ALTERNATIVE_2(oldinst, newinst1, flag1, newinst2, flag2)
> 
> and
> 
>   ALTERNATIVE(ALTERNATIVE(oldinst, newinst1, flag1),
>               newinst2, flag2)

Hmm, one more problem I see with this. You're handling it, it seems, but
the whole thing doesn't feel clean to me.

Here's an exemplary eval:

> #APP
> # 53 "./arch/x86/include/asm/page_64.h" 1
> 	# ALT: oldnstr
> 661:
> 	# ALT: oldnstr
> 661:

<--- X

> 	call clear_page_orig	#
> 662:
> # ALT: padding
> .skip -(((665f-664f)-(662b-661b)) > 0) * ((665f-664f)-(662b-661b)),0x90
> 663:
> .pushsection .altinstructions,"a"
>  .long 661b - .
>  .long 664f - .
>  .4byte ( 3*32+16)
>  .byte 663b-661b
>  .byte 665f-664f
> .popsection
> .pushsection .altinstr_replacement, "ax"
> # ALT: replacement 
> 664:
> 	call clear_page_rep	#
>  665:
> .popsection
> 
> 662:
> # ALT: padding
> .skip -(((665f-664f)-(662b-661b)) > 0) * ((665f-664f)-(662b-661b)),0x90
> 663:

<--- Z

So here it would add the padding again, unnecessarily.

> .pushsection .altinstructions,"a"
>  .long 661b - .

This refers to the 661 label, if you count backwards it would be the
second 661 label at my marker X above.

>  .long 664f - .

This is the 664 label at my marker Y below.

>  .4byte ( 9*32+ 9)
>  .byte 663b-661b

And here's where it gets interesting. That's source length. 663
backwards label is at marker Z which includes the second padding.

So if we do a lot of padding, that might grow vmlinux. Not a big deal
but still... Have you measured how much allyesconfig builds grow with
this patch?

>  .byte 665f-664f
> .popsection
> .pushsection .altinstr_replacement, "ax"
> # ALT: replacement 
> 664:

<--- Y

> 	call clear_page_erms	#
>  665:
> .popsection

In any case, I'd still like to solve this in a clean way, without the
fixup and unnecessary padding addition.

Lemme play some more with the preprocessor...

-- 
Regards/Gruss,
    Boris.

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