Re: [PATCH 36/45] KVM: x86: Propagate memory faults through load_pdptrs()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: x86: unconditional memory fault exit on read failure in load_pdptrs()
- [High] KVM: SVM: svm_cache_reg() kills VM when load_pdptrs() returns negative errno
--

commit 780e6889d183774ef90a40deaa0094e70e71d6a2
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86: Propagate memory faults through load_pdptrs()

This commit changes load_pdptrs() and its callers to propagate negative
errno values so memory attribute faults can be distinguished from other
PDPTR load failures.

> diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> index 3969b769f1c19..b7c25b16ffcaf 100644
> --- a/arch/x86/kvm/regs.c
> +++ b/arch/x86/kvm/regs.c
> @@ -176,7 +176,12 @@ int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3)
>  	/* Note the offset, PDPTRs are 32 byte aligned when using PAE paging. */
>  	ret = kvm_vcpu_read_guest_page(vcpu, gpa_to_gfn(real_gpa), pdpte,
>  				       cr3 & GENMASK(11, 5), sizeof(pdpte));
> -	if (ret < 0)
> +	if (ret == -EFAULT) {
> +		kvm_prepare_memory_fault_exit(vcpu, real_gpa & PAGE_MASK, PAGE_SIZE,
> +					      false, false, false);
> +		return ret;
> +	}
> +	if (ret)
>  		return 0;

[Severity: High]
Does load_pdptrs() misclassify every read failure from
kvm_vcpu_read_guest_page() as a memory attribute fault?

Looking at virt/kvm/kvm_main.c, kvm_vcpu_read_guest_page() calls
__kvm_read_guest_page(), which only ever returns 0 or -EFAULT:

virt/kvm/kvm_main.c:__kvm_read_guest_page() {
	addr = gfn_to_hva_memslot_prot(slot, gfn, NULL);
	if (kvm_is_error_hva(addr))
		return -EFAULT;

	attrs = kvm_get_vm_memory_attributes(kvm, gfn);
	if (attrs & attr)
		return -EFAULT;

	r = __copy_from_user(data, (void __user *)addr + offset, len);
	if (r)
		return -EFAULT;
	return 0;
}

Because __kvm_read_guest_page() returns -EFAULT for all failure cases
(including when real_gpa is unmapped and has no backing memslot), the
following check in load_pdptrs():

	if (ret)
		return 0;

becomes unreachable dead code, and any unmapped PAE CR3 GPA prepares a
KVM_EXIT_MEMORY_FAULT and returns -EFAULT instead of 0.

Doesn't this cause kvm_set_cr0(), kvm_set_cr3(), and kvm_set_cr4() to exit
to userspace with KVM_EXIT_MEMORY_FAULT instead of returning 1 to inject
#GP(0) when a PAE guest loads an unmapped GPA into CR3, and cause
__set_sregs_common() (via KVM_SET_SREGS or KVM_SET_SREGS2 without
KVM_SREGS2_FLAGS_PDPTRS_VALID) to fail with -EFAULT when CR3 is not backed
by a memslot?

Should load_pdptrs() check kvm_prepare_memory_fault_exit_for_range() (or
!kvm_mem_attributes_may_read_gfn()) before preparing KVM_EXIT_MEMORY_FAULT
and returning -EFAULT, similar to kvm_read_guest_virt_helper() and
emulator_read_write_onepage()?

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 669bf8b36bb63..fd4df88576c28 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1591,8 +1591,8 @@ static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
>  		 * When !npt_enabled, vcpu->pdptrs[] is already available since
>  		 * it is always updated per SDM when moving to CRs.
>  		 */
> -		if (npt_enabled)
> -			load_pdptrs(vcpu, kvm_read_cr3(vcpu));
> +		if (npt_enabled && load_pdptrs(vcpu, kvm_read_cr3(vcpu)) < 0)
> +			kvm_vm_dead(vcpu->kvm);
>  		break;
>  	default:
>  		KVM_BUG_ON(1, vcpu->kvm);

[Severity: High]
Can an L1 or L2 guest in PAE paging mode trigger kvm_vm_dead() here on AMD
SVM with NPT enabled?

When npt_enabled is true, init_vmcb() clears INTERCEPT_CR3_WRITE, so guest
writes to CR3 are not intercepted. On VM-exit, svm_vcpu_run() copies
svm->vmcb->save.cr3 to vcpu->arch.cr3 and clears VCPU_REG_PDPTR from
vcpu->arch.regs_avail (and on nested VMRUN with nested NPT enabled,
nested_svm_load_cr3() sets vcpu->arch.cr3 and calls
kvm_register_mark_for_reload(vcpu, VCPU_REG_PDPTR)).

If the guest sets CR3 in PAE mode to an unmapped GPA or a GPA with
KVM_MEMORY_ATTRIBUTE_NR (or if a nested GPA walk in kvm_translate_gpa()
hits a page with KVM_MEMORY_ATTRIBUTE_NR) and then triggers a KVM page walk
or userspace calls KVM_GET_SREGS2:

kvm_vcpu_ioctl_x86_get_sregs2() / walk_addr_generic()
  -> kvm_pdptr_read()
    -> svm_cache_reg(vcpu, VCPU_REG_PDPTR)
      -> load_pdptrs()

won't load_pdptrs() return -EFAULT and cause svm_cache_reg() to terminate
the VM via kvm_vm_dead() instead of treating an unmapped GPA as invalid
PDPTRs or propagating KVM_EXIT_MEMORY_FAULT to userspace?

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