Re: [PATCH 34/45] KVM: x86/mmu: Adjust gva_to_gpa to allow returning an error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: x86: mismatch between KVM_TRANSLATE docs and -EFAULT handling
- [Medium] KVM: x86: unconditional gva_to_gpa() walk in kvm_fixup_and_inject_pf_error()
--

commit 914d25167c243b05b87b4e07ce157df3731ce832
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86/mmu: Adjust gva_to_gpa to allow returning an error

This commit changes the gva_to_gpa callback and its helper wrappers to return
an int error code while writing the translated guest physical address to an
out parameter, allowing callers to distinguish -EFAULT from INVALID_GPA.

> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index ac8cd51bdadae..743a738ff8d39 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -521,7 +521,9 @@ data structures.
>  :Architectures: x86
>  :Type: vcpu ioctl
>  :Parameters: struct kvm_translation (in/out)
> -:Returns: 0 on success, -1 on error
> +:Returns: 0 on success, -1 on error.  errno is EFAULT if
> +          the parameter is invalid but also if guest memory
> +          could not be accessed
>  
>  Translates a virtual address according to the vcpu's current address
>  translation mode.

[ ... ]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bb36ca1106e58..53400fda9f44b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -9341,19 +9354,24 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
>  {
>  	unsigned long vaddr = tr->linear_address;
>  	gpa_t gpa;
> -	int idx;
> +	int idx, r;
>  
>  	vcpu_load(vcpu);
> -
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
> -	gpa = kvm_mmu_gva_to_gpa_system(vcpu, vaddr, NULL);
> +	r = kvm_mmu_gva_to_gpa_system(vcpu, vaddr, &gpa, NULL);
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +	vcpu_put(vcpu);
> +
> +	if (r) {
> +		if (r != -EFAULT)
> +			return r;
> +		gpa = INVALID_GPA;
> +	}
> +
>  	tr->physical_address = gpa;
>  	tr->valid = gpa != INVALID_GPA;
>  	tr->writeable = 1;
>  	tr->usermode = 0;
> -
> -	vcpu_put(vcpu);
>  	return 0;
>  }

[Severity: Medium]
Documentation/virt/kvm/api.rst is updated above to state that KVM_TRANSLATE
returns -1 with errno set to EFAULT when guest memory could not be accessed.

However, in kvm_arch_vcpu_ioctl_translate(), when kvm_mmu_gva_to_gpa_system()
returns -EFAULT, the error is caught and converted into gpa = INVALID_GPA,
causing the ioctl to return 0 with tr->valid = 0 instead of returning -EFAULT.

Should kvm_arch_vcpu_ioctl_translate() propagate -EFAULT to userspace, or was
the intention to report an invalid translation (tr->valid = 0) and the
documentation in api.rst needs to be updated to match?

> @@ -10815,11 +10833,15 @@ int kvm_fixup_and_inject_pf_error(struct kvm_vcpu *vcpu, gva_t gva, u16 error_co
>  {
>  	struct kvm_pagewalk *gva_walk = &vcpu->arch.gva_walk;
>  	struct x86_exception fault;
> +	gpa_t gpa;
> +	int r;
>  	u64 access = error_code &
>  		(PFERR_WRITE_MASK | PFERR_FETCH_MASK | PFERR_USER_MASK);
>  
> -	if (!(error_code & PFERR_PRESENT_MASK) ||
> -	    gva_walk->gva_to_gpa(vcpu, gva_walk, gva, access, &fault) != INVALID_GPA) {
> +	r = gva_walk->gva_to_gpa(vcpu, gva_walk, gva, access, &gpa, &fault);
> +	if (r)
> +		return r;
> +	if (!(error_code & PFERR_PRESENT_MASK) || gpa != INVALID_GPA) {
>  		/*
>  		 * If gva_walk->gva_to_gpa succeeded, the page
>  		 * tables probably do not match the TLB.  Just proceed

[Severity: Medium]
Previously, kvm_fixup_and_inject_pf_error() short-circuited and skipped
calling gva_walk->gva_to_gpa() whenever !(error_code & PFERR_PRESENT_MASK) was
true.

With gva_walk->gva_to_gpa() hoisted before the PFERR_PRESENT_MASK check, every
non-present page fault reaching kvm_fixup_and_inject_pf_error() (for instance,
in nested VMX when vmx_need_pf_intercept() causes prepare_vmcs02_rare() to
intercept all L2 page faults) now walks the guest and nested page tables even
though both gpa and fault are overwritten in the if-block when
PFERR_PRESENT_MASK is clear.

Aside from the extra page-table walk, if a guest PTE was concurrently made
present or upper-level guest PTEs are walked, FNAME(walk_addr_generic)() can
update guest Accessed/Dirty bits or return -EFAULT on a non-present fault.

Should gva_walk->gva_to_gpa() only be called when (error_code &
PFERR_PRESENT_MASK) is set?

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