[PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR

[PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR

[Paolo Bonzini]

From: Hyunwoo Kim <imv4bel@gmail.com>

free_nested() frees the shadow VMCS while vmcs01 still points to it. But
because it is asynchronous with respect to loaded_vmcs_clear(), the vCPU
might migrate before the pointer is cleared and __loaded_vmcs_clear()
may then execute VMCLEAR.

The VMCS needs to stay attached until its explicit VMCLEAR completes, but
then it can be hidden and the page safely freed.

Fixes: 355f4fb1405e ("kvm: nVMX: VMCLEAR an active shadow VMCS after last use")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 220d42ebc82e..ddf6df7bee93 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -336,6 +336,7 @@ static void nested_put_vmcs12_pages(struct kvm_vcpu *vcpu)
 static void free_nested(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vmcs *shadow_vmcs;
 
 	if (WARN_ON_ONCE(vmx->loaded_vmcs != &vmx->vmcs01))
 		vmx_switch_vmcs(vcpu, &vmx->vmcs01);
@@ -353,9 +354,15 @@ static void free_nested(struct kvm_vcpu *vcpu)
 	vmx->nested.current_vmptr = INVALID_GPA;
 	if (enable_shadow_vmcs) {
 		vmx_disable_shadow_vmcs(vmx);
-		vmcs_clear(vmx->vmcs01.shadow_vmcs);
-		free_vmcs(vmx->vmcs01.shadow_vmcs);
+
+		/*
+		 * Keep the pointer visible until after VMCLEAR, so migration
+		 * can clear an active shadow VMCS on the old CPU.
+		 */
+		shadow_vmcs = vmx->vmcs01.shadow_vmcs;
+		vmcs_clear(shadow_vmcs);
 		vmx->vmcs01.shadow_vmcs = NULL;
+		free_vmcs(shadow_vmcs);
 	}
 	kfree(vmx->nested.cached_vmcs12);
 	vmx->nested.cached_vmcs12 = NULL;
-- 
2.55.0

Re: [PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing compiler barrier allows pointer nullification to be reordered before VMCLEAR, recreating the UAF.
--

--- Patch [1]: [PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR ---
commit 002acbf68f96812492b255bf63d17f12b9b5b24a
Author: Hyunwoo Kim <imv4bel@gmail.com>

KVM: nVMX: Hide shadow VMCS right after VMCLEAR

This commit updates free_nested() to keep the shadow VMCS pointer visible
until after VMCLEAR has completed. This change is intended to ensure that
if the vCPU migrates, the migration process can still clear an active shadow
VMCS on the old CPU.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 6137e5307d0f6..d63cfdc1740e1 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
[ ... ]
> @@ -350,9 +351,15 @@ static void free_nested(struct kvm_vcpu *vcpu)
>  	vmx->nested.current_vmptr = INVALID_GPA;
>  	if (enable_shadow_vmcs) {
>  		vmx_disable_shadow_vmcs(vmx);
> -		vmcs_clear(vmx->vmcs01.shadow_vmcs);
> -		free_vmcs(vmx->vmcs01.shadow_vmcs);
> +
> +		/*
> +		 * Keep the pointer visible until after VMCLEAR, so migration
> +		 * can clear an active shadow VMCS on the old CPU.
> +		 */
> +		shadow_vmcs = vmx->vmcs01.shadow_vmcs;
> +		vmcs_clear(shadow_vmcs);
>  		vmx->vmcs01.shadow_vmcs = NULL;

[Severity: High]
Does this sequence need a compiler barrier (such as barrier() or WRITE_ONCE())
to prevent the compiler from hoisting the NULL assignment above vmcs_clear()?

Because vmcs_clear() relies on an inline assembly block without a "memory"
clobber and has no direct data dependency with vmx->vmcs01.shadow_vmcs, the
compiler could legally reorder the store. 

If a preemption occurs right between the hoisted assignment and the VMCLEAR,
and the vCPU migrates, vcpu_load() on the new CPU would IPI the old CPU to
clear the loaded VMCS. The old CPU would see the NULL pointer and skip
flushing, potentially leaving an active pointer to freed memory in its
hardware cache.

> +		free_vmcs(shadow_vmcs);
>  	}
>  	kfree(vmx->nested.cached_vmcs12);
>  	vmx->nested.cached_vmcs12 = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721102827.313208-1-pbonzini@redhat.com?part=1

Re: [PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR

[Sean Christopherson]

On Tue, Jul 21, 2026, sashiko-bot@kernel.org wrote:
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 6137e5307d0f6..d63cfdc1740e1 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> [ ... ]
> > @@ -350,9 +351,15 @@ static void free_nested(struct kvm_vcpu *vcpu)
> >  	vmx->nested.current_vmptr = INVALID_GPA;
> >  	if (enable_shadow_vmcs) {
> >  		vmx_disable_shadow_vmcs(vmx);
> > -		vmcs_clear(vmx->vmcs01.shadow_vmcs);
> > -		free_vmcs(vmx->vmcs01.shadow_vmcs);
> > +
> > +		/*
> > +		 * Keep the pointer visible until after VMCLEAR, so migration
> > +		 * can clear an active shadow VMCS on the old CPU.
> > +		 */
> > +		shadow_vmcs = vmx->vmcs01.shadow_vmcs;
> > +		vmcs_clear(shadow_vmcs);
> >  		vmx->vmcs01.shadow_vmcs = NULL;
> 
> [Severity: High]
> Does this sequence need a compiler barrier (such as barrier() or WRITE_ONCE())
> to prevent the compiler from hoisting the NULL assignment above vmcs_clear()?
> 
> Because vmcs_clear() relies on an inline assembly block without a "memory"
> clobber and has no direct data dependency with vmx->vmcs01.shadow_vmcs, the
> compiler could legally reorder the store. 
> 
> If a preemption occurs right between the hoisted assignment and the VMCLEAR,
> and the vCPU migrates, vcpu_load() on the new CPU would IPI the old CPU to
> clear the loaded VMCS. The old CPU would see the NULL pointer and skip
> flushing, potentially leaving an active pointer to freed memory in its
> hardware cache.

FWIW, I had a pretty strong preference for Paolo's off-list suggestion of
explicitly disabling preemption, even before seeing Sashiko's response.  Every
other path the VMCLEARs the shadow VMCS runs with preemption disabled, IMO it's
more intuitive to follow suit here.  And even with the comment, it took a good
5-10 minutes of staring to understand what exactly is going wrong, though that's
probably a me problem.

> > +		free_vmcs(shadow_vmcs);
> >  	}
> >  	kfree(vmx->nested.cached_vmcs12);
> >  	vmx->nested.cached_vmcs12 = NULL;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260721102827.313208-1-pbonzini@redhat.com?part=1

Re: [PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR

[Paolo Bonzini]

On Tue, Jul 21, 2026 at 5:25 PM Sean Christopherson <seanjc@google.com> wrote:
> FWIW, I had a pretty strong preference for Paolo's off-list suggestion of
> explicitly disabling preemption, even before seeing Sashiko's response.  Every
> other path the VMCLEARs the shadow VMCS runs with preemption disabled, IMO it's
> more intuitive to follow suit here.  And even with the comment, it took a good
> 5-10 minutes of staring to understand what exactly is going wrong, though that's
> probably a me problem.

Well, it would be at least an us problem---but I thought I was in the
minority... The only (IMO bad) reason to leave preemption enabled is
to obscure the change, but that's a lost battle anyway these days so
why bother.

That said, I am surprised that VMCLEAR/VMREAD/VMWRITE/VMPTRLD do not
have a memory clobber, since they access internal processor registers
and the compiler may reorder them without the clobber. Fixing that
would also work around Sashiko's complaint, so I guess we should do it
anyway? And disabling preemption can be done separately on top of this
change, which is already on its way to Linus.

Paolo

Re: [PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR

[Sean Christopherson]

On Tue, Jul 21, 2026, Paolo Bonzini wrote:
> On Tue, Jul 21, 2026 at 5:25 PM Sean Christopherson <seanjc@google.com> wrote:
> > FWIW, I had a pretty strong preference for Paolo's off-list suggestion of
> > explicitly disabling preemption, even before seeing Sashiko's response.  Every
> > other path the VMCLEARs the shadow VMCS runs with preemption disabled, IMO it's
> > more intuitive to follow suit here.  And even with the comment, it took a good
> > 5-10 minutes of staring to understand what exactly is going wrong, though that's
> > probably a me problem.
> 
> Well, it would be at least an us problem---but I thought I was in the
> minority... The only (IMO bad) reason to leave preemption enabled is
> to obscure the change, but that's a lost battle anyway these days so
> why bother.
> 
> That said, I am surprised that VMCLEAR/VMREAD/VMWRITE/VMPTRLD do not
> have a memory clobber, since they access internal processor registers
> and the compiler may reorder them without the clobber. Fixing that
> would also work around Sashiko's complaint, so I guess we should do it
> anyway? 

Ya, good point.  We should probably hardcode that into vmx_asm1() and vmx_asm2()?
INVVPID and INVEPT also need the "memory" clobber, no?

> And disabling preemption can be done separately on top of this change, which
> is already on its way to Linus.

+1

Re: [PATCH] KVM: nVMX: Hide shadow VMCS right after VMCLEAR

[Paolo Bonzini]

On Tue, Jul 21, 2026 at 6:27 PM Sean Christopherson <seanjc@google.com> wrote:
> > That said, I am surprised that VMCLEAR/VMREAD/VMWRITE/VMPTRLD do not
> > have a memory clobber, since they access internal processor registers
> > and the compiler may reorder them without the clobber. Fixing that
> > would also work around Sashiko's complaint, so I guess we should do it
> > anyway?
>
> Ya, good point.  We should probably hardcode that into vmx_asm1() and vmx_asm2()?

Definitely the easiest, even ignoring INVVPID/INVEPT...

> INVVPID and INVEPT also need the "memory" clobber, no?

... which strictly speaking do need the clobber, but in practice their
effect is only visible to the next VMLAUNCH/VMRESUME so the risk of
bad consequences from reordering is non-existent.

Patch going out soon...

Paolo