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: <20230907110917.GA10955@noisy.programming.kicks-ass.net>
Date:   Thu, 7 Sep 2023 13:09:17 +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, nik.borisov@...e.com
Subject: Re: [PATCH v2 10/11] x86/alternatives: Simplify ALTERNATIVE_n()

On Thu, Sep 07, 2023 at 10:31:58AM +0200, Borislav Petkov wrote:
> 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

665f-664f = 5 (rep)
662b-661b = 5 (orig)

5-5 > 0 = 0

so no padding

> > 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.

665f-664f = 5 (erms)
662b-661b = 5 (orig + padding)

5-5 > 0 = 0

no padding, also since, as you note 661b is the first, we include all
previous padding, and the skip will only add additional padding if the
new sequence is longer still.

So, no I'm not seeing it. Doubly not with this example where all 3
variants are 5 bytes.

Notably, the following nonsense alternative with 1 2 and 3 bytes
instructions:

	asm volatile (
		ALTERNATIVE_2("push %rbp",
			"push %r12", X86_FEATURE_ALWAYS,
			"mov %rsp,%rbp", X86_FEATURE_ALWAYS));

ends up as:

0004  204:      55                      push   %rbp
0005  205:      90                      nop
0006  206:      90                      nop

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 ;-)

Anyway, the 1,3,2 variant spelled out reads like:

#APP
# 1563 "../arch/x86/kernel/alternative.c" 1
# ALT: oldnstr
661:
# ALT: oldnstr
661:
push %rbp
662:
# ALT: padding
.skip -(((665f-664f)-(662b-661b)) > 0) * ((665f-664f)-(662b-661b)),0x90

 #   Which evaluates like:
 #     665f-664f = 3
 #     662b-661b = 1
 #     3-1 > 0 = -1
 #     --1 * (3-1) = 2
 #
 #   so two single byte nops get emitted 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:
mov %rsp,%rbp
665:
.popsection

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

 #   And this evaluates to:
 #     665f-664f = 2
 #     662b-661b = 3 (because it includes the original 1 byte instruction and 2 bytes padding)
 #     3-1 > 0 = 0
 #     0 * (3-1) = 0
 #
 #   so no extra padding

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:
push %r12
665:
.popsection

# 0 "" 2
# ../arch/x86/kernel/alternative.c:1569:        int3_selftest();
#NO_APP