Re: [PATCH] kvm: add EVER_MAPPED ioctl and capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The `ever_mapped_bitmap` is dynamically allocated during VM capability enablement but never freed during VM teardown.
- [High] Buffer overflow and KASAN out-of-bounds access due to a byte-aligned allocation size being manipulated by unsigned long bit operations.
- [High] Missing `cond_resched()` in the fallback iteration loop allows a single ioctl to monopolize the CPU, causing soft lockups.
- [High] `KVM_CAP_EVER_MAPPED` enablement succeeds based on the global `tdp_enabled` hardware flag, but silently fails to track memory if the legacy shadow MMU is actively handling the VM.
- [Medium] Unvalidated `reserved` fields in `struct kvm_ever_mapped_log` break future ABI compatibility.
- [Medium] Lockless reading of `kvm->arch.ever_mapped_max_gpa` and `ever_mapped_bitmap` can cause TOCTOU races and spurious errors.
--

--- Patch [1]: [PATCH] kvm: add EVER_MAPPED ioctl and capability ---
commit bb0c368d3b83d1838e5cca827b1430b4b426ca4d
Author: Florian Schmidt <flosch@nutanix.com>
Subject: kvm: add EVER_MAPPED ioctl and capability

This commit introduces a new capability, KVM_CAP_EVER_MAPPED, which allows a
VMM to track which guest pages have ever been mapped using a bitmap. It also
adds an ioctl, KVM_GET_EVER_MAPPED_LOG, to retrieve this tracking information.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0626e835e9eb7..89cbcf8399f49 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -2391,6 +2392,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_READONLY_MEM:
>  		r = kvm ? kvm_arch_has_readonly_mem(kvm) : 1;
>  		break;
> +	case KVM_CAP_EVER_MAPPED:
> +		r = (tdp_enabled && IS_ENABLED(CONFIG_X86_64));
> +		break;

[Severity: High]
Does this successfully track memory when the VM is running under the legacy
shadow MMU?

If a VM uses the legacy shadow MMU (for example, with nested L2 guests or the
kvm.tdp_mmu=0 module parameter), the page mappings are handled in mmu.c, which
bypasses the kvm_ever_mapped_set_range() hook added to tdp_mmu.c. Since this
check only verifies the global tdp_enabled hardware flag, could this silently
fail to track memory and cause userspace to observe a clean log when the pages
were actually modified?

[ ... ]

> @@ -4190,6 +4194,36 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		mutex_unlock(&kvm->lock);
>  		break;
>  	}
> +	case KVM_CAP_EVER_MAPPED: {
> +		unsigned long *mapped_bitmap;
> +		unsigned long num_granules;
> +
> +		r = 0;
> +		mutex_lock(&kvm->lock);
> +		if (!tdp_enabled) {
> +			r = -EOPNOTSUPP;
> +		} else if (kvm->arch.ever_mapped_bitmap) {
> +			r = -EEXIST;
> +		} else if (cap->args[0] == 0 ||
> +			   cap->args[0] > (1ULL << kvm_host.maxphyaddr)) {
> +			r = -EINVAL;
> +		} else {
> +			num_granules = DIV_ROUND_UP(cap->args[0], 1ULL << KVM_EVER_MAPPED_SHIFT);
> +			mapped_bitmap =
> +				kvcalloc(BITS_TO_LONGS(num_granules),
> +					 sizeof(long), GFP_KERNEL_ACCOUNT);

[Severity: High]
Is this allocated memory ever freed?

When a VM is destroyed via the kvm_arch_destroy_vm() path, does it clean up
the ever_mapped_bitmap? An unprivileged process could repeatedly create VMs,
enable this capability, and destroy the VMs, potentially exhausting host
memory if the bitmap isn't explicitly freed.

> +			if (!mapped_bitmap)
> +				r = -ENOMEM;
> +			else {
> +				write_lock(&kvm->mmu_lock);
> +				kvm->arch.ever_mapped_bitmap = mapped_bitmap;
> +				kvm->arch.ever_mapped_max_gpa = num_granules << KVM_EVER_MAPPED_SHIFT;
> +				write_unlock(&kvm->mmu_lock);
> +			}
> +		}
> +		mutex_unlock(&kvm->lock);
> +		break;
> +	}

[ ... ]

> @@ -4197,6 +4231,73 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  	return r;
>  }
>  
> +static int kvm_vm_ioctl_get_ever_mapped_log(struct kvm *kvm,
> +					    struct kvm_ever_mapped_log *log)
> +{
> +	u64 max_granules;
> +	unsigned int coarseness;
> +	unsigned long *bitmap;
> +	int r;
> +
> +	if (!kvm->arch.ever_mapped_bitmap)
> +		return -ENOENT;
> +
> +	if (log->flags)
> +		return -EINVAL;

[Severity: Medium]
Should we also validate that the reserved fields in the log structure are
strictly zero here?

If userspace leaves log->reserved uninitialized, stack garbage could be passed
in. Does this prevent safely repurposing these fields in the future without
breaking ABI compatibility with older binaries?

> +
> +	if (log->granule_shift < KVM_EVER_MAPPED_SHIFT || log->granule_shift >= 64)
> +		return -EINVAL;
> +
> +	max_granules = kvm->arch.ever_mapped_max_gpa >> log->granule_shift;

[Severity: Medium]
Could this lockless read of ever_mapped_max_gpa cause a TOCTOU race?

If this is called concurrently with KVM_CAP_EVER_MAPPED enablement, could the
reader observe a non-NULL ever_mapped_bitmap but a stale max_gpa of 0 due to
compiler reordering or missing memory barriers?

If so, max_granules would evaluate to 0 and the ioctl might erroneously fail
with -EINVAL.

> +	if (log->num_granules > max_granules ||
> +	    log->first_granule > max_granules - log->num_granules)
> +		return -EINVAL;
> +
> +	/*
> +	 * For the case where the request uses the same granularity as our internal
> +	 * tracking, and we are byte-aligned (the expected common case), we can
> +	 * just copy_to_user. Otherwise, we have to create a new bitmap to pass.
> +	 *
> +	 * We do not lock against concurrent vCPUs mapping new memory on any of
> +	 * these operations. Bits are set atomically and never cleared, so any race
> +	 * here is indistinguishable from a write happening after this handler
> +	 * finishes but before the caller reads the results.
> +	 */
> +	coarseness = log->granule_shift - KVM_EVER_MAPPED_SHIFT;
> +	if (coarseness == 0 && !(log->first_granule & 7) && !(log->num_granules & 7)) {
> +		if (copy_to_user(log->bitmap,
> +				 (u8 *)kvm->arch.ever_mapped_bitmap + log->first_granule / 8,
> +				 log->num_granules / 8))
> +			return -EFAULT;
> +		return 0;
> +	}
> +
> +	bitmap = kvzalloc(DIV_ROUND_UP(log->num_granules, 8), GFP_KERNEL_ACCOUNT);

[Severity: High]
Does this allocation size safely accommodate the unsigned long bit operations
that follow?

The buffer is allocated with exact byte granularity here using
kvzalloc(DIV_ROUND_UP(log->num_granules, 8), ...), but it is later populated
using __set_bit() which operates on 8-byte (unsigned long) boundaries.

If the requested size is not a multiple of 8 bytes (for example, 9 bytes for
65 granules), could __set_bit() access memory past the requested allocation
boundary and trigger a KASAN out-of-bounds splat?

> +	if (!bitmap)
> +		return -ENOMEM;
> +
> +	for (u64 gran = log->first_granule;
> +	     gran < log->first_granule + log->num_granules;
> +	     gran++) {

[Severity: High]
Can this loop monopolize the CPU and trigger a soft lockup?

If userspace calls this ioctl with unaligned parameters that bypass the
copy_to_user fast path, log->num_granules can be very large (up to 2^31 for a
52-bit physical address space and a 2MB granule).

Does iterating over a potentially massive user-controlled scale without a
cond_resched() in this loop risk keeping the CPU occupied long enough to
trigger the soft lockup watchdog?

> +		unsigned long start = gran << coarseness;
> +		unsigned long end = start + (1UL << coarseness);
> +
> +		if (find_next_bit(kvm->arch.ever_mapped_bitmap, end, start) < end)
> +			__set_bit(gran - log->first_granule, bitmap);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731091708.2963414-1-flosch@nutanix.com?part=1