Re: [PATCH 38/45] KVM: x86: mark VM as dead after KVM_SET_SREGS fails to load PDPTRs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: x86: __set_sregs_common() kills VM when PAE CR3 page is unmapped
--

commit 4ba7fe2bbdbb50dd511abb52c67718a74b2c9c1c
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86: mark VM as dead after KVM_SET_SREGS fails to load PDPTRs

This commit marks the VM as dead in __set_sregs_common() if load_pdptrs()
returns a negative error when setting PAE paging state, preventing the vCPU
from running with stale PDPTRs and an unreset MMU context.

> diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> index b7c25b16ffcaf..3a11a6d068897 100644
> --- a/arch/x86/kvm/regs.c
> +++ b/arch/x86/kvm/regs.c
> @@ -634,6 +634,7 @@ static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
>  			r = load_pdptrs(vcpu, kvm_read_cr3(vcpu));
>  			if (r < 0) {
>  				srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +				kvm_vm_dead(vcpu->kvm);
>  				return r;

[Severity: High]
Won't calling kvm_vm_dead() here terminate the VM whenever a PAE vCPU's CR3
points to an unmapped GPA during KVM_SET_SREGS (as well as KVM_SET_SREGS2
without KVM_SREGS2_FLAGS_PDPTRS_VALID, or KVM_RUN with KVM_SYNC_X86_SREGS)?

In arch/x86/kvm/regs.c, load_pdptrs() reads the guest page with
kvm_vcpu_read_guest_page():

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));
	if (ret == -EFAULT) {
		kvm_prepare_memory_fault_exit(vcpu, real_gpa & PAGE_MASK, PAGE_SIZE,
					      false, false, false);
		return ret;
	}
	if (ret)
		return 0;
	...
}

Because kvm_vcpu_read_guest_page() -> __kvm_read_guest_page() returns 0 or
-EFAULT, it returns -EFAULT not only for guest_memfd memory attribute faults,
but also whenever the GFN has no memslot (for example, when userspace calls
KVM_SET_SREGS before populating memslots via KVM_SET_USER_MEMORY_REGION, or
on SVM with NPT when the guest loaded an unmapped GPA into CR3).

This also triggers during state restore of a nested PAE L2 guest when
userspace calls KVM_SET_SREGS before KVM_SET_NESTED_STATE (such as in
vcpu_load_state() in KVM selftests). At KVM_SET_SREGS time, is_guest_mode()
is still false, so load_pdptrs() treats L2's CR3 as an L1 GPA without nested
NPT/EPT translation; if that GPA does not map to a readable L1 memslot,
load_pdptrs() returns -EFAULT.

In those cases, returning early and calling kvm_vm_dead() leaves segment
registers and CR8 unloaded, skips the MMU reset, overwrites vcpu->run with
KVM_EXIT_MEMORY_FAULT outside KVM_RUN, and permanently kills the VM. Should
__set_sregs_common() ignore -EFAULT from load_pdptrs() (or avoid preparing a
memory fault exit outside KVM_RUN) and continue loading the remaining sregs
and resetting the MMU as before?

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