* [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
@ 2026-09-08 13:28 Jim Mattson
2026-09-08 13:57 ` sashiko-bot
2026-09-08 17:34 ` James Houghton
0 siblings, 2 replies; 14+ messages in thread
From: Jim Mattson @ 2026-09-08 13:28 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, Yosry Ahmed, Jim Mattson, stable
When a vCPU is destroyed while L2 is active, KVM synthesizes a nested
VM-Exit, which flushes the cached shadow VMCS12 back to guest memory:
vmx_vcpu_free()
|-> nested_vmx_free_vcpu()
|-> vmx_leave_nested()
|-> nested_vmx_vmexit(vcpu, -1, 0, 0)
|-> nested_flush_cached_shadow_vmcs12()
|-> kvm_write_guest_cached()
|-> __copy_to_user(ghc->hva, ...)
During process exit, do_exit() calls exit_mm() before closing file
descriptors, so vCPU destruction runs with current->mm == NULL on a
borrowed lazy TLB active_mm. If the borrowed address space has a writable
mapping at ghc->hva, __copy_to_user() corrupts an unrelated task's memory.
Skip the flush when KVM synthesizes a VM-exit with vm_exit_reason == -1
(e.g. during vCPU teardown or SMM entry). In these paths, KVM forces the
vCPU out of guest mode internally--no architectural VM-exit is delivered
to L1.
Fixes: 61ada7488ffd ("KVM: nVMX: Cache shadow vmcs12 on VMEntry and flush to memory on VMExit")
Cc: stable@vger.kernel.org
Signed-off-by: Jim Mattson <jmattson@google.com>
---
arch/x86/kvm/vmx/nested.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 151873407abd..33dc8a07019f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5121,21 +5121,28 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
if (likely(!vmx->fail)) {
sync_vmcs02_to_vmcs12(vcpu, vmcs12);
- if (vm_exit_reason != -1)
+ if (vm_exit_reason != -1) {
prepare_vmcs12(vcpu, vmcs12, vm_exit_reason,
exit_intr_info, exit_qualification,
exit_insn_len);
- /*
- * Must happen outside of sync_vmcs02_to_vmcs12() as it will
- * also be used to capture vmcs12 cache as part of
- * capturing nVMX state for snapshot (migration).
- *
- * Otherwise, this flush will dirty guest memory at a
- * point it is already assumed by user-space to be
- * immutable.
- */
- nested_flush_cached_shadow_vmcs12(vcpu, vmcs12);
+ /*
+ * Must happen outside of sync_vmcs02_to_vmcs12() as it will
+ * also be used to capture vmcs12 cache as part of
+ * capturing nVMX state for snapshot (migration).
+ *
+ * Otherwise, this flush will dirty guest memory at a
+ * point it is already assumed by user-space to be
+ * immutable.
+ *
+ * Do not flush when vm_exit_reason == -1 (e.g. forced exit
+ * on vCPU teardown or SMM entry). On teardown, KVM may run
+ * with current->mm == NULL on a borrowed lazy TLB mm,
+ * where writing guest memory would corrupt an unrelated
+ * task. Nothing can observe the flush in that case anyway.
+ */
+ nested_flush_cached_shadow_vmcs12(vcpu, vmcs12);
+ }
} else {
/*
* The only expected VM-instruction error is "VM entry with
base-commit: 28924df2a08f440c73991b83028032c901de2ae4
--
2.55.0.979.g7e5102b832-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-08 13:28 [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown Jim Mattson
@ 2026-09-08 13:57 ` sashiko-bot
2026-09-08 17:34 ` James Houghton
1 sibling, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-09-08 13:57 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Live migration during SMM with an active nested guest overwrites L2 state (e.g. RIP/RSP) with stale data from the hardware shadow VMCS.
- [High] Live migration during SMM with nested shadow VMCS enabled causes a Triple Fault on the destination.
- [High] During vCPU teardown, __nested_vmx_vmexit() unconditionally loads L1 host state, triggering a cross-process memory read via load_pdptrs() with a borrowed active_mm.
--
commit 15d3183acc0011b1c1063bab25d832a346f7a45c
Author: Jim Mattson <jmattson@google.com>
KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
When a vCPU is destroyed while L2 is active, KVM synthesizes a nested VM-Exit.
During this process exit, the vCPU destruction runs with a borrowed lazy TLB
active_mm, so skipping the shadow VMCS12 flush prevents __copy_to_user()
from corrupting an unrelated task's memory.
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 151873407abd3..33dc8a07019f8 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5121,21 +5121,28 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
> if (likely(!vmx->fail)) {
> sync_vmcs02_to_vmcs12(vcpu, vmcs12);
>
> - if (vm_exit_reason != -1)
> + if (vm_exit_reason != -1) {
> prepare_vmcs12(vcpu, vmcs12, vm_exit_reason,
> exit_intr_info, exit_qualification,
> exit_insn_len);
>
> - /*
> - * Must happen outside of sync_vmcs02_to_vmcs12() as it will
> - * also be used to capture vmcs12 cache as part of
> - * capturing nVMX state for snapshot (migration).
> - *
> - * Otherwise, this flush will dirty guest memory at a
> - * point it is already assumed by user-space to be
> - * immutable.
> - */
> - nested_flush_cached_shadow_vmcs12(vcpu, vmcs12);
> + /*
> + * Must happen outside of sync_vmcs02_to_vmcs12() as it will
> + * also be used to capture vmcs12 cache as part of
> + * capturing nVMX state for snapshot (migration).
> + *
> + * Otherwise, this flush will dirty guest memory at a
> + * point it is already assumed by user-space to be
> + * immutable.
> + *
> + * Do not flush when vm_exit_reason == -1 (e.g. forced exit
> + * on vCPU teardown or SMM entry). On teardown, KVM may run
> + * with current->mm == NULL on a borrowed lazy TLB mm,
> + * where writing guest memory would corrupt an unrelated
> + * task. Nothing can observe the flush in that case anyway.
> + */
> + nested_flush_cached_shadow_vmcs12(vcpu, vmcs12);
> + }
> } else {
> /*
> * The only expected VM-instruction error is "VM entry with
[Severity: High]
This is a pre-existing issue, but since this patch handles vCPU teardown with
a borrowed active_mm, does the later call to load_vmcs12_host_state() in
__nested_vmx_vmexit() have a similar issue?
During vCPU teardown, __nested_vmx_vmexit() unconditionally loads L1 host
state:
arch/x86/kvm/vmx/nested.c:__nested_vmx_vmexit() {
...
load_vmcs12_host_state(vcpu, vmcs12);
...
}
If the L1 host state uses PAE paging, nested_vmx_load_cr3() triggers the
PDPTR load:
arch/x86/kvm/vmx/nested.c:nested_vmx_load_cr3() {
...
if (reload_pdptrs && !nested_ept && is_pae_paging(vcpu) &&
CC(!load_pdptrs(vcpu, cr3))) {
...
}
This executes a memory read using copy_from_user() in the context of the
borrowed active_mm:
arch/x86/kvm/regs.c:load_pdptrs() {
...
ret = kvm_vcpu_read_guest_page(vcpu, gpa_to_gfn(real_gpa), pdpte,
cr3 & GENMASK(11, 5), sizeof(pdpte));
...
}
Could this cross-process memory read cause destructive side-effects if it hits
a memory-mapped I/O (MMIO) region in the unrelated task?
[Severity: Critical]
This is a pre-existing issue, but does skipping the shadow VMCS12 flush on
SMM entry (where vm_exit_reason == -1) cause data corruption during live
migration?
In __nested_vmx_vmexit(), need_vmcs12_to_shadow_sync is prevented from being
set on SMM entry because of the vm_exit_reason check:
arch/x86/kvm/vmx/nested.c:__nested_vmx_vmexit() {
...
if ((vm_exit_reason != -1) &&
(enable_shadow_vmcs || nested_vmx_is_evmptr12_valid(vmx)))
vmx->nested.need_vmcs12_to_shadow_sync = true;
...
}
If live migration is initiated while in SMM, vmx_get_nested_state() treats
the hardware shadow VMCS as authoritative:
arch/x86/kvm/vmx/nested.c:vmx_get_nested_state() {
...
else if (enable_shadow_vmcs)
copy_shadow_to_vmcs12(vmx);
...
}
Since need_vmcs12_to_shadow_sync was not set, doesn't this overwrite the
correctly synced vmcs12 with stale data from the hardware shadow VMCS,
resulting in guest state rollback (e.g., RIP/RSP)?
[Severity: High]
This isn't a bug introduced by this patch, but is there a Triple Fault risk
during live migration in SMM with nested shadow VMCS enabled?
Because is_guest_mode is false in SMM, the shadow_vmcs12 is omitted from the
migration stream:
arch/x86/kvm/vmx/nested.c:vmx_get_nested_state() {
...
if (is_guest_mode(vcpu) &&
nested_cpu_has_shadow_vmcs(vmcs12) &&
vmcs12->vmcs_link_pointer != INVALID_GPA)
kvm_state.size += sizeof(user_vmx_nested_state->shadow_vmcs12);
...
}
On the destination, vmx_set_nested_state() skips restoring shadow_vmcs12 and
returns early:
arch/x86/kvm/vmx/nested.c:vmx_set_nested_state() {
...
if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE))
return 0;
...
}
When the guest leaves SMM, vmx_leave_smm() checks the revision ID of the
zeroed shadow_vmcs12:
arch/x86/kvm/vmx/vmx.c:vmx_leave_smm() {
...
/* Triple fault if the state is invalid. */
if (nested_vmx_check_restored_vmcs12(vcpu) < 0)
return 1;
...
}
Could this zeroed buffer fail the revision ID check and induce a Triple Fault?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260908132838.2116068-1-jmattson@google.com?part=1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-08 13:28 [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown Jim Mattson
2026-09-08 13:57 ` sashiko-bot
@ 2026-09-08 17:34 ` James Houghton
2026-09-08 19:12 ` Jim Mattson
1 sibling, 1 reply; 14+ messages in thread
From: James Houghton @ 2026-09-08 17:34 UTC (permalink / raw)
To: Jim Mattson; +Cc: Sean Christopherson, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Tue, Sep 8, 2026 at 6:54 AM Jim Mattson <jmattson@google.com> wrote:
>
> When a vCPU is destroyed while L2 is active, KVM synthesizes a nested
> VM-Exit, which flushes the cached shadow VMCS12 back to guest memory:
>
> vmx_vcpu_free()
> |-> nested_vmx_free_vcpu()
> |-> vmx_leave_nested()
> |-> nested_vmx_vmexit(vcpu, -1, 0, 0)
> |-> nested_flush_cached_shadow_vmcs12()
> |-> kvm_write_guest_cached()
> |-> __copy_to_user(ghc->hva, ...)
>
> During process exit, do_exit() calls exit_mm() before closing file
> descriptors, so vCPU destruction runs with current->mm == NULL on a
> borrowed lazy TLB active_mm. If the borrowed address space has a writable
> mapping at ghc->hva, __copy_to_user() corrupts an unrelated task's memory.
>
> Skip the flush when KVM synthesizes a VM-exit with vm_exit_reason == -1
> (e.g. during vCPU teardown or SMM entry). In these paths, KVM forces the
> vCPU out of guest mode internally--no architectural VM-exit is delivered
> to L1.
>
> Fixes: 61ada7488ffd ("KVM: nVMX: Cache shadow vmcs12 on VMEntry and flush to memory on VMExit")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jim Mattson <jmattson@google.com>
Hi Jim,
This patch looks good for a stable backport, but I feel this kind of
bug could easily happen again with the current API.
One simple thing that would have prevented this would be to:
1. Change kvm_write_guest_cached() (and related) to take the vcpu
instead of the kvm, and
2. Check that vcpu->mm == current->mm before doing the uaccess.
This wouldn't be as suitable for a backport, but I think it's better
for preventing similar classes of bugs. I'm sure you (and
Paolo/Sean/others) will have better ideas. What do you think?
Thank you very much for debugging this. :)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-08 17:34 ` James Houghton
@ 2026-09-08 19:12 ` Jim Mattson
2026-09-09 15:41 ` James Houghton
2026-09-09 19:00 ` Sean Christopherson
0 siblings, 2 replies; 14+ messages in thread
From: Jim Mattson @ 2026-09-08 19:12 UTC (permalink / raw)
To: James Houghton
Cc: Sean Christopherson, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Tue, Sep 8, 2026 at 10:35 AM James Houghton <jthoughton@google.com> wrote:
>
> On Tue, Sep 8, 2026 at 6:54 AM Jim Mattson <jmattson@google.com> wrote:
> >
> > When a vCPU is destroyed while L2 is active, KVM synthesizes a nested
> > VM-Exit, which flushes the cached shadow VMCS12 back to guest memory:
> >
> > vmx_vcpu_free()
> > |-> nested_vmx_free_vcpu()
> > |-> vmx_leave_nested()
> > |-> nested_vmx_vmexit(vcpu, -1, 0, 0)
> > |-> nested_flush_cached_shadow_vmcs12()
> > |-> kvm_write_guest_cached()
> > |-> __copy_to_user(ghc->hva, ...)
> >
> > During process exit, do_exit() calls exit_mm() before closing file
> > descriptors, so vCPU destruction runs with current->mm == NULL on a
> > borrowed lazy TLB active_mm. If the borrowed address space has a writable
> > mapping at ghc->hva, __copy_to_user() corrupts an unrelated task's memory.
> >
> > Skip the flush when KVM synthesizes a VM-exit with vm_exit_reason == -1
> > (e.g. during vCPU teardown or SMM entry). In these paths, KVM forces the
> > vCPU out of guest mode internally--no architectural VM-exit is delivered
> > to L1.
> >
> > Fixes: 61ada7488ffd ("KVM: nVMX: Cache shadow vmcs12 on VMEntry and flush to memory on VMExit")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jim Mattson <jmattson@google.com>
>
> Hi Jim,
>
> This patch looks good for a stable backport, but I feel this kind of
> bug could easily happen again with the current API.
>
> One simple thing that would have prevented this would be to:
> 1. Change kvm_write_guest_cached() (and related) to take the vcpu
> instead of the kvm, and
> 2. Check that vcpu->mm == current->mm before doing the uaccess.
>
> This wouldn't be as suitable for a backport, but I think it's better
> for preventing similar classes of bugs. I'm sure you (and
> Paolo/Sean/others) will have better ideas. What do you think?
There seems to be some awareness of this issue within KVM, so that may
not be necessary. Also, as Sashiko points out, we have issues with
reads as well as writes. I don't know how many choke points we'd have
to test.
AFAICT (with AI assistance), all of the current problems are rooted in
the following call to nested_vmx_vmexit():
void vmx_leave_nested(struct kvm_vcpu *vcpu)
{
if (is_guest_mode(vcpu)) {
vcpu->arch.nested_run_pending = 0;
nested_vmx_vmexit(vcpu, -1, 0, 0);
}
free_nested(vcpu);
}
On the SVM side, there is no comparable faux VM-exit. Instead of
calling nested_svm_vmexit(), the teardown of SVM nested state is an
open-coded sequence. Maybe something like that would work better for
VMX?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-08 19:12 ` Jim Mattson
@ 2026-09-09 15:41 ` James Houghton
2026-09-09 19:00 ` Sean Christopherson
1 sibling, 0 replies; 14+ messages in thread
From: James Houghton @ 2026-09-09 15:41 UTC (permalink / raw)
To: Jim Mattson; +Cc: Sean Christopherson, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Tue, Sep 8, 2026 at 12:13 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Tue, Sep 8, 2026 at 10:35 AM James Houghton <jthoughton@google.com> wrote:
> > Hi Jim,
> >
> > This patch looks good for a stable backport, but I feel this kind of
> > bug could easily happen again with the current API.
> >
> > One simple thing that would have prevented this would be to:
> > 1. Change kvm_write_guest_cached() (and related) to take the vcpu
> > instead of the kvm, and
> > 2. Check that vcpu->mm == current->mm before doing the uaccess.
> >
> > This wouldn't be as suitable for a backport, but I think it's better
> > for preventing similar classes of bugs. I'm sure you (and
> > Paolo/Sean/others) will have better ideas. What do you think?
>
> There seems to be some awareness of this issue within KVM, so that may
> not be necessary. Also, as Sashiko points out, we have issues with
> reads as well as writes. I don't know how many choke points we'd have
> to test.
>
> AFAICT (with AI assistance), all of the current problems are rooted in
> the following call to nested_vmx_vmexit():
>
> void vmx_leave_nested(struct kvm_vcpu *vcpu)
> {
> if (is_guest_mode(vcpu)) {
> vcpu->arch.nested_run_pending = 0;
> nested_vmx_vmexit(vcpu, -1, 0, 0);
> }
> free_nested(vcpu);
> }
>
> On the SVM side, there is no comparable faux VM-exit. Instead of
> calling nested_svm_vmexit(), the teardown of SVM nested state is an
> open-coded sequence. Maybe something like that would work better for
> VMX?
Maybe, but I don't think I'd feel any better about it (because the
change is still KVM/x86-specific).
Thinking harder about this, I think it is always invalid to call
`copy_to_user()` if `current->mm` is NULL. The riscv implementation[1]
seems to require `current->mm` to be non-NULL. So I'd be much happier
with a check in `__access_ok()` that `current->mm != NULL`, but I'm
not 100% sure that is okay.
I'm also trying to think a little bit harder about the exact mechanism
that causes the memory corruption. I think for `active_mm` (and
therefore the loaded page tables/CR3) to be different than the mm we
destroyed in `exit_mm()`, there must have been a context switch to the
victim user process and then back to the dying process after
`exit_mm()`. Right?
[1] See arch/riscv/include/asm/uaccess.h
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-08 19:12 ` Jim Mattson
2026-09-09 15:41 ` James Houghton
@ 2026-09-09 19:00 ` Sean Christopherson
2026-09-10 18:58 ` James Houghton
2026-09-11 17:39 ` Jim Mattson
1 sibling, 2 replies; 14+ messages in thread
From: Sean Christopherson @ 2026-09-09 19:00 UTC (permalink / raw)
To: Jim Mattson; +Cc: James Houghton, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Tue, Sep 08, 2026, Jim Mattson wrote:
> On Tue, Sep 8, 2026 at 10:35 AM James Houghton <jthoughton@google.com> wrote:
> >
> > On Tue, Sep 8, 2026 at 6:54 AM Jim Mattson <jmattson@google.com> wrote:
> > >
> > > When a vCPU is destroyed while L2 is active, KVM synthesizes a nested
> > > VM-Exit, which flushes the cached shadow VMCS12 back to guest memory:
> > >
> > > vmx_vcpu_free()
> > > |-> nested_vmx_free_vcpu()
> > > |-> vmx_leave_nested()
> > > |-> nested_vmx_vmexit(vcpu, -1, 0, 0)
> > > |-> nested_flush_cached_shadow_vmcs12()
> > > |-> kvm_write_guest_cached()
> > > |-> __copy_to_user(ghc->hva, ...)
> > >
> > > During process exit, do_exit() calls exit_mm() before closing file
> > > descriptors, so vCPU destruction runs with current->mm == NULL on a
> > > borrowed lazy TLB active_mm. If the borrowed address space has a writable
> > > mapping at ghc->hva, __copy_to_user() corrupts an unrelated task's memory.
> > >
> > > Skip the flush when KVM synthesizes a VM-exit with vm_exit_reason == -1
> > > (e.g. during vCPU teardown or SMM entry). In these paths, KVM forces the
> > > vCPU out of guest mode internally--no architectural VM-exit is delivered
> > > to L1.
> > >
> > > Fixes: 61ada7488ffd ("KVM: nVMX: Cache shadow vmcs12 on VMEntry and flush to memory on VMExit")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> >
> > Hi Jim,
> >
> > This patch looks good for a stable backport, but I feel this kind of
> > bug could easily happen again with the current API.
> >
> > One simple thing that would have prevented this would be to:
> > 1. Change kvm_write_guest_cached() (and related) to take the vcpu
> > instead of the kvm, and
> > 2. Check that vcpu->mm == current->mm before doing the uaccess.
The mm pointer is store in "kvm", not in "kvm_vcpu", i.e. this sort of hardening
shouldn't require modifying the callsites.
> > This wouldn't be as suitable for a backport, but I think it's better
> > for preventing similar classes of bugs. I'm sure you (and
> > Paolo/Sean/others) will have better ideas. What do you think?
>
> There seems to be some awareness of this issue within KVM, so that may
> not be necessary. Also, as Sashiko points out, we have issues with
> reads as well as writes. I don't know how many choke points we'd have
> to test.
>
> AFAICT (with AI assistance), all of the current problems are rooted in
> the following call to nested_vmx_vmexit():
>
> void vmx_leave_nested(struct kvm_vcpu *vcpu)
> {
> if (is_guest_mode(vcpu)) {
> vcpu->arch.nested_run_pending = 0;
> nested_vmx_vmexit(vcpu, -1, 0, 0);
> }
> free_nested(vcpu);
> }
>
> On the SVM side, there is no comparable faux VM-exit. Instead of
> calling nested_svm_vmexit(), the teardown of SVM nested state is an
> open-coded sequence. Maybe something like that would work better for
> VMX?
Yes, vmx_leave_nested()'s use of nested_vmx_vmexit() is an endless source of pain
and needs to be rewritten.
However, the bigger flaw is that the memslots are still valid when the VM is
being destroyed. The hardening James suggested above really should be hardening,
not the primary mechanism for ensuring correctness.
Manually freeing each memslot one-by-one isn't a great option, because the latency
introduced by each synchronize_srcu_expedited() call would be rather absurd. But
once KVM unregisters its mmu_notifier, i.e. once kvm_mmu_notifier_release() and
thus kvm_flush_shadow_all() runs, all indirect references to memslots need to be
gone. And by "indirect references" I mean code in KVM that relies on a memslot
existing and being reachable, e.g. x86's rmaps and shadow page accounting.
KVM is infuriatingly close to enforcing that already, as only kvm_arch_destroy_vm()
and kvm_destroy_devices() run between unregistering the mmu_notifier and freeing
all memslot metadata.
mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
/*
* At this point, pending calls to invalidate_range_start()
* have completed but no more MMU notifiers will run, so
* mn_active_invalidate_count may remain unbalanced.
* No threads can be waiting in kvm_swap_active_memslots() as the
* last reference on KVM has been dropped, but freeing
* memslots would deadlock without this manual intervention.
*
* If the count isn't unbalanced, i.e. KVM did NOT unregister its MMU
* notifier between a start() and end(), then there shouldn't be any
* in-progress invalidations.
*/
WARN_ON(rcuwait_active(&kvm->mn_memslots_update_rcuwait));
if (kvm->mn_active_invalidate_count)
kvm->mn_active_invalidate_count = 0;
else
WARN_ON(kvm->mmu_invalidate_in_progress);
kvm_arch_destroy_vm(kvm);
kvm_destroy_devices(kvm);
for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
kvm_free_memslots(kvm, &kvm->__memslots[i][1]); <=== KVM will be very sad after this
}
Destroying memslots before kvm_destroy_devices() is a-ok, all implementations do
nothing more than kvm_io_bus_unregister_dev() (a nop at this point in the VM's
lifecycle, as buses are already destroyed), and free of memory.
Unfortunately, kvm_arch_destroy_vm() is practically infeasible to audit. But, we
don't need to audit that code, we just need to audit kvm_arch_flush_shadow_memslot(),
because if there are memslot references that are dropped by flush_shadow_memslot()
but not flush_shadow_all(), then KVM is already buggy and vulnerable.
*sigh*
And of course PPC is a disaster and does literally nothing on flush_shadow_all().
Doubly hilarious, the worst offender seems to be kvmhv_release_all_nested(). As
a quick and dirty fix, I think we can simply iterate over memslots and manually
flush each one?
Which, amazingly, appears to be safe even though kvmppc_uvmem_drop_pages() takes
mmap_lock, as exit_mmap()'s call to mmu_notifier_release() is (thanfully) super
obviously done without hold mmap_lock.
/* mm's last user has gone, and its about to be pulled down */
mmu_notifier_release(mm);
mmap_read_lock(mm);
diff --git arch/powerpc/include/asm/kvm_host.h arch/powerpc/include/asm/kvm_host.h
index 2d139c807577..1c8d9d7360e9 100644
--- arch/powerpc/include/asm/kvm_host.h
+++ arch/powerpc/include/asm/kvm_host.h
@@ -903,7 +903,6 @@ struct kvm_vcpu_arch {
#define __KVM_HAVE_CREATE_DEVICE
static inline void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen) {}
-static inline void kvm_arch_flush_shadow_all(struct kvm *kvm) {}
static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {}
static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
diff --git arch/powerpc/kvm/powerpc.c arch/powerpc/kvm/powerpc.c
index 9194cf492d1c..2b88fb9e9bc4 100644
--- arch/powerpc/kvm/powerpc.c
+++ arch/powerpc/kvm/powerpc.c
@@ -764,6 +764,17 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
kvmppc_core_commit_memory_region(kvm, old, new, change);
}
+void kvm_arch_flush_shadow_all(struct kvm *kvm)
+{
+ struct kvm_memory_slot *memslot;
+ struct kvm_memslots *slots;
+ int bkt;
+
+ slots = kvm_memslots(kvm);
+ kvm_for_each_memslot(memslot, bkt, slots)
+ kvmppc_core_flush_memslot(kvm, slot);
+}
+
void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
struct kvm_memory_slot *slot)
{
If the above works for PPC, then KVM can nuke memslots before calling into
kvm_arch_destroy_vm(). x86's asinine memslot deletion in kvm_arch_destroy_vm()
needs to be addressed, but that code exists purely to do vm_munmap(), and can
and should be moved to kvm_arch_free_memslot().
All that said, I'm not sure this aggressive fix is the right thing to send to
stable@. For that, James' suggestion of hardening KVM's usage of
__copy_{to,from}_user() seems like the best blend of being comprehensive without
being overly invasive/risky.
So, as an immediate set of changes, what if we do this over ~5 patches, with patches
1 and 2 tagged for stable@?
1. Add kvm_copy_{to,from}_user{,_inatomic)() and return -EFAULT if current->mm
is not kvm->mm.
2. Hack-a-fix PPC's kvm_arch_flush_shadow_all().
3. Do x86's vm_munmap() in kvm_arch_free_memslot().
4. Nuke memslots before calling kvm_arch_destroy_vm().
5. Change the current->mm checks in kvm_copy_{to,from}_user{,_inatomic)() to
WARN_ON_ONCE() on failure.
And then in the near-ish future, take things a step further and do:
6. Fix the vmx_leave_nested() trainwreck.
7. Harden the common kvm_{read,write}_guest family of APIs even further by
adding an early WARN_ON_ONCE() on current->mm != kvm->mm, i.e. to detect
bad KVM behavior as additional defense-in-depth.
The diff for #3 and #4 (lightly tested on x86):
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4b3681796c75..f05ba2d1364d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9925,7 +9925,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
* @size > 0 to install a new slot, while @size == 0 to uninstall a
* slot. The return code can be one of the following:
*
- * HVA: on success (uninstall will return a bogus HVA)
+ * HVA: on success (uninstall will return a NULL HVA)
* -errno: on error
*
* The caller should always use IS_ERR() to check the return value
@@ -9938,10 +9938,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
u32 size)
{
- int i, r;
- unsigned long hva, old_npages;
struct kvm_memslots *slots = kvm_memslots(kvm);
struct kvm_memory_slot *slot;
+ unsigned long hva;
+ int i, r;
lockdep_assert_held(&kvm->slots_lock);
@@ -9965,8 +9965,7 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
if (!slot || !slot->npages)
return NULL;
- old_npages = slot->npages;
- hva = slot->userspace_addr;
+ hva = 0;
}
for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
@@ -9982,9 +9981,6 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
return ERR_PTR_USR(r);
}
- if (!size)
- vm_munmap(hva, old_npages * PAGE_SIZE);
-
return (void __user *)hva;
}
EXPORT_SYMBOL_FOR_KVM_INTERNAL(__x86_set_memory_region);
@@ -10013,20 +10009,6 @@ void kvm_arch_pre_destroy_vm(struct kvm *kvm)
void kvm_arch_destroy_vm(struct kvm *kvm)
{
- if (current->mm == kvm->mm) {
- /*
- * Free memory regions allocated on behalf of userspace,
- * unless the memory map has changed due to process exit
- * or fd copying.
- */
- mutex_lock(&kvm->slots_lock);
- __x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
- 0, 0);
- __x86_set_memory_region(kvm, IDENTITY_PAGETABLE_PRIVATE_MEMSLOT,
- 0, 0);
- __x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, 0, 0);
- mutex_unlock(&kvm->slots_lock);
- }
if (kvm->arch.created_mediated_pmu)
perf_release_mediated_pmu();
kvm_destroy_vcpus(kvm);
@@ -10066,6 +10048,16 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
}
kvm_page_track_free_memslot(slot);
+
+ /*
+ * Free memory regions allocated on behalf of userspace, unless the
+ * memory map has changed due to process exit or fd copying. Leak the
+ * mapping on failure, e.g. if the task is killed, worst case scenario,
+ * the page(s) will be reclaimed when the process exits.
+ */
+ if (current->mm == kvm->mm && slot->id >= KVM_USER_MEM_SLOTS &&
+ !WARN_ON_ONCE(!slot->npages))
+ vm_munmap(slot->userspace_addr, slot->npages * PAGE_SIZE);
}
int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 03bfc92864b6..bed48f70b2d8 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1693,6 +1693,7 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu);
bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu);
bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu);
+void kvm_arch_destroy_memslots(struct kvm *kvm);
void kvm_arch_pre_destroy_vm(struct kvm *kvm);
void kvm_arch_create_vm_debugfs(struct kvm *kvm);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..899bf5970434 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -945,23 +945,36 @@ static void kvm_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
kfree(slot);
}
-static void kvm_free_memslots(struct kvm *kvm, struct kvm_memslots *slots)
+static const struct kvm_memslots kvm_empty_memslots = {
+ .generation = -1ull,
+ .hva_tree = RB_ROOT_CACHED,
+ .gfn_tree = RB_ROOT,
+ .id_hash[0 ... (ARRAY_SIZE(kvm_empty_memslots.id_hash) - 1)] = HLIST_HEAD_INIT,
+ .node_idx = 0,
+};
+
+static void kvm_destroy_memslots(struct kvm *kvm)
{
struct hlist_node *idnode;
struct kvm_memory_slot *memslot;
- int bkt;
+ int bkt, i;
+
+ mutex_lock(&kvm->slots_lock);
+ for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++)
+ rcu_assign_pointer(kvm->memslots[i], &kvm_empty_memslots);
+
+ synchronize_srcu_expedited(&kvm->srcu);
+ mutex_unlock(&kvm->slots_lock);
/*
* The same memslot objects live in both active and inactive sets,
- * arbitrarily free using index '1' so the second invocation of this
- * function isn't operating over a structure with dangling pointers
- * (even though this function isn't actually touching them).
+ * arbitrarily free using index '1'.
*/
- if (!slots->node_idx)
- return;
-
- hash_for_each_safe(slots->id_hash, bkt, idnode, memslot, id_node[1])
- kvm_free_memslot(kvm, memslot);
+ for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
+ hash_for_each_safe(kvm->__memslots[i][1].id_hash, bkt, idnode,
+ memslot, id_node[1])
+ kvm_free_memslot(kvm, memslot);
+ }
}
static umode_t kvm_stats_debugfs_mode(const struct kvm_stats_desc *desc)
@@ -1292,12 +1305,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
kvm->mn_active_invalidate_count = 0;
else
WARN_ON(kvm->mmu_invalidate_in_progress);
+ kvm_destroy_memslots(kvm);
+
kvm_arch_destroy_vm(kvm);
kvm_destroy_devices(kvm);
- for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
- kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
- kvm_free_memslots(kvm, &kvm->__memslots[i][1]);
- }
cleanup_srcu_struct(&kvm->irq_srcu);
srcu_barrier(&kvm->srcu);
cleanup_srcu_struct(&kvm->srcu);
@@ -2005,6 +2016,9 @@ static int kvm_set_memory_region(struct kvm *kvm,
lockdep_assert_held(&kvm->slots_lock);
+ if (WARN_ON_ONCE(!refcount_read(&kvm->users_count)))
+ return -EIO;
+
r = check_memory_region_flags(kvm, mem);
if (r)
return r;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-09 19:00 ` Sean Christopherson
@ 2026-09-10 18:58 ` James Houghton
2026-09-10 19:14 ` Sean Christopherson
2026-09-11 17:39 ` Jim Mattson
1 sibling, 1 reply; 14+ messages in thread
From: James Houghton @ 2026-09-10 18:58 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Jim Mattson, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Wed, Sep 9, 2026 at 12:00 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Sep 08, 2026, Jim Mattson wrote:
> > On Tue, Sep 8, 2026 at 10:35 AM James Houghton <jthoughton@google.com> wrote:
> > >
> > > On Tue, Sep 8, 2026 at 6:54 AM Jim Mattson <jmattson@google.com> wrote:
> > > >
> > > > When a vCPU is destroyed while L2 is active, KVM synthesizes a nested
> > > > VM-Exit, which flushes the cached shadow VMCS12 back to guest memory:
> > > >
> > > > vmx_vcpu_free()
> > > > |-> nested_vmx_free_vcpu()
> > > > |-> vmx_leave_nested()
> > > > |-> nested_vmx_vmexit(vcpu, -1, 0, 0)
> > > > |-> nested_flush_cached_shadow_vmcs12()
> > > > |-> kvm_write_guest_cached()
> > > > |-> __copy_to_user(ghc->hva, ...)
> > > >
> > > > During process exit, do_exit() calls exit_mm() before closing file
> > > > descriptors, so vCPU destruction runs with current->mm == NULL on a
> > > > borrowed lazy TLB active_mm. If the borrowed address space has a writable
> > > > mapping at ghc->hva, __copy_to_user() corrupts an unrelated task's memory.
> > > >
> > > > Skip the flush when KVM synthesizes a VM-exit with vm_exit_reason == -1
> > > > (e.g. during vCPU teardown or SMM entry). In these paths, KVM forces the
> > > > vCPU out of guest mode internally--no architectural VM-exit is delivered
> > > > to L1.
> > > >
> > > > Fixes: 61ada7488ffd ("KVM: nVMX: Cache shadow vmcs12 on VMEntry and flush to memory on VMExit")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > >
> > > Hi Jim,
> > >
> > > This patch looks good for a stable backport, but I feel this kind of
> > > bug could easily happen again with the current API.
> > >
> > > One simple thing that would have prevented this would be to:
> > > 1. Change kvm_write_guest_cached() (and related) to take the vcpu
> > > instead of the kvm, and
> > > 2. Check that vcpu->mm == current->mm before doing the uaccess.
>
> The mm pointer is store in "kvm", not in "kvm_vcpu", i.e. this sort of hardening
> shouldn't require modifying the callsites.
Yeahhh, I noticed this a few minutes after I sent that email, heh. :(
> > > This wouldn't be as suitable for a backport, but I think it's better
> > > for preventing similar classes of bugs. I'm sure you (and
> > > Paolo/Sean/others) will have better ideas. What do you think?
> >
> > There seems to be some awareness of this issue within KVM, so that may
> > not be necessary. Also, as Sashiko points out, we have issues with
> > reads as well as writes. I don't know how many choke points we'd have
> > to test.
> >
> > AFAICT (with AI assistance), all of the current problems are rooted in
> > the following call to nested_vmx_vmexit():
> >
> > void vmx_leave_nested(struct kvm_vcpu *vcpu)
> > {
> > if (is_guest_mode(vcpu)) {
> > vcpu->arch.nested_run_pending = 0;
> > nested_vmx_vmexit(vcpu, -1, 0, 0);
> > }
> > free_nested(vcpu);
> > }
> >
> > On the SVM side, there is no comparable faux VM-exit. Instead of
> > calling nested_svm_vmexit(), the teardown of SVM nested state is an
> > open-coded sequence. Maybe something like that would work better for
> > VMX?
>
> Yes, vmx_leave_nested()'s use of nested_vmx_vmexit() is an endless source of pain
> and needs to be rewritten.
>
> However, the bigger flaw is that the memslots are still valid when the VM is
> being destroyed. The hardening James suggested above really should be hardening,
> not the primary mechanism for ensuring correctness.
>
> Manually freeing each memslot one-by-one isn't a great option, because the latency
> introduced by each synchronize_srcu_expedited() call would be rather absurd. But
> once KVM unregisters its mmu_notifier, i.e. once kvm_mmu_notifier_release() and
> thus kvm_flush_shadow_all() runs, all indirect references to memslots need to be
> gone. And by "indirect references" I mean code in KVM that relies on a memslot
> existing and being reachable, e.g. x86's rmaps and shadow page accounting.
>
> KVM is infuriatingly close to enforcing that already, as only kvm_arch_destroy_vm()
> and kvm_destroy_devices() run between unregistering the mmu_notifier and freeing
> all memslot metadata.
>
> mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
> /*
> * At this point, pending calls to invalidate_range_start()
> * have completed but no more MMU notifiers will run, so
> * mn_active_invalidate_count may remain unbalanced.
> * No threads can be waiting in kvm_swap_active_memslots() as the
> * last reference on KVM has been dropped, but freeing
> * memslots would deadlock without this manual intervention.
> *
> * If the count isn't unbalanced, i.e. KVM did NOT unregister its MMU
> * notifier between a start() and end(), then there shouldn't be any
> * in-progress invalidations.
> */
> WARN_ON(rcuwait_active(&kvm->mn_memslots_update_rcuwait));
> if (kvm->mn_active_invalidate_count)
> kvm->mn_active_invalidate_count = 0;
> else
> WARN_ON(kvm->mmu_invalidate_in_progress);
> kvm_arch_destroy_vm(kvm);
> kvm_destroy_devices(kvm);
> for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
> kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
> kvm_free_memslots(kvm, &kvm->__memslots[i][1]); <=== KVM will be very sad after this
>
> }
>
> Destroying memslots before kvm_destroy_devices() is a-ok, all implementations do
> nothing more than kvm_io_bus_unregister_dev() (a nop at this point in the VM's
> lifecycle, as buses are already destroyed), and free of memory.
>
> Unfortunately, kvm_arch_destroy_vm() is practically infeasible to audit. But, we
> don't need to audit that code, we just need to audit kvm_arch_flush_shadow_memslot(),
> because if there are memslot references that are dropped by flush_shadow_memslot()
> but not flush_shadow_all(), then KVM is already buggy and vulnerable.
>
> *sigh*
>
> And of course PPC is a disaster and does literally nothing on flush_shadow_all().
> Doubly hilarious, the worst offender seems to be kvmhv_release_all_nested(). As
> a quick and dirty fix, I think we can simply iterate over memslots and manually
> flush each one?
>
> Which, amazingly, appears to be safe even though kvmppc_uvmem_drop_pages() takes
> mmap_lock, as exit_mmap()'s call to mmu_notifier_release() is (thanfully) super
> obviously done without hold mmap_lock.
>
> /* mm's last user has gone, and its about to be pulled down */
> mmu_notifier_release(mm);
>
> mmap_read_lock(mm);
>
> diff --git arch/powerpc/include/asm/kvm_host.h arch/powerpc/include/asm/kvm_host.h
> index 2d139c807577..1c8d9d7360e9 100644
> --- arch/powerpc/include/asm/kvm_host.h
> +++ arch/powerpc/include/asm/kvm_host.h
> @@ -903,7 +903,6 @@ struct kvm_vcpu_arch {
> #define __KVM_HAVE_CREATE_DEVICE
>
> static inline void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen) {}
> -static inline void kvm_arch_flush_shadow_all(struct kvm *kvm) {}
> static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {}
> static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
>
> diff --git arch/powerpc/kvm/powerpc.c arch/powerpc/kvm/powerpc.c
> index 9194cf492d1c..2b88fb9e9bc4 100644
> --- arch/powerpc/kvm/powerpc.c
> +++ arch/powerpc/kvm/powerpc.c
> @@ -764,6 +764,17 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> kvmppc_core_commit_memory_region(kvm, old, new, change);
> }
>
> +void kvm_arch_flush_shadow_all(struct kvm *kvm)
> +{
> + struct kvm_memory_slot *memslot;
> + struct kvm_memslots *slots;
> + int bkt;
> +
> + slots = kvm_memslots(kvm);
> + kvm_for_each_memslot(memslot, bkt, slots)
> + kvmppc_core_flush_memslot(kvm, slot);
> +}
> +
> void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> struct kvm_memory_slot *slot)
> {
>
> If the above works for PPC, then KVM can nuke memslots before calling into
> kvm_arch_destroy_vm(). x86's asinine memslot deletion in kvm_arch_destroy_vm()
> needs to be addressed, but that code exists purely to do vm_munmap(), and can
> and should be moved to kvm_arch_free_memslot().
>
> All that said, I'm not sure this aggressive fix is the right thing to send to
> stable@. For that, James' suggestion of hardening KVM's usage of
> __copy_{to,from}_user() seems like the best blend of being comprehensive without
> being overly invasive/risky.
This seems kind of nightmareish to backport; there are a lot of
copy_*_user() callsites that will need updating. Maybe I have a
different idea of the diff you're suggesting.
I wish we could just change the uaccess primitives, like
{,__}access_ok(), to check that `current->mm` is not NULL (and WARN
and return -EFAULT if it is NULL). That diff is also not trivial to
backport; many arch implementations would need updating. I have half a
mind to send an RFC patch to linux-mm@ to see what they think. :)
> So, as an immediate set of changes, what if we do this over ~5 patches, with patches
> 1 and 2 tagged for stable@?
>
> 1. Add kvm_copy_{to,from}_user{,_inatomic)() and return -EFAULT if current->mm
> is not kvm->mm.
SGTM. This is not mutually exclusive with the generic uaccess changes
I'm suggesting above. If this is actually reasonably backportable,
sure let's backport it.
> 2. Hack-a-fix PPC's kvm_arch_flush_shadow_all().
> 3. Do x86's vm_munmap() in kvm_arch_free_memslot().
> 4. Nuke memslots before calling kvm_arch_destroy_vm().
> 5. Change the current->mm checks in kvm_copy_{to,from}_user{,_inatomic)() to
> WARN_ON_ONCE() on failure.
>
> And then in the near-ish future, take things a step further and do:
>
> 6. Fix the vmx_leave_nested() trainwreck.
> 7. Harden the common kvm_{read,write}_guest family of APIs even further by
> adding an early WARN_ON_ONCE() on current->mm != kvm->mm, i.e. to detect
> bad KVM behavior as additional defense-in-depth.
This all SGTM, thanks Sean.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-10 18:58 ` James Houghton
@ 2026-09-10 19:14 ` Sean Christopherson
2026-09-10 19:32 ` Sean Christopherson
2026-09-10 19:40 ` Sean Christopherson
0 siblings, 2 replies; 14+ messages in thread
From: Sean Christopherson @ 2026-09-10 19:14 UTC (permalink / raw)
To: James Houghton; +Cc: Jim Mattson, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Thu, Sep 10, 2026, James Houghton wrote:
> On Wed, Sep 9, 2026 at 12:00 PM Sean Christopherson <seanjc@google.com> wrote:
> > If the above works for PPC, then KVM can nuke memslots before calling into
> > kvm_arch_destroy_vm(). x86's asinine memslot deletion in kvm_arch_destroy_vm()
> > needs to be addressed, but that code exists purely to do vm_munmap(), and can
> > and should be moved to kvm_arch_free_memslot().
> >
> > All that said, I'm not sure this aggressive fix is the right thing to send to
> > stable@. For that, James' suggestion of hardening KVM's usage of
> > __copy_{to,from}_user() seems like the best blend of being comprehensive without
> > being overly invasive/risky.
>
> This seems kind of nightmareish to backport; there are a lot of
> copy_*_user() callsites that will need updating. Maybe I have a
> different idea of the diff you're suggesting.
Nah, it's not many, because it's only the __copy_{to,from}_user{,inatomic}() usage
that needs handling. Everything else is strictly scoped to an ioctl, where (a)
current->mm can't be NULL and (b) KVM doesn't make any assumption about the address
space.
At a glance, it's 11 total: 5 in virt/kvm, 4 in vmx.c, and 2 in PPC's book3s_64_mmu_radix.c.
Well, plus 4 more to also harden {,__}kvm_{get,put}_guest().
And even if that number were doubled or tripled, the backports would still be
relatively easy. The overwhelming majority won't conflict, and the few that do
should be trivial to resolve (more than likely, simply drop the change).
> I wish we could just change the uaccess primitives, like
> {,__}access_ok(), to check that `current->mm` is not NULL (and WARN
> and return -EFAULT if it is NULL).
That wouldn't help at all in this case, because the access_ok() check is done
when memslots are modified. Which is the crux of KVM's problems: KVM decouples
the initial checks from the accesses, relying on kvm->mm to
And even if we hardened all of the uaccess helpers, we'd _still_ have problems,
because it's not just a NULL current->mm that's problematic. The last reference
to a VM file, i.e. to struct kvm, can be put by a different _process_. I.e. KVM
still needs to guard against reading/writing guest memory using a valid, non-NULL
current->mm that isn't kvm->mm. That can't be genericized in the uaccess APIs,
because the rule that only a specific address space can be used is very much unique
to KVM.
> That diff is also not trivial to backport; many arch implementations would
> need updating. I have half a mind to send an RFC patch to linux-mm@ to see
> what they think. :)
>
> > So, as an immediate set of changes, what if we do this over ~5 patches, with patches
> > 1 and 2 tagged for stable@?
> >
> > 1. Add kvm_copy_{to,from}_user{,_inatomic)() and return -EFAULT if current->mm
> > is not kvm->mm.
>
> SGTM. This is not mutually exclusive with the generic uaccess changes
> I'm suggesting above. If this is actually reasonably backportable,
> sure let's backport it.
>
> > 2. Hack-a-fix PPC's kvm_arch_flush_shadow_all().
> > 3. Do x86's vm_munmap() in kvm_arch_free_memslot().
> > 4. Nuke memslots before calling kvm_arch_destroy_vm().
> > 5. Change the current->mm checks in kvm_copy_{to,from}_user{,_inatomic)() to
> > WARN_ON_ONCE() on failure.
> >
> > And then in the near-ish future, take things a step further and do:
> >
> > 6. Fix the vmx_leave_nested() trainwreck.
> > 7. Harden the common kvm_{read,write}_guest family of APIs even further by
> > adding an early WARN_ON_ONCE() on current->mm != kvm->mm, i.e. to detect
> > bad KVM behavior as additional defense-in-depth.
>
> This all SGTM, thanks Sean.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-10 19:14 ` Sean Christopherson
@ 2026-09-10 19:32 ` Sean Christopherson
2026-09-10 19:40 ` Sean Christopherson
1 sibling, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2026-09-10 19:32 UTC (permalink / raw)
To: James Houghton; +Cc: Jim Mattson, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Thu, Sep 10, 2026, Sean Christopherson wrote:
> On Thu, Sep 10, 2026, James Houghton wrote:
> > On Wed, Sep 9, 2026 at 12:00 PM Sean Christopherson <seanjc@google.com> wrote:
> > I wish we could just change the uaccess primitives, like
> > {,__}access_ok(), to check that `current->mm` is not NULL (and WARN
> > and return -EFAULT if it is NULL).
>
> That wouldn't help at all in this case, because the access_ok() check is done
> when memslots are modified. Which is the crux of KVM's problems: KVM decouples
> the initial checks from the accesses, relying on kvm->mm to
>
> And even if we hardened all of the uaccess helpers, we'd _still_ have problems,
> because it's not just a NULL current->mm that's problematic. The last reference
> to a VM file, i.e. to struct kvm, can be put by a different _process_. I.e. KVM
> still needs to guard against reading/writing guest memory using a valid, non-NULL
> current->mm that isn't kvm->mm. That can't be genericized in the uaccess APIs,
> because the rule that only a specific address space can be used is very much unique
> to KVM.
Oh, and I'm not saying we shouldn't try to harden the uaccess APIs to guard against
a NULL current->mm. That absolutely would be worthwhile. I'm just saying that for
KVM, it sadly isn't sufficient.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-10 19:14 ` Sean Christopherson
2026-09-10 19:32 ` Sean Christopherson
@ 2026-09-10 19:40 ` Sean Christopherson
1 sibling, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2026-09-10 19:40 UTC (permalink / raw)
To: James Houghton; +Cc: Jim Mattson, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Thu, Sep 10, 2026, Sean Christopherson wrote:
> On Thu, Sep 10, 2026, James Houghton wrote:
> > On Wed, Sep 9, 2026 at 12:00 PM Sean Christopherson <seanjc@google.com> wrote:
> > > If the above works for PPC, then KVM can nuke memslots before calling into
> > > kvm_arch_destroy_vm(). x86's asinine memslot deletion in kvm_arch_destroy_vm()
> > > needs to be addressed, but that code exists purely to do vm_munmap(), and can
> > > and should be moved to kvm_arch_free_memslot().
> > >
> > > All that said, I'm not sure this aggressive fix is the right thing to send to
> > > stable@. For that, James' suggestion of hardening KVM's usage of
> > > __copy_{to,from}_user() seems like the best blend of being comprehensive without
> > > being overly invasive/risky.
> >
> > This seems kind of nightmareish to backport; there are a lot of
> > copy_*_user() callsites that will need updating. Maybe I have a
> > different idea of the diff you're suggesting.
>
> Nah, it's not many, because it's only the __copy_{to,from}_user{,inatomic}() usage
> that needs handling. Everything else is strictly scoped to an ioctl, where (a)
> current->mm can't be NULL and (b) KVM doesn't make any assumption about the address
> space.
>
> At a glance, it's 11 total: 5 in virt/kvm, 4 in vmx.c, and 2 in PPC's book3s_64_mmu_radix.c.
> Well, plus 4 more to also harden {,__}kvm_{get,put}_guest().
>
> And even if that number were doubled or tripled, the backports would still be
> relatively easy. The overwhelming majority won't conflict, and the few that do
> should be trivial to resolve (more than likely, simply drop the change).
To clarify: my goal isn't to harden literally every uaccess in KVM, just those
that have a dependency on memslots, i.e. are accessing guest memory. Those also
happen to be the ones that are often buried deep in KVM, in widely-used APIs.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-09 19:00 ` Sean Christopherson
2026-09-10 18:58 ` James Houghton
@ 2026-09-11 17:39 ` Jim Mattson
2026-09-11 18:10 ` Sean Christopherson
1 sibling, 1 reply; 14+ messages in thread
From: Jim Mattson @ 2026-09-11 17:39 UTC (permalink / raw)
To: Sean Christopherson
Cc: James Houghton, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Wed, Sep 9, 2026 at 12:00 PM Sean Christopherson <seanjc@google.com> wrote:
>
> So, as an immediate set of changes, what if we do this over ~5 patches, with patches
> 1 and 2 tagged for stable@?
By "we" here, you do mean the Royal "we," right?
I just want to confirm that you aren't expecting anything from me. :)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown
2026-09-11 17:39 ` Jim Mattson
@ 2026-09-11 18:10 ` Sean Christopherson
2026-09-11 19:10 ` [PATCH] KVM: selftests: Add test for shadow VMCS flush " Jim Mattson
0 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2026-09-11 18:10 UTC (permalink / raw)
To: Jim Mattson; +Cc: James Houghton, Paolo Bonzini, kvm, Yosry Ahmed, stable
On Fri, Sep 11, 2026, Jim Mattson wrote:
> On Wed, Sep 9, 2026 at 12:00 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > So, as an immediate set of changes, what if we do this over ~5 patches, with patches
> > 1 and 2 tagged for stable@?
>
> By "we" here, you do mean the Royal "we," right?
Correct.
> I just want to confirm that you aren't expecting anything from me. :)
Do you happen to have a selftest for this? Even if it's half-baked. I'll take
anything you have, at this point I just want a way to test the changes.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] KVM: selftests: Add test for shadow VMCS flush during vCPU teardown
2026-09-11 18:10 ` Sean Christopherson
@ 2026-09-11 19:10 ` Jim Mattson
2026-09-11 19:25 ` sashiko-bot
0 siblings, 1 reply; 14+ messages in thread
From: Jim Mattson @ 2026-09-11 19:10 UTC (permalink / raw)
To: Sean Christopherson
Cc: James Houghton, Paolo Bonzini, kvm, Yosry Ahmed, Jim Mattson
As requested, a half-baked selftest. :)
I dropped stable from the cc list.
When a vCPU is destroyed while L2 is active and VMCS shadowing is
enabled, KVM synthesizes a nested VM-Exit. This flushes the cached
shadow VMCS12 back to guest memory. However, on process exit, do_exit()
calls exit_mm() before closing file descriptors. KVM teardown runs
with current->mm == NULL on a borrowed lazy TLB active_mm.
Consequently, nested_flush_cached_shadow_vmcs12() writes the shadow
VMCS12 into whatever address space is active on that CPU.
Add a selftest to verify this behavior. The test runs a victim process
and a nested VMM process on the same physical CPU. The victim process
maps a page and fills it with canary bytes. The VMM sets up an L2 guest
with VMCS shadowing mapped at the identical host virtual address using
MAP_FIXED_NOREPLACE. When the VMM exits with open file descriptors, the
victim yields the CPU while the VMM terminates. This scheduling
heuristic attempts to hit the race window where VMM teardown runs under
the victim's active_mm and flushes the shadow VMCS into the victim's
address space.
---
tools/testing/selftests/kvm/Makefile.kvm | 1 +
.../kvm/x86/vmx_shadow_vmcs_teardown_test.c | 317 ++++++++++++++++++
2 files changed, 318 insertions(+)
create mode 100644 tools/testing/selftests/kvm/x86/vmx_shadow_vmcs_teardown_test.c
diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 5e2fcee198f2..2aa9cc75fbd7 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -143,6 +143,7 @@ TEST_GEN_PROGS_x86 += x86/vmx_exception_with_invalid_guest_state
TEST_GEN_PROGS_x86 += x86/vmx_msrs_test
TEST_GEN_PROGS_x86 += x86/vmx_invalid_nested_guest_state
TEST_GEN_PROGS_x86 += x86/vmx_nested_la57_state_test
+TEST_GEN_PROGS_x86 += x86/vmx_shadow_vmcs_teardown_test
TEST_GEN_PROGS_x86 += x86/apic_bus_clock_test
TEST_GEN_PROGS_x86 += x86/xapic_ipi_test
TEST_GEN_PROGS_x86 += x86/xapic_state_test
diff --git a/tools/testing/selftests/kvm/x86/vmx_shadow_vmcs_teardown_test.c b/tools/testing/selftests/kvm/x86/vmx_shadow_vmcs_teardown_test.c
new file mode 100644
index 000000000000..e0863ef20d3e
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86/vmx_shadow_vmcs_teardown_test.c
@@ -0,0 +1,317 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vmx_shadow_vmcs_teardown_test
+ *
+ * Verify that destroying a nested VMM with active shadow VMCS12 does not
+ * flush the cached shadow VMCS into an unrelated process memory space
+ * during process exit in lazy TLB mode.
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/wait.h>
+#include <linux/mman.h>
+#include <unistd.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+#include "vmx.h"
+#include "kselftest.h"
+
+#define PORT_L0_EXIT 0x2000
+#define SHADOW_VMCS_GPA 0x80000000ULL
+#define SHADOW_VMCS_GVA 0x80000000ULL
+#define CANARY_BYTE 0x5a
+#define L2_GUEST_STACK_SIZE 64
+
+struct test_result {
+ bool corrupted;
+ size_t corrupt_offset;
+ uint8_t corrupt_val;
+};
+
+static void l2_guest_code(void)
+{
+ /* Exit to L0 */
+ asm volatile("inb %%dx, %%al"
+ : : [port] "d" (PORT_L0_EXIT) : "rax");
+}
+
+static void l1_guest_code(struct vmx_pages *vmx_pages)
+{
+ unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
+
+ GUEST_ASSERT(prepare_for_vmx_operation(vmx_pages));
+ GUEST_ASSERT(load_vmcs(vmx_pages));
+
+ /* Prepare the VMCS for L2 execution. */
+ prepare_vmcs(vmx_pages, l2_guest_code,
+ &l2_guest_stack[L2_GUEST_STACK_SIZE]);
+
+ /* Enable VMCS shadowing and set the shadow VMCS link pointer. */
+ vmwrite(SECONDARY_VM_EXEC_CONTROL,
+ vmreadz(SECONDARY_VM_EXEC_CONTROL) | SECONDARY_EXEC_SHADOW_VMCS);
+ vmwrite(VMCS_LINK_POINTER, vmx_pages->shadow_vmcs_gpa);
+
+ vmlaunch();
+ GUEST_FAIL("L1 guest must not regain control");
+}
+
+static bool kvm_cpu_has_shadow_vmcs(void)
+{
+ uint64_t ctrl;
+
+ ctrl = kvm_get_feature_msr(MSR_IA32_VMX_TRUE_PROCBASED_CTLS) >> 32;
+ if (!(ctrl & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS))
+ return false;
+
+ ctrl = kvm_get_feature_msr(MSR_IA32_VMX_PROCBASED_CTLS2) >> 32;
+ return ctrl & SECONDARY_EXEC_SHADOW_VMCS;
+}
+
+static void run_vmm(int pcpu, void *target_hva, int c2_to_c1_fd)
+{
+ vm_vaddr_t vmx_pages_gva;
+ struct kvm_vcpu *vcpu;
+ struct kvm_vm *vm;
+ struct vmx_pages *vmx;
+ void *shadow_hva;
+
+ pin_self_to_cpu(pcpu);
+
+ vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
+
+ /* Allocate VMX pages and shared descriptors. */
+ vcpu_alloc_vmx(vm, &vmx_pages_gva);
+ vmx = addr_gva2hva(vm, vmx_pages_gva);
+
+ /*
+ * Map the shadow VMCS page at the exact target_hva allocated by the
+ * victim so it collides with the victim process's mapping.
+ */
+ shadow_hva = mmap(target_hva, PAGE_SIZE, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED_NOREPLACE, -1, 0);
+ TEST_ASSERT(shadow_hva == target_hva, "mmap target_hva failed in VMM");
+ memset(shadow_hva, 0, PAGE_SIZE);
+
+ /* Add a caller-managed memslot for the shadow VMCS backing page. */
+ vm_userspace_mem_region_add_caller_managed(vm, shadow_hva,
+ SHADOW_VMCS_GPA, 10, 1, 0);
+ virt_pg_map(vm, SHADOW_VMCS_GVA, SHADOW_VMCS_GPA);
+
+ /* Override the shadow VMCS pointers in vmx_pages. */
+ vmx->shadow_vmcs = (void *)SHADOW_VMCS_GVA;
+ vmx->shadow_vmcs_hva = shadow_hva;
+ vmx->shadow_vmcs_gpa = SHADOW_VMCS_GPA;
+
+ vcpu_args_set(vcpu, 1, vmx_pages_gva);
+
+ for (;;) {
+ struct kvm_run *run = vcpu->run;
+ struct ucall uc;
+
+ vcpu_run(vcpu);
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
+
+ if (run->io.port == PORT_L0_EXIT)
+ break;
+
+ switch (get_ucall(vcpu, &uc)) {
+ case UCALL_ABORT:
+ REPORT_GUEST_ASSERT(uc);
+ /* NOT REACHED */
+ default:
+ TEST_FAIL("Unknown ucall %lu", uc.cmd);
+ }
+ }
+
+ /*
+ * Signal the victim process that the VMM is about to terminate
+ * while L2 is active with VMCS shadowing enabled.
+ */
+ TEST_ASSERT(write(c2_to_c1_fd, "X", 1) == 1, "write to victim failed");
+
+ /*
+ * Exit immediately without closing VM or vCPU file descriptors.
+ * The kernel closes them in exit_files() after exit_mm() sets
+ * current->mm = NULL.
+ */
+ _exit(0);
+}
+
+static void run_victim(int pcpu, int ready_fd, int c2_to_c1_fd,
+ int p_to_c1_fd, int result_fd)
+{
+ struct test_result result = { .corrupted = false };
+ void *victim_hva;
+ char sync_byte;
+ size_t i;
+ int flags;
+
+ pin_self_to_cpu(pcpu);
+
+ victim_hva = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ TEST_ASSERT(victim_hva != MAP_FAILED, "mmap failed in victim");
+ memset(victim_hva, CANARY_BYTE, PAGE_SIZE);
+
+ /* Send mapped virtual address to parent so VMM can collide with it. */
+ TEST_ASSERT(write(ready_fd, &victim_hva, sizeof(victim_hva)) == sizeof(victim_hva),
+ "write ready failed");
+
+ /* Wait for VMM to launch L2 and reach exit to userspace. */
+ TEST_ASSERT(read(c2_to_c1_fd, &sync_byte, 1) == 1, "read sync failed");
+
+ /* Set parent-to-victim pipe to non-blocking. */
+ flags = fcntl(p_to_c1_fd, F_GETFL, 0);
+ TEST_ASSERT(flags >= 0, "fcntl F_GETFL failed");
+ TEST_ASSERT(fcntl(p_to_c1_fd, F_SETFL, flags | O_NONBLOCK) == 0,
+ "fcntl F_SETFL failed");
+
+ /*
+ * Loop yielding the CPU while the VMM terminates. When the VMM
+ * sleeps in synchronize_srcu() during teardown, the scheduler
+ * runs this victim process on the CPU. When the VMM wakes up,
+ * the scheduler context switches from this victim to the VMM
+ * with current->mm == NULL, leaving the victim's CR3 active.
+ */
+ for (;;) {
+ ssize_t ret = read(p_to_c1_fd, &sync_byte, 1);
+
+ if (ret == 1)
+ break;
+ if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) {
+ sched_yield();
+ continue;
+ }
+ TEST_FAIL("read from parent pipe failed: ret=%zd, errno=%d",
+ ret, errno);
+ }
+
+ /* Verify that the entire page remained untouched. */
+ for (i = 0; i < PAGE_SIZE; i++) {
+ if (((uint8_t *)victim_hva)[i] != CANARY_BYTE) {
+ result.corrupted = true;
+ result.corrupt_offset = i;
+ result.corrupt_val = ((uint8_t *)victim_hva)[i];
+ break;
+ }
+ }
+
+ TEST_ASSERT(write(result_fd, &result, sizeof(result)) == sizeof(result),
+ "write result failed");
+ _exit(0);
+}
+
+static bool run_iteration(int pcpu)
+{
+ int pipe_ready[2];
+ int pipe_c2_to_c1[2];
+ int pipe_p_to_c1[2];
+ int pipe_result[2];
+ pid_t pid_victim;
+ pid_t pid_vmm;
+ struct test_result result = {};
+ void *target_hva = NULL;
+ int status;
+
+ TEST_ASSERT(pipe(pipe_ready) == 0, "pipe failed");
+ TEST_ASSERT(pipe(pipe_c2_to_c1) == 0, "pipe failed");
+ TEST_ASSERT(pipe(pipe_p_to_c1) == 0, "pipe failed");
+ TEST_ASSERT(pipe(pipe_result) == 0, "pipe failed");
+
+ pid_victim = fork();
+ TEST_ASSERT(pid_victim >= 0, "fork victim failed");
+ if (pid_victim == 0) {
+ close(pipe_ready[0]);
+ close(pipe_c2_to_c1[1]);
+ close(pipe_p_to_c1[1]);
+ close(pipe_result[0]);
+ run_victim(pcpu, pipe_ready[1], pipe_c2_to_c1[0],
+ pipe_p_to_c1[0], pipe_result[1]);
+ }
+
+ close(pipe_ready[1]);
+ close(pipe_result[1]);
+ close(pipe_p_to_c1[0]);
+
+ /* Wait for victim to initialize its mapping and retrieve its address. */
+ TEST_ASSERT(read(pipe_ready[0], &target_hva, sizeof(target_hva)) == sizeof(target_hva),
+ "wait ready failed");
+ close(pipe_ready[0]);
+
+ pid_vmm = fork();
+ TEST_ASSERT(pid_vmm >= 0, "fork VMM failed");
+ if (pid_vmm == 0) {
+ close(pipe_c2_to_c1[0]);
+ close(pipe_p_to_c1[1]);
+ close(pipe_result[0]);
+ run_vmm(pcpu, target_hva, pipe_c2_to_c1[1]);
+ }
+
+ close(pipe_c2_to_c1[0]);
+ close(pipe_c2_to_c1[1]);
+
+ /* Wait for VMM to completely terminate. */
+ waitpid(pid_vmm, &status, 0);
+ TEST_ASSERT(WIFEXITED(status) && WEXITSTATUS(status) == 0,
+ "VMM child failed with status %d", status);
+
+ /* Signal victim to exit yield loop. */
+ TEST_ASSERT(write(pipe_p_to_c1[1], "D", 1) == 1, "signal victim failed");
+ close(pipe_p_to_c1[1]);
+
+ /* Read result from victim. */
+ TEST_ASSERT(read(pipe_result[0], &result, sizeof(result)) == sizeof(result),
+ "read result failed");
+ close(pipe_result[0]);
+
+ waitpid(pid_victim, &status, 0);
+ TEST_ASSERT(WIFEXITED(status) && WEXITSTATUS(status) == 0,
+ "Victim child failed with status %d", status);
+
+ if (result.corrupted)
+ pr_info("Memory check: corruption at offset 0x%zx: expected 0x%02x, actual 0x%02x\n",
+ result.corrupt_offset, CANARY_BYTE, result.corrupt_val);
+ else
+ pr_info("Memory check: entire page intact (canary = 0x%02x)\n",
+ CANARY_BYTE);
+
+ return result.corrupted;
+}
+
+int main(int argc, char *argv[])
+{
+ bool corrupted = false;
+ int pcpu;
+ int i;
+ const int iterations = 5;
+
+ TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX));
+ TEST_REQUIRE(kvm_cpu_has_shadow_vmcs());
+
+ pcpu = pin_self_to_any_cpu();
+
+ pr_info("Testing shadow VMCS teardown on pCPU %d (%d iterations)...\n",
+ pcpu, iterations);
+
+ for (i = 0; i < iterations; i++) {
+ if (run_iteration(pcpu)) {
+ corrupted = true;
+ break;
+ }
+ }
+
+ TEST_ASSERT(!corrupted,
+ "Memory corruption detected! Active MM was corrupted during vCPU teardown: canary byte 0x%02x overwritten\n",
+ CANARY_BYTE);
+
+ pr_info("PASS: No memory corruption detected during teardown.\n");
+ return 0;
+}
--
2.55.0.1032.g73a4cd73de-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: selftests: Add test for shadow VMCS flush during vCPU teardown
2026-09-11 19:10 ` [PATCH] KVM: selftests: Add test for shadow VMCS flush " Jim Mattson
@ 2026-09-11 19:25 ` sashiko-bot
0 siblings, 0 replies; 14+ messages in thread
From: sashiko-bot @ 2026-09-11 19:25 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Medium] Virtual address collision causes `mmap` with `MAP_FIXED_NOREPLACE` to fail, preventing the test from running.
- [Low] The test invokes an undefined function `vm_userspace_mem_region_add_caller_managed()`, causing a compilation error.
- [Low] The function `prepare_vmcs` is called with three arguments, but it is declared to take only two.
Pre-existing issues:
- [Critical] Cross-process memory corruption during nested VMM teardown due to guest memory write from lazy TLB address space.
--
commit 7a55753773b4f88412b975373e87f2ed41cc10a0
Author: Jim Mattson <jmattson@google.com>
KVM: selftests: Add test for shadow VMCS flush during vCPU teardown
This commit adds a selftest to verify KVM's behavior when a vCPU is
destroyed while L2 is active and VMCS shadowing is enabled. It attempts
to trigger a race condition where the nested VMM terminates and flushes
the shadow VMCS12 into a victim process's address space.
> When a vCPU is destroyed while L2 is active and VMCS shadowing is
> enabled, KVM synthesizes a nested VM-Exit. This flushes the cached
> shadow VMCS12 back to guest memory. However, on process exit, do_exit()
> calls exit_mm() before closing file descriptors. KVM teardown runs
> with current->mm == NULL on a borrowed lazy TLB active_mm.
> Consequently, nested_flush_cached_shadow_vmcs12() writes the shadow
> VMCS12 into whatever address space is active on that CPU.
[Severity: Critical]
This is a pre-existing issue, but this text describes a cross-process
memory corruption vulnerability during nested VMM teardown. When KVM
destroys a vCPU via kvm_arch_vcpu_destroy() calling vmx_leave_nested(),
it synthesizes a nested VM-exit. This flushes the shadow VMCS to guest
memory via kvm_write_guest_cached().
Because VMM teardown runs during do_exit() after exit_mm() sets
current->mm to NULL, kvm_write_guest_offset_cached() invokes
__copy_to_user() using the host virtual address on the borrowed active_mm
of whatever process previously ran on that CPU. If the HVA maps to valid
memory in the victim process, __copy_to_user() will silently overwrite it
with KVM's shadow VMCS data. Is there an upstream fix planned for this
underlying KVM vulnerability?
> diff --git a/tools/testing/selftests/kvm/x86/vmx_shadow_vmcs_teardown_test.c b/tools/testing/selftests/kvm/x86/vmx_shadow_vmcs_teardown_test.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/vmx_shadow_vmcs_teardown_test.c
[ ... ]
> +static void l1_guest_code(struct vmx_pages *vmx_pages)
> +{
> + unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
> +
> + GUEST_ASSERT(prepare_for_vmx_operation(vmx_pages));
> + GUEST_ASSERT(load_vmcs(vmx_pages));
> +
> + /* Prepare the VMCS for L2 execution. */
> + prepare_vmcs(vmx_pages, l2_guest_code,
> + &l2_guest_stack[L2_GUEST_STACK_SIZE]);
[Severity: Low]
Does this function call cause a build failure? The prepare_vmcs()
function is declared in the KVM selftests headers to take only two
arguments (struct vmx_pages *vmx, void *guest_rip), but it is being
called here with a third argument for the guest stack.
[ ... ]
> +static void run_vmm(int pcpu, void *target_hva, int c2_to_c1_fd)
> +{
> + vm_vaddr_t vmx_pages_gva;
> + struct kvm_vcpu *vcpu;
> + struct kvm_vm *vm;
> + struct vmx_pages *vmx;
> + void *shadow_hva;
> +
> + pin_self_to_cpu(pcpu);
> +
> + vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
> +
> + /* Allocate VMX pages and shared descriptors. */
> + vcpu_alloc_vmx(vm, &vmx_pages_gva);
> + vmx = addr_gva2hva(vm, vmx_pages_gva);
> +
> + /*
> + * Map the shadow VMCS page at the exact target_hva allocated by the
> + * victim so it collides with the victim process's mapping.
> + */
> + shadow_hva = mmap(target_hva, PAGE_SIZE, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED_NOREPLACE, -1, 0);
> + TEST_ASSERT(shadow_hva == target_hva, "mmap target_hva failed in VMM");
[Severity: Medium]
Will this mmap() call reliably fail with EEXIST and prevent the test from
running? Because the VMM and victim processes are forked from the same
parent and share identical ASLR bases, the victim's earlier mmap grabs
the highest available virtual address as target_hva.
When run_vmm() calls vm_create_with_one_vcpu() earlier in this function,
it makes its own large guest memory allocations using mmap(). Since mmap()
allocates top-down, the VMM's allocation is likely to overlap or envelop
target_hva before we reach this MAP_FIXED_NOREPLACE call.
> + memset(shadow_hva, 0, PAGE_SIZE);
> +
> + /* Add a caller-managed memslot for the shadow VMCS backing page. */
> + vm_userspace_mem_region_add_caller_managed(vm, shadow_hva,
> + SHADOW_VMCS_GPA, 10, 1, 0);
[Severity: Low]
Does this result in a compilation error? The function
vm_userspace_mem_region_add_caller_managed() does not appear to be defined
anywhere in the KVM selftests codebase.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911191011.528460-1-jmattson@google.com?part=1
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-09-11 19:25 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-08 13:28 [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown Jim Mattson
2026-09-08 13:57 ` sashiko-bot
2026-09-08 17:34 ` James Houghton
2026-09-08 19:12 ` Jim Mattson
2026-09-09 15:41 ` James Houghton
2026-09-09 19:00 ` Sean Christopherson
2026-09-10 18:58 ` James Houghton
2026-09-10 19:14 ` Sean Christopherson
2026-09-10 19:32 ` Sean Christopherson
2026-09-10 19:40 ` Sean Christopherson
2026-09-11 17:39 ` Jim Mattson
2026-09-11 18:10 ` Sean Christopherson
2026-09-11 19:10 ` [PATCH] KVM: selftests: Add test for shadow VMCS flush " Jim Mattson
2026-09-11 19:25 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox