linux-kernel - Re: [PATCH] KVM: VMX: Flush shadow VMCS on emergency reboot

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: <Z_lKE-GjP3WQrdkR@google.com>
Date: Fri, 11 Apr 2025 09:57:55 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] KVM: VMX: Flush shadow VMCS on emergency reboot

On Fri, Apr 11, 2025, Chao Gao wrote:
> On Thu, Apr 10, 2025 at 02:55:29PM -0700, Sean Christopherson wrote:
> >On Mon, Mar 24, 2025, Chao Gao wrote:
> >> Ensure the shadow VMCS cache is evicted during an emergency reboot to
> >> prevent potential memory corruption if the cache is evicted after reboot.
> >
> >I don't suppose Intel would want to go on record and state what CPUs would actually
> >be affected by this bug.  My understanding is that Intel has never shipped a CPU
> >that caches shadow VMCS state.
> 
> I am not sure. Would you like me to check internally?

Eh, if it's easy, it'd be nice to have, but don't put much effort into it.  I'm
probably being too cute in hesitating about sending this to stable@.  The risk
really shouldn't be high.

> However, SDM Chapter 26.11 includes a footnote stating:
> "
> As noted in Section 26.1, execution of the VMPTRLD instruction makes a VMCS is
> active. In addition, VM entry makes active any shadow VMCS referenced by the
> VMCS link pointer in the current VMCS. If a shadow VMCS is made active by VM
> entry, it is necessary to execute VMCLEAR for that VMCS before allowing that
> VMCS to become active on another logical processor.
> "
> 
> To me, this suggests that shadow VMCS may be cached, and software shouldn't
> assume the CPU won't cache it. But, I don't know if this is the reality or
> if the statement is merely for hardware implementation flexibility.
> 
> >
> >On a very related topic, doesn't SPR+ now flush the VMCS caches on VMXOFF?  If
> 
> Actually this behavior is not publicly documented.

Well shoot.  That should probably be remedied.  Even if the behavior is guaranteed
only on CPUs that support SEAM, _that_ detail should be documented.  I'm not
holding my breath on Intel allowing third party code in SEAM, but the mode _is_
documented in the SDM, and so IMO, the SDM should also document how things like
clearing the VMCS cache are supposed to work when there are VMCSes that "untrusted"
software may not be able to access.

> >that's going to be the architectural behavior going forward, will that behavior
> >be enumerated to software?  Regardless of whether there's software enumeration,
> >I would like to have the emergency disable path depend on that behavior.  In part
> >to gain confidence that SEAM VMCSes won't screw over kdump, but also in light of
> >this bug.
> 
> I don't understand how we can gain confidence that SEAM VMCSes won't screw
> over kdump.

If KVM relies on VMXOFF to purge the VMCS cache, then it gives a measure of
confidence that running TDX VMs won't leave behind SEAM VMCSes in the cache.  KVM
can't easily clear SEAM VMCSs, but IIRC, the memory can be "forcefully" reclaimed
by paving over it with MOVDIR64B, at which point having VMCS cache entries for
the memory would be problematic.

> If a VMM wants to leverage the VMXOFF behavior, software enumeration
> might be needed for nested virtualization. Using CPU F/M/S (SPR+) to
> enumerate a behavior could be problematic for virtualization. Right?

Yeah, F/M/S is a bad idea.  Architecturally, I think the behavior needs to be
tied to support for SEAM.  Is there a safe-ish way to probe for SEAM support,
without having to glean it from MSR_IA32_MKTME_KEYID_PARTITIONING?

> >If all past CPUs never cache shadow VMCS state, and all future CPUs flush the
> >caches on VMXOFF, then this is a glorified NOP, and thus probably shouldn't be
> >tagged for stable.
> 
> Agreed.
> 
> Sean, I am not clear whether you intend to fix this issue and, if so, how.
> Could you clarify?

Oh, I definitely plan on taking this patch, I'm just undecided on whether or not
to tag it for stable@.