linux-kernel - Re: [PATCH v10 03/38] x86/msr: Add the WRMSRNS instruction support

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: <0e7d37db-e1af-ac40-6eca-5565d1bebcde@zytor.com>
Date:   Thu, 14 Sep 2023 18:01:01 -0700
From:   "H. Peter Anvin" <hpa@...or.com>
To:     andrew.cooper3@...rix.com, Thomas Gleixner <tglx@...utronix.de>,
        Xin Li <xin3.li@...el.com>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-edac@...r.kernel.org,
        linux-hyperv@...r.kernel.org, kvm@...r.kernel.org,
        xen-devel@...ts.xenproject.org
Cc:     mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        x86@...nel.org, luto@...nel.org, pbonzini@...hat.com,
        seanjc@...gle.com, peterz@...radead.org, jgross@...e.com,
        ravi.v.shankar@...el.com, mhiramat@...nel.org,
        jiangshanlai@...il.com
Subject: Re: [PATCH v10 03/38] x86/msr: Add the WRMSRNS instruction support

> WRMSR has one complexity that most other PV-ops don't, and that's the
> exception table reference for the instruction itself.
> 
> In a theoretical future ought to look like:
> 
>     mov    $msr, %ecx
>     mov    $lo, %eax
>     mov    $hi, %edx
>     1: {call paravirt_blah(%rip) | cs...cs wrmsr | cs...cs wrmsrns }
>     _ASM_EXTABLE(1b, ...)
> 
> In paravirt builds, the CALL needs to be the emitted form, because it
> needs to function in very early boot.
> 
> But once the paravirt-ness has been chosen and alternatives run, the
> as-native paths are fully inline.
> 
> The alternative which processes this site wants to conclude that, in the
> case it does not alter from the CALL, to clobber the EXTABLE reference. 
> CALL instructions can #GP, and you don't want to end up thinking you're
> handling a WRMSR #GP when in fact it was a non-canonical function pointer.


On 9/14/23 17:36, andrew.cooper3@...rix.com wrote:> On 15/09/2023 1:07 
am, H. Peter Anvin wrote:
 >> Is *that* your concern?! Just put a NOP before WRMSR – you need 
padding NOP bytes anyway – and the extable entry is no longer at the 
same address. Problem solved.
 >>
 >> Either that, or use a direct call, which can't #GP in the address 
range used by the kernel.
 >
 > For non-paravirt builds, I really hope the inlining DoesTheRightThing.
 > If it doesn't lets fix it to do so.
 >
 > For paravirt builds, the emitted form must be the indirect call so it
 > can function in boot prior to alternatives running [1].
 >
No, it doesn't. You always have the option of a direct call to an 
indirect JMP. This is in fact exactly what userspace does in the form of 
the PLT.

 > So you still need some way of putting the EXTABLE reference at the
 > emitted site, not in the .altintr_replacement section where the
 > WRMSR{,NS} instruction lives.  This needs to be at build time because
 > the EXTABLE references aren't shuffled at runtime.
 >
 > How else do you propose getting an extable reference to midway through
 > an instruction on the "wrong" part of an alternative?
Well, obviously there has to be a magic inline at the patch site. It 
ends up looking something like this:

	asm volatile("1:"
		     ALTERNATIVE_2("call pv_wrmsr(%%rip)",
			"nop; wrmsr", X86_FEATURE_NATIVE_MSR,
			"nop; wrmsrns", X86_FEATURE_WRMSRNS)
		     "2:"
		     _ASM_EXTABLE_TYPE(1b+1, 2b, EX_TYPE_WRMSR)
		     : : "c" (msr), "a" (low), "d" (high) : "memory");


[one can argue whether or not WRMSRNS specifically should require 
"memory" or not.]

The whole bit with alternatives and pvops being separate is a major 
maintainability problem, and honestly it never made any sense in the 
first place. Never have two mechanisms to do one job; it makes it harder 
to grok their interactions.

As an alternative to the NOP, the EX_TYPE_*MSR* handlers could simply 
look for a CALL opcode at the origin.

	-hpa