Re: [PATCH 15/24] 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:
- [High] gfn_to_pfn_cache (GPC) bypasses memory attribute protections (NR/NW).
- [Medium] Redundant memory attribute checks in __kvm_get_guest and __kvm_put_guest lead to duplicate xarray lookups (performance regression).
- [Low] kvm_memslots_check_gen() causes a build failure when CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES is disabled.
--

--- Patch [15]: [PATCH 15/24] KVM: Take memory protections into account for accesses with cached gfn->hva ---
commit 51177f153d521afab2fb1286cac8c924ca7aa90d
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

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

Account for memory attributes when accessing guest memory through
kvm_get/put_guest().

This requires tracking the memory attributes generation as part of
gfn_to_hva_cache's data, invalidate the cached information if the
generation changes, and failing to refresh the cache if restrictive
memory attributes are found within the GPA range.

> Similar to how gfn_to_hva_cache disallows caching gfns mapped within
> read-only memory slots, gfns marked as read-only by memory attributes
> will also fail to initialize. Unsurprisingly, the same behaviour applies
> to gfns mapped as non-accessible (NR/NW), while non-executable mappings
> are okay.

[Severity: High]
Does gfn_to_pfn_cache (GPC) bypass memory attribute protections (NR/NW) while
this patch updates gfn_to_hva_cache (GHC)?

In virt/kvm/pfncache.c:__kvm_gpc_refresh():

			gpc->attrs_generation = kvm_mem_attributes_generation(kvm);
			gpc->gpa = gpa;
			gpc->slots_generation = slots->generation;
			gpc->memslot = __gfn_to_memslot(slots, gfn);
			gpc->uhva = gfn_to_hva_memslot(gpc->memslot, gfn);

The HVA is retrieved without evaluating memory attributes to see if access is
actually permitted. KVM can bypass guest memory isolation boundaries, writing
to memory that the guest expects to be read-only (NW). Should gfn_to_pfn_cache
also validate kvm_mem_attributes_may_write_gfn()?

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1fad3eb303c17..71ab2cbecbd12 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
[ ... ]
> @@ -1351,7 +1351,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;								\
>  })

[Severity: Medium]
Are these memory attribute checks redundant and introducing an unnecessary
xarray lookup on the fast path?

gfn_to_hva() inherently checks memory attributes and fails if not permitted,
rendering these macro checks redundant:

virt/kvm/kvm_main.c:gfn_to_hva() {
    ...
	if (!kvm_mem_attributes_may_write_gfn(kvm, gfn))
		return KVM_HVA_ERR_RO_BAD;
    ...
}

If !kvm_is_error_hva(__addr) is true, the memory is already guaranteed to be
readable and writable.

> @@ -1371,7 +1372,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);				\
>  	if (!__ret)							\
>  		mark_page_dirty(kvm, gfn);				\

[Severity: Medium]
Similar to __kvm_get_guest above, does this inject a redundant xarray lookup
since !kvm_is_error_hva() already confirmed the memory is writable?

[ ... ]
> @@ -2632,6 +2634,12 @@ static inline unsigned long kvm_get_memory_attributes(struct kvm *kvm, gfn_t gfn
>  {
>  	return 0;
>  }
> +static inline bool kvm_range_has_any_memory_attributes(struct kvm *kvm,
> +						       gfn_t start, gfn_t end,
> +						       unsigned long mask)
> +{
> +	return false;
> +}
>  static inline u64 kvm_mem_attributes_generation(struct kvm *kvm)
>  {
>  	return 0;
>  }

[Severity: Low]
Will kvm_memslots_check_gen() cause a build failure when
CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES is disabled?

kvm_memslots_check_gen() unconditionally accesses kvm->mem_attrs.generation:

include/linux/kvm_host.h:kvm_memslots_check_gen() {
    ...
	return slots->generation == slots_generation && 
               kvm->mem_attrs.generation == attrs_generation;
}

However, the mem_attrs struct member is conditionally compiled under
CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES. Should kvm_memslots_check_gen() use the
kvm_mem_attributes_generation(kvm) wrapper instead, which correctly returns 0
when the feature is disabled?

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