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: <20251030061012.hkt2wsbxo5vh55ub@desk>
Date: Wed, 29 Oct 2025 23:11:50 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: 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>,
	Brendan Jackman <jackmanb@...gle.com>
Subject: Re: [PATCH 3/3] x86/mmio: Unify VERW mitigation for guests

On Wed, Oct 29, 2025 at 05:27:37PM -0700, Sean Christopherson wrote:
> On Wed, Oct 29, 2025, Pawan Gupta wrote:
> > When a system is only affected by MMIO Stale Data, VERW mitigation is
> > currently handled differently than other data sampling attacks like
> > MDS/TAA/RFDS, that do the VERW in asm. This is because for MMIO Stale Data,
> > VERW is needed only when the guest can access host MMIO, this was tricky to
> > check in asm.
> > 
> > Refactoring done by:
> > 
> >   83ebe7157483 ("KVM: VMX: Apply MMIO Stale Data mitigation if KVM maps
> >   MMIO into the guest")
> > 
> > now makes it easier to execute VERW conditionally in asm based on
> > VMX_RUN_CLEAR_CPU_BUFFERS_FOR_MMIO.
> > 
> > Unify MMIO Stale Data mitigation with other VERW-based mitigations and only
> > have single VERW callsite in __vmx_vcpu_run(). Remove the now unnecessary
> > call to x86_clear_cpu_buffer() in vmx_vcpu_enter_exit().
> > 
> > This also untangles L1D Flush and MMIO Stale Data mitigation. Earlier, an
> > L1D Flush would skip the VERW for MMIO Stale Data. Now, both the
> > mitigations are independent of each other. Although, this has little
> > practical implication since there are no CPUs that are affected by L1TF and
> > are *only* affected by MMIO Stale Data (i.e. not affected by MDS/TAA/RFDS).
> > But, this makes the code cleaner and easier to maintain.
> 
> Heh, and largely makes our discussion on the L1TF cleanup moot :-)

Well, this series is largely a result of that discussion :-)

> > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> > ---
> 
> ...
> 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 451be757b3d1b2fec6b2b79157f26dd43bc368b8..303935882a9f8d1d8f81a499cdce1fdc8dad62f0 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -903,9 +903,16 @@ unsigned int __vmx_vcpu_run_flags(struct vcpu_vmx *vmx)
> >  	if (!msr_write_intercepted(vmx, MSR_IA32_SPEC_CTRL))
> >  		flags |= VMX_RUN_SAVE_SPEC_CTRL;
> >  
> > -	if (static_branch_unlikely(&cpu_buf_vm_clear_mmio_only) &&
> > -	    kvm_vcpu_can_access_host_mmio(&vmx->vcpu))
> > -		flags |= VMX_RUN_CLEAR_CPU_BUFFERS_FOR_MMIO;
> > +	/*
> > +	 * When affected by MMIO Stale Data only (and not other data sampling
> > +	 * attacks) only clear for MMIO-capable guests.
> > +	 */
> > +	if (static_branch_unlikely(&cpu_buf_vm_clear_mmio_only)) {
> > +		if (kvm_vcpu_can_access_host_mmio(&vmx->vcpu))
> > +			flags |= VMX_RUN_CLEAR_CPU_BUFFERS;
> > +	} else {
> > +		flags |= VMX_RUN_CLEAR_CPU_BUFFERS;
> > +	}
> 
> This is quire confusing and subtle.

I realized that and sent the below follow-up almost at the same time:

	Setting the flag here is harmless but not necessary when the CPU is not
	affected by any of the data sampling attacks. VM_CLEAR_CPU_BUFFERS would be
	a NOP in the case.

	However, me looking at this code in a year or two would be confused why the
	flag is always set on unaffected CPUs. Below change to conditionally set
	the flag would make it clearer.

	---
	diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
	index 303935882a9f..0eab59ab2698 100644
	--- a/arch/x86/kvm/vmx/vmx.c
	+++ b/arch/x86/kvm/vmx/vmx.c
	@@ -910,7 +910,7 @@ unsigned int __vmx_vcpu_run_flags(struct vcpu_vmx *vmx)
		if (static_branch_unlikely(&cpu_buf_vm_clear_mmio_only)) {
			if (kvm_vcpu_can_access_host_mmio(&vmx->vcpu))
				flags |= VMX_RUN_CLEAR_CPU_BUFFERS;
	-	} else {
	+	} else if (cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF_VM)) {
			flags |= VMX_RUN_CLEAR_CPU_BUFFERS;
		}


> E.g. it requires the reader to know that cpu_buf_vm_clear_mmio_only is
> mutually exlusive with X86_FEATURE_CLEAR_CPU_BUF, and that
> VMX_RUN_CLEAR_CPU_BUFFERS is ignored if X86_FEATURE_CLEAR_CPU_BUF=n.
> 
> At least, I think that's how it works :-)

That is right, only thing is instead of X86_FEATURE_CLEAR_CPU_BUF,
VM_CLEAR_CPU_BUFFERS depends on KVM specific X86_FEATURE_CLEAR_CPU_BUF_VM.

> Isn't the above equivalent to this when all is said and done?
> 
> 	if (cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF) ||

For the above reason, it might be better to to use
X86_FEATURE_CLEAR_CPU_BUF_VM (as in the diff I pasted above).

> 	    (static_branch_unlikely(&cpu_buf_vm_clear_mmio_only) &&
> 	     kvm_vcpu_can_access_host_mmio(&vmx->vcpu)))
> 		flags |= VMX_RUN_CLEAR_CPU_BUFFERS;
>