From: Sean Christopherson When doing RSM to L2, validate L1's state after copying it back into vmcb01 from SMRAM, as SMRAM is carved out of guest memory, i.e. is writable by the guest. Failure to validate L1's state is especially problematic for inputs to MMU context, as KVM can end up with impossible CR4 vs. EFER state. E.g. without the recent hardening FNAME(walk_addr_generic), a guest can trigger: ------------[ cut here ]------------ UBSAN: array-index-out-of-bounds in arch/x86/kvm/mmu/paging_tmpl.h:435:15 index 3 is out of range for type 'u32 [2]' CPU: 3 UID: 0 PID: 92 Comm: CPU 1/KVM Not tainted 7.3.0-rc1 #2 PREEMPT(lazy) Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 Call Trace: dump_stack_lvl+0x5f/0x90 dump_stack+0x10/0x18 ubsan_epilogue+0x9/0x39 __ubsan_handle_out_of_bounds.cold+0x50/0x55 paging32_walk_addr_generic+0x889/0x970 paging32_page_fault+0x79/0xa60 kvm_mmu_do_page_fault+0x125/0x270 kvm_mmu_page_fault+0x88/0x7b0 npf_interception+0xc8/0x220 [kvm_amd] svm_invoke_exit_handler+0x16d/0x1d0 [kvm_amd] svm_handle_exit+0xb7/0x160 [kvm_amd] vcpu_enter_guest+0x428/0x16a0 kvm_arch_vcpu_ioctl_run+0x479/0x920 kvm_vcpu_ioctl+0x316/0xb00 __x64_sys_ioctl+0xa3/0x100 x64_sys_call+0x281/0x23d0 do_syscall_64+0xba/0x550 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x79bb30deb04b ---[ end trace ]--- Fixes: 37be407b2ce8 ("KVM: nSVM: Fix L1 state corruption upon return from SMM") Cc: stable@vger.kernel.org Reported-by: Bill Wendling Reported-by: Stefan Teodorescu Tested-by: Stefan Teodorescu Signed-off-by: Sean Christopherson Message-ID: <20261022021046.295001-1-seanjc@google.com> Signed-off-by: Paolo Bonzini --- arch/x86/kvm/svm/nested.c | 23 ++++++++++++++++------- arch/x86/kvm/svm/svm.c | 2 ++ arch/x86/kvm/svm/svm.h | 2 ++ 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index af0ef678433d..07554089ec05 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -1889,6 +1889,21 @@ static void nested_copy_vmcb_cache_to_control(struct vmcb_control_area *dst, /* 'clean' and 'hv_enlightenments' are not changed by KVM */ } +bool nested_svm_check_vmcb01_save(struct kvm_vcpu *vcpu, + struct vmcb_save_area *save) +{ + struct vmcb_save_area_cached save_cached; + + if (!(save->cr0 & X86_CR0_PG) || + !(save->cr0 & X86_CR0_PE) || + (save->rflags & X86_EFLAGS_VM) || + ((save->efer & EFER_LMA) && !(save->efer & EFER_LME))) + return false; + + __nested_copy_vmcb_save_to_cache(&save_cached, save); + return nested_vmcb_check_save(vcpu, &save_cached, false); +} + static int svm_get_nested_state(struct kvm_vcpu *vcpu, struct kvm_nested_state __user *user_kvm_nested_state, u32 user_data_size) @@ -1968,7 +1983,6 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, &user_kvm_nested_state->data.svm[0]; struct vmcb_control_area *ctl; struct vmcb_save_area *save; - struct vmcb_save_area_cached save_cached; struct vmcb_ctrl_area_cached ctl_cached; bool use_separate_l2_pat; unsigned long cr0; @@ -2040,12 +2054,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, * KVM_GET_NESTED_STATE, and (b) it is not loaded at emulated * #VMEXIT. */ - __nested_copy_vmcb_save_to_cache(&save_cached, save); - if (!(save->cr0 & X86_CR0_PG) || - !(save->cr0 & X86_CR0_PE) || - (save->rflags & X86_EFLAGS_VM) || - ((save->efer & EFER_LMA) && !(save->efer & EFER_LME)) || - !nested_vmcb_check_save(vcpu, &save_cached, false)) + if (!nested_svm_check_vmcb01_save(vcpu, save)) goto out_free; /* diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 487897ef11dd..581bc3a23c95 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -5219,6 +5219,8 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram) */ svm_copy_vmrun_state(vcpu, &svm->vmcb01.ptr->save, m_save.map.hva + 0x400); + if (!nested_svm_check_vmcb01_save(vcpu, &svm->vmcb01.ptr->save)) + return 1; /* * Enter the nested guest now diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 4b0b2ee13a3a..b9f7786bf17c 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -911,6 +911,8 @@ void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm, void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm, struct vmcb_save_area *save); void nested_sync_control_from_vmcb02(struct vcpu_svm *svm); +bool nested_svm_check_vmcb01_save(struct kvm_vcpu *vcpu, + struct vmcb_save_area *save); void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb); -- 2.52.0 From: Sean Christopherson Conditionally reschedule when retrying a guest PTE walk because the CMPXCHG to update A/D bits failed, e.g. because a different task/vCPU modified the relevant gPTEs. On kernels without involuntary preemption of some kind, a misbehaving guest can "capture" a pCPU effectively indefinitely and trigger soft lockups by continuously modifying its gPTEs such that the result of the walk is remains valid. Fixes: b3e4e63fd9ee ("KVM: MMU: Use cmpxchg for pte updates on walk_addr()") Cc: stable@vger.kernel.org Reported-by: Stefan Teodorescu Signed-off-by: Sean Christopherson Message-ID: <20261022015903.282261-1-seanjc@google.com> Signed-off-by: Paolo Bonzini --- arch/x86/kvm/mmu/paging_tmpl.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index 8e350095508c..18f7eb633127 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -340,6 +340,13 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, gpa_t real_gpa; gfn_t gfn; + /* + * KVM might sleep/resched when retrying the walk if the updating A/D + * fails because the gPTE changed between reading the gPTE and trying + * the CMPXHG to set A/D bits. + */ + might_sleep(); + trace_kvm_mmu_pagetable_walk(addr, access); retry_walk: walker->level = w->cpu_role.base.level; @@ -482,8 +489,10 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, addr, write_fault); if (unlikely(ret < 0)) goto error; - else if (ret) + else if (ret) { + cond_resched(); goto retry_walk; + } } return 1; -- 2.52.0 From: Sean Christopherson When restarting emulation, e.g. of a REP-prefixed string instruction, conditionally reschedule as emulating guest writes can be deceptively slow and expensive, e.g. when shadow paging is in use and each write can trigger write tracking. In a perfect world, restarting emulation (or not) would be less arbitrary, e.g. would honor not only host events (NEED_RESCHED, signals, etc.) but also guest events (e.g. by checking kvm_vcpu_has_events()). Unfortunately, as pointed out by Sashiko, pio_in_emulated()'s read-ahead logic is baked into the restart logic, as KVM relies on restarting emulation to drain the cache and avoid duplicate reads from the I/O port. I.e. re-entering the guest on kvm_vcpu_has_events() or __xfer_to_guest_mode_work_pending() would introduce architecturally-incorrect, guest-visible changes. Fixes: 5cd21917da24 ("KVM: x86 emulator: restart string instruction without going back to a guest.") Cc: stable@vger.kernel.org Cc: David Woodhouse Reported-by: Stefan Teodorescu Suggested-by: Stefan Teodorescu Signed-off-by: Sean Christopherson Message-ID: <20261028173638.1765329-1-seanjc@google.com> Signed-off-by: Paolo Bonzini --- arch/x86/kvm/x86.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 32563a91fba9..c4a06214cde6 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6558,10 +6558,12 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, } else if (vcpu->arch.complete_userspace_io) { writeback = false; r = 0; - } else if (r == EMULATION_RESTART) + } else if (r == EMULATION_RESTART) { + cond_resched(); goto restart; - else + } else { r = 1; + } writeback: if (writeback) { -- 2.52.0 From: Sean Christopherson Add a missing "break" in the inner for-loop of the open coded mess that unwinds MSRs back to the pre-VM-Enter state when VMLAUNCH/VMRESUME VM-Fails due to a consistency check that is caught by hardware but missed by KVM. I.e. move on to the next MSR in the VM-Entry load list after performing a write using a value found in the VM-Exit load list. Processing *every* matching entry in the VM-Exit load list makes the processing quadratic, which is problematic if the load lists are full of MSRs that are expensive to emulate. E.g. writing APIC_BASE triggers kvm_recalculate_apic_map() and a whole pile of unnecessary work. Functionally, arbitrarily restoring the value of the first matching entry is "fine", because none of this is architecturally correct: the load lists are never supposed to have been used in the first place. In practice, it's likely a moot point as no well-behaved VMM will have multiple, diverging entries in the load lists. Fixes: bd18bffca353 ("KVM: nVMX: restore host state in nested_vmx_vmexit for VMFail") Cc: stable@vger.kernel.org Cc: David Woodhouse Reported-by: Stefan Teodorescu Suggested-by: Stefan Teodorescu Signed-off-by: Sean Christopherson Message-ID: <20261027185917.1478906-1-seanjc@google.com> Signed-off-by: Paolo Bonzini --- arch/x86/kvm/vmx/nested.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 9b0bfa2f854c..f0d75f5a968c 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -5057,6 +5057,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) __func__, j, h.index, h.value); goto vmabort; } + break; } } -- 2.52.0 From: Sean Christopherson When potentially reusing a shadow page, zap the shadow page if its number of parent PTEs exceeds KVM's (mostly) arbitrary rmap recycling threshold. Letting the guest create a (mostly) unbounded number of parent PTEs allows the guest to coerce KVM into creating lists containing millions of PTEs, effectively bounded only by the maximum number of shadow pages. Because PTE lists are simple singly linked lists, removal is O(n). Even worse, if a list is constructed just so, teardown can be O(n^2), e.g. if SPTEs are built such that zapping child SPTEs always zaps the last entry in the list. To cap things off, all paths that walk PTE lists do so while holding mmu_lock, without conditionally rescheduling. Using a doubly linked list would not address the issue, as removal would remain O(n) since KVM still wouldn't know where in the list to remove from. Removal could be made O(1) by storing a per-SPTE pointer, but that would largely defeat the purpose of reusing SPs, and would penalize well-behaved guests, especially for nested TDP where aliasing page tables is uncommon. So, cap the maximum number of parent PTEs at the rmap limit. In practice, well-behaved guests will likely never run afoul of the limit. E.g. when running Linux as L1 with TDP, the number of parent PTEs for a single SP tops out at under 200 (though undoubtedly obscure workloads can push it higher). Deliberately use a "greater than or equal" as opposed to rmap's strictly "greater than", as the rmap code checks the threshold immediately *after* inserting into the list, i.e. the limit is 1000 for rmaps, KVM just very briefly blows past it. Apply the check to both indirect and direct SPs, because while a fully direct MMU naturally limits the number of parent PTEs to '1', direct SPs in indirect MMUs have no such constraint. E.g. if the guest maps a huge PTE into many locations in its page tables, and KVM is backing the hugepage with a direct SP, i.e. with a series of smaller pages, then KVM could end up with a direct SP with many parents. Note, shadow_mmu_try_split_huge_pages() always restarts the rmap walk or bails entirely if it gets to the point where it tries to grab a SP, i.e. there is *no* pre-existing bug that would be tripped by zapping a SP and thus rmaps when getting a SP. Cc: Jim Mattson Cc: James Houghton Cc: Yosry Ahmed Cc: David Woodhouse Cc: stable@vger.kernel.org Reported-by: Stefan Teodorescu Signed-off-by: Sean Christopherson Message-ID: <20261027171518.1432557-1-seanjc@google.com> Signed-off-by: Paolo Bonzini --- arch/x86/kvm/mmu/mmu.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 5a2cc6a17a9b..f4b12d6aef55 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2355,6 +2355,16 @@ static struct kvm_mmu_page *kvm_mmu_find_shadow_page(struct kvm *kvm, continue; } + /* + * Note! Unlike fully direct MMUs, direct SPs in indirect MMUs + * can have an arbitrary number of parents, e.g if the guest is + * mapping a huge PTE at many points in its page tables. + */ + if (pte_list_count(&sp->parent_ptes) >= RMAP_RECYCLE_THRESHOLD) { + kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); + break; + } + /* unsync and write-flooding only apply to indirect SPs. */ if (sp->role.direct) goto out; -- 2.52.0 From: Sean Christopherson Force an MMU reset when emulating an INIT even if paging was already disabled, as the MMU page role includes guest_mode (L2), i.e. an INIT while L2 is running with CR0.PG=0 will result in KVM running L1 with a stale MMU context. Alternatively, the reset could be scoped strictly to paging being enabled or L2 being active, but SMM is also encoded in the page role. Even though it _should_ be impossible for the vCPU to be INIT'd while SMM is active, there's zero value in not handling SMM. At that point, the only scenario where KVM _doesn't_ need to reset the MMU context is on an INIT with the vCPU running in Real Mode or Unpaged Protect Mode, and while INIT in those modes is somewhat common, it's far from a hot path and re-initializing the MMU is not expensive, especially when the role isn't changing (KVM elides most of the re-configuration in that case). Note, like the Fixes commit, this bug only affects nested SVM as INIT is an unconditional intercept on VMX, i.e. it's impossible to emulate INIT while L2 is active. Temporarily keep the guest TLB flush when CR0.PG is being cleared even though it is fully redundant with the flush that is emitted on all INIT events (KVM itself only does RESET when a vCPU is being created, i.e. when CR0.PG must be '0'). Fixes: ed129ec9057f ("KVM: x86: forcibly leave nested mode on vCPU reset") Reported-by: Stefan Teodorescu Reported-by: Oren Yomtov Reported-by: Chris Mason Cc: stable@vger.kernel.org Cc: Yosry Ahmed Signed-off-by: Sean Christopherson Message-ID: <20261022015407.279870-2-seanjc@google.com> Signed-off-by: Paolo Bonzini --- arch/x86/kvm/x86.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c4a06214cde6..09c6df00ebb6 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9731,18 +9731,23 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) kvm_x86_call(set_efer)(vcpu, 0); kvm_x86_call(update_exception_bitmap)(vcpu); - /* - * On the standard CR0/CR4/EFER modification paths, there are several - * complex conditions determining whether the MMU has to be reset and/or - * which PCIDs have to be flushed. However, CR0.WP and the paging-related - * bits in CR4 and EFER are irrelevant if CR0.PG was '0'; and a reset+flush - * is needed anyway if CR0.PG was '1' (which can only happen for INIT, as - * CR0 will be '0' prior to RESET). So we only need to check CR0.PG here. - */ - if (old_cr0 & X86_CR0_PG) { + if (old_cr0 & X86_CR0_PG) kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu); - kvm_mmu_reset_context(vcpu); - } + + /* + * Unconditionally reset the MMU, even though super strictly speaking, + * a MMU reset is only necessary if and only one of the page role bits + * is changing. All of the CR0/CR4/EFER bits effectively get covered + * by checking CR0.PG=1, as CR0.WP and the paging-related bits in CR4 + * and EFER are irrelevant if CR0.PG was '0'. Subtly, if the vCPU was + * in guest mode (L2) or SMM (although emulating INIT in SMM _should_ + * be impossible), an MMU reset is also necessary as "guest mode" and + * "smm" are both encoded in the shadow page role. As a result, the + * only scenarios where an MMU reset is *not* needed is if the vCPU is + * in Real Mode or Unpaged Protected Mode, and micro-optimizing those + * paths are not worth the risk of running with a stale MMU context. + */ + kvm_mmu_reset_context(vcpu); /* * Intel's SDM states that all TLB entries are flushed on INIT. AMD's -- 2.52.0 From: Sean Christopherson Flush the guest TLB when emulating a RESET even though such a flush should be completely unnecessary since KVM only performs a RESET when creating a vCPU, and so it should be impossible for the vCPU to have any accrued TLB state. Making the flush unconditional aligns KVM's behavior with both the SDM and APM (ignoring that both specifications contradict themselves), and avoids having to worry about hypotheticals like "what if KVM did provide a way to emulate RESET after a vCPU is created?". Signed-off-by: Sean Christopherson Message-ID: <20261022015407.279870-3-seanjc@google.com> Signed-off-by: Paolo Bonzini --- arch/x86/kvm/x86.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 09c6df00ebb6..26e7beaff8a0 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9731,9 +9731,6 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) kvm_x86_call(set_efer)(vcpu, 0); kvm_x86_call(update_exception_bitmap)(vcpu); - if (old_cr0 & X86_CR0_PG) - kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu); - /* * Unconditionally reset the MMU, even though super strictly speaking, * a MMU reset is only necessary if and only one of the page role bits @@ -9750,16 +9747,16 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) kvm_mmu_reset_context(vcpu); /* - * Intel's SDM states that all TLB entries are flushed on INIT. AMD's - * APM states the TLBs are untouched by INIT, but it also states that - * the TLBs are flushed on "External initialization of the processor." - * Flush the guest TLB regardless of vendor, there is no meaningful - * benefit in relying on the guest to flush the TLB immediately after - * INIT. A spurious TLB flush is benign and likely negligible from a - * performance perspective. + * Intel's SDM states that all TLB entries are flushed on INIT "as with + * a hardware reset", and AMD's APM states the TLBs are flushed on + * "External initialization of the processor". Flush the guest TLB for + * both RESET and INIT, regardless of vendor, even though both the SDM + * and APM contradict the above statements in other tables. There is + * no meaningful benefit in relying on the guest to flush the TLB, and + * a spurious TLB flush is benign and likely negligible from a + * performance perspective (especially for RESET). */ - if (init_event) - kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu); + kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu); } EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_vcpu_reset); -- 2.52.0