| 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
| ||
|
Message-ID: <20251030061741.xorsg5ags4fos7u3@desk>
Date: Wed, 29 Oct 2025 23:17:41 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: Yao Yuan <yaoyuan@...ux.alibaba.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>,
Sean Christopherson <seanjc@...gle.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 Thu, Oct 30, 2025 at 01:52:39PM +0800, Yao Yuan wrote:
> On Wed, Oct 29, 2025 at 05:33:46PM +0800, Pawan Gupta wrote:
> > On Wed, Oct 29, 2025 at 02:27:00PM -0700, 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.
> > >
> > > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> > > ---
> > > arch/x86/kvm/vmx/run_flags.h | 12 ++++++------
> > > arch/x86/kvm/vmx/vmenter.S | 5 +++++
> > > arch/x86/kvm/vmx/vmx.c | 26 ++++++++++----------------
> > > 3 files changed, 21 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx/run_flags.h b/arch/x86/kvm/vmx/run_flags.h
> > > index 2f20fb170def8b10c8c0c46f7ba751f845c19e2c..004fe1ca89f05524bf3986540056de2caf0abbad 100644
> > > --- a/arch/x86/kvm/vmx/run_flags.h
> > > +++ b/arch/x86/kvm/vmx/run_flags.h
> > > @@ -2,12 +2,12 @@
> > > #ifndef __KVM_X86_VMX_RUN_FLAGS_H
> > > #define __KVM_X86_VMX_RUN_FLAGS_H
> > >
> > > -#define VMX_RUN_VMRESUME_SHIFT 0
> > > -#define VMX_RUN_SAVE_SPEC_CTRL_SHIFT 1
> > > -#define VMX_RUN_CLEAR_CPU_BUFFERS_FOR_MMIO_SHIFT 2
> > > +#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_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_FOR_MMIO BIT(VMX_RUN_CLEAR_CPU_BUFFERS_FOR_MMIO_SHIFT)
> > > +#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)
> > >
> > > #endif /* __KVM_X86_VMX_RUN_FLAGS_H */
> > > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > > index 0dd23beae207795484150698d1674dc4044cc520..ec91f4267eca319ffa8e6079887e8dfecc7f96d8 100644
> > > --- a/arch/x86/kvm/vmx/vmenter.S
> > > +++ b/arch/x86/kvm/vmx/vmenter.S
> > > @@ -137,6 +137,9 @@ 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 vmlaunch or vmresume is needed */
> > > bt $VMX_RUN_VMRESUME_SHIFT, %ebx
> > >
> > > @@ -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
> > > /* Clobbers EFLAGS.ZF */
> > > VM_CLEAR_CPU_BUFFERS
> > > .Lskip_clear_cpu_buffers:
> > > 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;
> > > + }
> >
> > 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;
> > }
> >
>
> Oh, even no need a or two year later, I just feel confusion
> when look at this part first time. But this change anyway
> makes it more clear to me.
:-) I felt the same after sending the patch.
Powered by blists - more mailing lists