Re: [PATCH 26/45] KVM: pfncache: Fail to refresh if it contains memory protections

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: pfncache: __kvm_gpc_refresh() skips updating gpc->gpa on RW protection error
--

commit 808264658bd3c3bcb182dcf95cb531c1ff5d0f1b
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: pfncache: Fail to refresh if it contains memory protections

This commit updates __kvm_gpc_refresh() in virt/kvm/pfncache.c to reject
refreshing GPA-based caches when the target GFN has NR or NW memory
protection attributes.

Link: https://patch.msgid.link/20260918135030.171564-24-pbonzini@redhat.com

> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> index e82247ca46372..ed8884a6d2404 100644
> --- a/virt/kvm/pfncache.c
> +++ b/virt/kvm/pfncache.c
> @@ -298,7 +298,18 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
>  		    gpc->gpa != gpa || kvm_is_error_hva(gpc->uhva)) {
>  			gfn_t gfn = gpa_to_gfn(gpa);
>  
> +			/*
> +			 * RW memory attributes are incompatible with GPC. The
> +			 * RW protection check has to happen after storing the
> +			 * generation number.
> +			 */
>  			gpc->attrs_generation = kvm_mem_attributes_generation(kvm);
> +			if (kvm_range_has_rw_memory_protections(kvm, gfn, gfn + 1)) {
> +				gpc->uhva = KVM_HVA_ERR_BAD;
> +				ret = -EFAULT;
> +				goto out;
> +			}
> +
>  			gpc->gpa = gpa;
>  			gpc->slots_generation = slots->generation;
>  			gpc->memslot = __gfn_to_memslot(slots, gfn);

[Severity: High]
Because this error path jumps to out before setting gpc->gpa = gpa, what
happens when kvm_gpc_activate() is called on a GPA backed by NR or NW memory
protections?

In virt/kvm/pfncache.c, __kvm_gpc_activate() marks the cache active before
calling __kvm_gpc_refresh():

virt/kvm/pfncache.c:__kvm_gpc_activate() {
	if (!gpc->active) {
		...
		write_lock_irq(&gpc->lock);
		gpc->active = true;
		write_unlock_irq(&gpc->lock);
	}
	return __kvm_gpc_refresh(gpc, gpa, uhva);
}

Unlike the existing gfn_to_hva_memslot() failure path immediately below,
jumping to out on kvm_range_has_rw_memory_protections() leaves gpc->gpa
unchanged while setting gpc->uhva = KVM_HVA_ERR_BAD:

1. If the cache was freshly initialized or previously HVA-active (so
gpc->gpa == INVALID_GPA), it is left with gpc->active == true,
gpc->gpa == INVALID_GPA, and gpc->uhva == KVM_HVA_ERR_BAD. On a subsequent
kvm_gpc_refresh() call, kvm_is_error_gpa(gpc->gpa) is true, so
kvm_gpc_refresh() passes both gpa == INVALID_GPA and uhva == KVM_HVA_ERR_BAD
into __kvm_gpc_refresh():

virt/kvm/pfncache.c:kvm_gpc_refresh() {
	...
	uhva = kvm_is_error_gpa(gpc->gpa) ? gpc->uhva : KVM_HVA_ERR_BAD;

	return __kvm_gpc_refresh(gpc, gpc->gpa, uhva);
}

virt/kvm/pfncache.c:__kvm_gpc_refresh() {
	/* Either gpa or uhva must be valid, but not both */
	if (WARN_ON_ONCE(kvm_is_error_gpa(gpa) == kvm_is_error_hva(uhva)))
		return -EINVAL;
}

This allows a guest (for example, by writing MSR_KVM_SYSTEM_TIME_NEW with a
protected GPA) to trigger the host WARN_ON_ONCE(), and prevents
kvm_gpc_refresh() from ever recovering even if userspace later clears the
memory protections.

2. In that same state (gpc->active == true and gpc->gpa == INVALID_GPA),
kvm_gpc_is_hva_active() evaluates to true, which causes
kvm_xen_hvm_get_attr() and kvm_xen_vcpu_get_attr() in arch/x86/kvm/xen.c to
copy gpc->uhva (KVM_HVA_ERR_BAD, i.e. PAGE_OFFSET) to userspace:

arch/x86/kvm/xen.c:kvm_xen_vcpu_get_attr() {
	case KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA:
		if (kvm_gpc_is_hva_active(&vcpu->arch.xen.vcpu_info_cache))
			data->u.hva = vcpu->arch.xen.vcpu_info_cache.uhva;
}

3. If the cache was already active at a valid writable GPA and is
re-activated via kvm_gpc_activate() with a new GPA that has NR or NW memory
protections, gpc->valid is cleared and gpc->uhva is set to KVM_HVA_ERR_BAD,
but gpc->gpa remains set to the old GPA. The next kvm_gpc_refresh() call will
then re-translate the stale gpc->gpa and silently resume writing to the old
guest physical address.

Should gpc->gpa (along with gpc->slots_generation and gpc->memslot) be
updated before checking kvm_range_has_rw_memory_protections(), matching the
gfn_to_hva_memslot() error path?

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