Re: [PATCH 1/2] KVM: Bound per-VM GFN materialization in KVM_SET_MEMORY_ATTRIBUTES

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Recounting materialized entries on every mutation is an O(N) operation that causes severe CPU exhaustion.
- [High] The arbitrary 128 GiB limit (KVM_MEM_ATTR_MAX_GFNS) introduces a severe functional regression for large confidential VMs.
- [Medium] Iterating the XArray using xas_for_each() without holding rcu_read_lock() triggers lockdep warnings.

Pre-existing issues:
- [High] The KVM_MEM_ATTR_MAX_GFNS limit check is bypassed when clearing attributes, failing to fix the memory exhaustion DoS.
--

commit 200db6833ee43dd9cdcad914e3c4e7942e9f7e5a
Author: David Ballesteros <davimaba.v@proton.me>
KVM: Bound per-VM GFN materialization in KVM_SET_MEMORY_ATTRIBUTES

This commit attempts to fix a memory exhaustion denial of service by
capping the maximum number of materialized memory attribute GFNs per VM.
It introduces a counter for materialized entries and restricts updates
exceeding a hardcoded limit.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -573,6 +573,13 @@ static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
>  #define KVM_MEM_MAX_NR_PAGES ((1UL << 31) - 1)
> +/*
> + * Hardening bound: maximum number of GFNs with a materialized entry in
> + * mem_attr_array per VM (~300 MiB of xa_nodes at 2^25).  Without it,
> + * KVM_SET_MEMORY_ATTRIBUTES grows the array without limit (~9.3 bytes of
> + * kernel memory per GFN) on GFNs with no backing memory.
> + */
> +#define KVM_MEM_ATTR_MAX_GFNS	(1UL << 25)

[Severity: High]
Does this limit cause legitimate large enterprise confidential VMs to fail?

A limit of 1UL << 25 pages equates to exactly 128 GiB of memory on systems
using a 4 KiB page size. Since AMD SEV-SNP and Intel TDX VMs routinely require
hundreds of gigabytes or even terabytes of private memory, capping the
allowed private memory to 128 GiB per VM seems like a functional regression
that will break existing large setups with -ENOSPC.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2544,6 +2545,33 @@ static bool kvm_pre_set_memory_attributes(struct kvm *kvm,
> [ ... ]
> +static unsigned long kvm_count_mem_attr_entries(struct kvm *kvm)
> +{
> +	XA_STATE(xas, &kvm->mem_attr_array, 0);
> +	unsigned long count = 0;
> +	void *entry;
> +
> +	xas_for_each(&xas, entry, ULONG_MAX) {

[Severity: Medium]
Will this xas_for_each() loop trigger lockdep warnings when PROVE_RCU is
enabled? 

The caller holds the slots_lock, but iterating the XArray using this
macro implicitly dereferences node pointers. XArray requires either
rcu_read_lock() or the internal xa_lock to satisfy its lockdep invariants
during traversal, neither of which appear to be held here.

> @@ -2579,6 +2608,23 @@ static int kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
> [ ... ]
> +	if (attributes &&
> +	    kvm->mem_attr_gfn_count + (end - start) > KVM_MEM_ATTR_MAX_GFNS) {
> +		r = -ENOSPC;
> +		goto out_unlock;
> +	}

[Severity: High]
This is a pre-existing issue, but does bypassing the limit check when clearing
attributes re-expose the vulnerability?

Because this check requires attributes to be non-zero, a caller can set a
single attribute at index 0, and then call KVM_SET_MEMORY_ATTRIBUTES with
attributes set to 0 for an enormous range. The kvm_range_has_memory_attributes()
function will evaluate to false due to the index 0 entry, and this limit check
will be bypassed.

> [ ... ]
>  	for (i = start; i < end; i++) {
>  		r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT);
>  		if (r)
>  			goto out_unlock;

...which causes this subsequent loop to unconditionally reserve XArray nodes for
billions of GFNs until the host hits ENOMEM, exiting without freeing the leaked
reservations.

> [ ... ]
>  out_unlock:
> +	/*
> +	 * Resynchronize with the actual state only when the array may have
> +	 * changed: on ENOMEM partway through the reserve loop, partially
> +	 * reserved GFNs are retained and must be counted, and a successful
> +	 * set or clear changes the population.  Non-mutating exits skip the
> +	 * walk so idempotent requests stay O(1).
> +	 */
> +	if (mutated)
> +		kvm->mem_attr_gfn_count = kvm_count_mem_attr_entries(kvm);
>  	mutex_unlock(&kvm->slots_lock);

[Severity: High]
Does this trigger a severe CPU denial of service during standard memory updates?

When a guest issues a series of 1-page memory attribute updates, this mutated
flag will be true for each successful update. Calling kvm_count_mem_attr_entries()
then iterates over up to 33.5 million populated entries in the XArray while holding
the slots_lock.

Performing an O(N) iteration that takes tens of milliseconds on every successful
mutation yields an O(N^2) complexity that ties up the CPU and monopolizes the
slots_lock for long periods, blocking memslot and dirty logging operations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911184819.101123-1-davimaba.v@proton.me?part=1