Re: [PATCH 25/45] KVM: Take memory protections into account for accesses with cached gfn->hva

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] KVM: redundant mem_attr checks and RO failure in __kvm_get/put_guest()
- [Medium] KVM: unupdated attrs_generation on error in __kvm_gfn_to_hva_cache_init()
- [Medium] KVM: stale ghc->gpa on error in __kvm_gfn_to_hva_cache_init()
--

commit 70e5ce5ddda9132daea8ebdb4ea3879b1bca2fa1
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: Take memory protections into account for accesses with cached gfn->hva

This commit adds memory attribute protection checks to __kvm_get_guest(),
__kvm_put_guest(), and __kvm_gfn_to_hva_cache_init(), invalidating and
rejecting gfn_to_hva_cache entries when read or write restrictions apply
to the GPA range.

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

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2c854aaab92a1..de540af1fa244 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1371,7 +1371,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc,
>  	typeof(v) __user *__uaddr = (typeof(__uaddr))(__addr + offset);	\
>  	int __ret = -EFAULT;						\
>  									\
> -	if (!kvm_is_error_hva(__addr))					\
> +	if (!kvm_is_error_hva(__addr) &&                                \
> +	    kvm_mem_attributes_may_read_gfn(kvm, gfn))		\
>  		__ret = get_user(v, __uaddr);				\
>  	__ret;								\
>  })
> @@ -1391,7 +1392,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc,
>  	typeof(v) __user *__uaddr = (typeof(__uaddr))(__addr + offset);	\
>  	int __ret = -EFAULT;						\
>  									\
> -	if (!kvm_is_error_hva(__addr))					\
> +	if (!kvm_is_error_hva(__addr) &&                                \
> +	    kvm_mem_attributes_may_write_gfn(kvm, gfn))		\
>  		__ret = put_user(v, __uaddr);				\

[Severity: Medium]
Both __kvm_get_guest() and __kvm_put_guest() obtain __addr via
gfn_to_hva(kvm, gfn), which already checks both read and write memory
attributes:

virt/kvm/kvm_main.c:gfn_to_hva() {
	addr = gfn_to_hva_many(gfn_to_memslot(kvm, gfn), gfn, NULL);
	if (kvm_is_error_hva(addr))
		return addr;

	if (!kvm_mem_attributes_may_read_gfn(kvm, gfn))
		return KVM_HVA_ERR_BAD;

	if (!kvm_mem_attributes_may_write_gfn(kvm, gfn))
		return KVM_HVA_ERR_RO_BAD;

	return addr;
}

Because both KVM_HVA_ERR_BAD and KVM_HVA_ERR_RO_BAD satisfy
kvm_is_error_hva(__addr), the additional calls to
kvm_mem_attributes_may_read_gfn() and kvm_mem_attributes_may_write_gfn()
in these macros perform a duplicate XArray lookup in kvm->mem_attrs.array
and evaluate the macro arguments kvm and gfn a second time.

Furthermore, because __kvm_get_guest() calls gfn_to_hva() rather than
gfn_to_hva_prot(), any page marked read-only with KVM_MEMORY_ATTRIBUTE_NW
causes gfn_to_hva() to return KVM_HVA_ERR_RO_BAD, failing __kvm_get_guest()
with -EFAULT before kvm_mem_attributes_may_read_gfn() is reached.

Should __kvm_get_guest() use gfn_to_hva_prot(kvm, gfn, NULL) so that
read-only pages remain readable, and drop the redundant attribute check in
__kvm_put_guest()? Also, the commit message mentions that tracking the
memory attributes generation in gfn_to_hva_cache is required for
kvm_get/put_guest(), though kvm_get_guest() and kvm_put_guest() do not use
gfn_to_hva_cache.

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 6aa6a0a55b070..8283b2415adc2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3664,15 +3664,22 @@ static int __kvm_gfn_to_hva_cache_init(struct kvm *kvm,
>  	 * If the requested region crosses two memslots, we still
>  	 * verify that the entire region is valid here.
>  	 */
> -	for ( ; start_gfn <= end_gfn; start_gfn += nr_pages_avail) {
> -		ghc->memslot = __gfn_to_memslot(slots, start_gfn);
> -		ghc->hva = gfn_to_hva_many(ghc->memslot, start_gfn,
> -					   &nr_pages_avail);
> +	for (gfn_t gfn = start_gfn ; gfn <= end_gfn; gfn += nr_pages_avail) {
> +		ghc->memslot = __gfn_to_memslot(slots, gfn);
> +		ghc->hva = gfn_to_hva_many(ghc->memslot, gfn, &nr_pages_avail);
>  		if (kvm_is_error_hva(ghc->hva))
>  			return -EFAULT;
>  	}
>  
> +	/*
> +	 * RW memory attributes are incompatible with GHC. The RW protection
> +	 * check has to happen after storing the generation number.
> +	 */
>  	ghc->attrs_generation = kvm_mem_attributes_generation(kvm);

[Severity: Medium]
At the start of __kvm_gfn_to_hva_cache_init(), ghc->slots_generation is
updated before any error checks so that if initialization fails, subsequent
calls to kvm_read_guest_offset_cached() or kvm_write_guest_offset_cached()
within the same generation pass kvm_memslots_check_gen() and immediately
return -EFAULT on kvm_is_error_hva(ghc->hva).

Because ghc->attrs_generation is only updated after the start_gfn > end_gfn
check and the memslot lookup loop, if kvm->mem_attrs.generation changes on
a cache whose GPA fails one of those earlier checks, ghc->attrs_generation
remains stale. Every subsequent call to kvm_read_guest_offset_cached() or
kvm_write_guest_offset_cached() will then fail kvm_memslots_check_gen() and
repeatedly re-run __kvm_gfn_to_hva_cache_init().

Should ghc->attrs_generation = kvm_mem_attributes_generation(kvm) be moved
to the top of __kvm_gfn_to_hva_cache_init() alongside ghc->slots_generation?

> +	if (kvm_range_has_rw_memory_protections(kvm, start_gfn, end_gfn + 1)) {
> +		ghc->hva = KVM_HVA_ERR_BAD;
> +		return -EFAULT;
> +	}

[Severity: Medium]
When a gfn_to_hva_cache previously initialized to a valid gpa1 is
re-initialized to a new gpa2 that fails
kvm_range_has_rw_memory_protections(), __kvm_gfn_to_hva_cache_init()
updates ghc->slots_generation and ghc->attrs_generation and sets
ghc->hva = KVM_HVA_ERR_BAD, but returns -EFAULT before updating ghc->gpa
and ghc->len at the end of the function:

virt/kvm/kvm_main.c:__kvm_gfn_to_hva_cache_init() {
	...
	ghc->gpa = gpa;
	ghc->len = len;
	return 0;
}

This leaves ghc->hva poisoned with KVM_HVA_ERR_BAD while ghc->gpa still
holds the previous valid gpa1. Callers that guard cache re-initialization
with a GPA comparison, such as nested_vmx_check_vmcs_link_ptr(),
nested_cache_shadow_vmcs12(), and nested_flush_cached_shadow_vmcs12():

arch/x86/kvm/vmx/nested.c:nested_vmx_check_vmcs_link_ptr() {
	if (ghc->gpa != vmcs12->vmcs_link_pointer &&
	    CC(kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc,
					 vmcs12->vmcs_link_pointer, VMCS12_SIZE)))
		return -EINVAL;
}

or callers that keep the previous GPA active when an MSR update to a new
GPA is rejected (such as kvm_lapic_set_pv_eoi() and
kvm_pv_enable_async_pf()), will then see matching generations and fail
valid accesses to gpa1 with -EFAULT.

Would it make sense to update ghc->gpa and ghc->len at the start of
__kvm_gfn_to_hva_cache_init() before the error returns so that a failed
re-initialization does not leave a stale ghc->gpa paired with
KVM_HVA_ERR_BAD?

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