linux-kernel - Re: [PATCH 3/3] x86/mmio: Unify VERW mitigation for guests

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: <DDVSPNXCG4HY.1B7OBAPDZ97CX@google.com>
Date: Thu, 30 Oct 2025 16:26:10 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>, Brendan Jackman <jackmanb@...gle.com>
Cc: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, 
	Peter Zijlstra <peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>, 
	Ingo Molnar <mingo@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>, 
	"H. Peter Anvin" <hpa@...or.com>, Paolo Bonzini <pbonzini@...hat.com>, <linux-kernel@...r.kernel.org>, 
	<kvm@...r.kernel.org>, Tao Zhang <tao1.zhang@...el.com>, 
	Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH 3/3] x86/mmio: Unify VERW mitigation for guests

On Thu Oct 30, 2025 at 4:06 PM UTC, Sean Christopherson wrote:
> On Thu, Oct 30, 2025, Brendan Jackman wrote:
>> > @@ -160,6 +163,8 @@ SYM_FUNC_START(__vmx_vcpu_run)
>> >  	/* Load guest RAX.  This kills the @regs pointer! */
>> >  	mov VCPU_RAX(%_ASM_AX), %_ASM_AX
>> >  
>> > +	/* Check EFLAGS.ZF from the VMX_RUN_CLEAR_CPU_BUFFERS bit test above */
>> > +	jz .Lskip_clear_cpu_buffers
>> 
>> Hm, it's a bit weird that we have the "alternative" inside
>> VM_CLEAR_CPU_BUFFERS, but then we still keep the test+jz
>> unconditionally. 
>
> Yeah, I had the same reaction, but couldn't come up with a clean-ish solution
> and so ignored it :-)
>
>> If we really want to super-optimise the no-mitigations-needed case,
>> shouldn't we want to avoid the conditional in the asm if it never
>> actually leads to a flush?
>> 
>> On the other hand, if we don't mind a couple of extra instructions,
>> shouldn't we be fine with just having the whole asm code based solely
>> on VMX_RUN_CLEAR_CPU_BUFFERS and leaving the
>> X86_FEATURE_CLEAR_CPU_BUF_VM to the C code?
>> 
>> I guess the issue is that in the latter case we'd be back to having
>> unnecessary inconsistency with AMD code while in the former case... well
>> that would just be really annoying asm code - am I on the right
>> wavelength there? So I'm not necessarily asking for changes here, just
>> probing in case it prompts any interesting insights on your side.
>> 
>> (Also, maybe this test+jz has a similar cost to the nops that the
>> "alternative" would inject anyway...?)
>
> It's not at all expensive.  My bigger objection is that it's hard to follow what's
> happening.
>
> Aha!  Idea.  IIUC, only the MMIO Stale Data is conditional based on the properties
> of the vCPU, so we should track _that_ in a KVM_RUN flag.  And then if we add yet
> another X86_FEATURE for MMIO Stale Data flushing (instead of a static branch),
> this path can use ALTERNATIVE_2.  The use of ALTERNATIVE_2 isn't exactly pretty,
> but IMO this is much more intuitive.
>
> diff --git a/arch/x86/kvm/vmx/run_flags.h b/arch/x86/kvm/vmx/run_flags.h
> index 004fe1ca89f0..b9651960e069 100644
> --- a/arch/x86/kvm/vmx/run_flags.h
> +++ b/arch/x86/kvm/vmx/run_flags.h
> @@ -4,10 +4,10 @@
>  
>  #define VMX_RUN_VMRESUME_SHIFT                 0
>  #define VMX_RUN_SAVE_SPEC_CTRL_SHIFT           1
> -#define VMX_RUN_CLEAR_CPU_BUFFERS_SHIFT                2
> +#define VMX_RUN_CAN_ACCESS_HOST_MMIO_SHIT      2
>  
>  #define VMX_RUN_VMRESUME               BIT(VMX_RUN_VMRESUME_SHIFT)
>  #define VMX_RUN_SAVE_SPEC_CTRL         BIT(VMX_RUN_SAVE_SPEC_CTRL_SHIFT)
> -#define VMX_RUN_CLEAR_CPU_BUFFERS      BIT(VMX_RUN_CLEAR_CPU_BUFFERS_SHIFT)
> +#define VMX_RUN_CAN_ACCESS_HOST_MMIO   BIT(VMX_RUN_CAN_ACCESS_HOST_MMIO_SHIT)
>  
>  #endif /* __KVM_X86_VMX_RUN_FLAGS_H */
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index ec91f4267eca..50a748b489b4 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -137,8 +137,10 @@ SYM_FUNC_START(__vmx_vcpu_run)
>         /* Load @regs to RAX. */
>         mov (%_ASM_SP), %_ASM_AX
>  
> -       /* jz .Lskip_clear_cpu_buffers below relies on this */
> -       test $VMX_RUN_CLEAR_CPU_BUFFERS, %ebx
> +       /* Check if jz .Lskip_clear_cpu_buffers below relies on this */
> +       ALTERNATIVE_2 "",
> +                     "", X86_FEATURE_CLEAR_CPU_BUF
> +                     "test $VMX_RUN_CAN_ACCESS_HOST_MMIO, %ebx", X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO

Er, I don't understand the ALTERNATIVE_2 here, don't we just need this?

ALTERNATIVE "" "test $VMX_RUN_CAN_ACCESS_HOST_MMIO, %ebx", 
	    X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO

>  
>         /* Check if vmlaunch or vmresume is needed */
>         bt   $VMX_RUN_VMRESUME_SHIFT, %ebx
> @@ -163,8 +165,9 @@ SYM_FUNC_START(__vmx_vcpu_run)
>         /* Load guest RAX.  This kills the @regs pointer! */
>         mov VCPU_RAX(%_ASM_AX), %_ASM_AX
>  
> -       /* Check EFLAGS.ZF from the VMX_RUN_CLEAR_CPU_BUFFERS bit test above */
> -       jz .Lskip_clear_cpu_buffers
> +       ALTERNATIVE_2 "jmp .Lskip_clear_cpu_buffers",
> +                     "", X86_FEATURE_CLEAR_CPU_BUF
> +                     "jz .Lskip_clear_cpu_buffers", X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO

To fit with the rest of Pawan's code this would need
s/X86_FEATURE_CLEAR_CPU_BUF/X86_FEATURE_CLEAR_CPU_BUF_VM/, right?

In case it reveals that I just don't understand ALTERNATIVE_2 at all,
I'm reading this second one as saying:

if cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO)
   "jz .Lskip_clear_cpu_buffers "
else if !cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF_VM)
   "jmp .Lskip_clear_cpu_buffers"

I.e. I'm understanding X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO as mutually
exclusive with X86_FEATURE_CLEAR_CPU_BUF_VM, it means "you _only_ need
to verw MMIO". So basically we moved cpu_buf_vm_clear_mmio_only into a
CPU feature to make it accessible from asm?

(Also let's use BUF instead of BUFFERS in the name, for consistency)