Re: [RFC PATCH v2 10/15] KVM: arm64: Introduce support to pin VMIDs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] kvm: arm64: integer underflow in max_pinned_vmids allows slab OOB write
--

commit 5166cc2f774d24bbef4a1dfb451334ee93c6273a
Author: Shameer Kolothum <skolothumtho@nvidia.com>
KVM: arm64: Introduce support to pin VMIDs

Introduce kvm_arm_pinned_vmid_get() and kvm_arm_pinned_vmid_put(), to pin
a VMID associated with a KVM instance. This will guarantee that VMID
remains the same after a rollover.

> diff --git a/arch/arm64/kvm/vmid.c b/arch/arm64/kvm/vmid.c
> index 7fe8ba1a2851c..322279f59e881 100644
> --- a/arch/arm64/kvm/vmid.c
> +++ b/arch/arm64/kvm/vmid.c
[ ... ]
> @@ -169,6 +175,49 @@ void kvm_arm_vmid_update(struct kvm_vmid *kvm_vmid)
>  	raw_spin_unlock_irqrestore(&cpu_vmid_lock, flags);
>  }
>  
> +int kvm_arm_pinned_vmid_get(struct kvm *kvm)
> +{
> +	struct kvm_vmid *kvm_vmid;
> +	u64 vmid;
> +
> +	kvm_vmid = &kvm->arch.mmu.vmid;
> +
> +	guard(raw_spinlock_irqsave)(&cpu_vmid_lock);
> +	vmid = atomic64_read(&kvm_vmid->id);
> +
> +	if (refcount_inc_not_zero(&kvm_vmid->pinned))
> +		return (vmid & ~VMID_MASK);
> +
> +	if (nr_pinned_vmids >= max_pinned_vmids)
> +		return -EINVAL;

[Severity: Critical]
Does this capacity check protect against pinning exhaustion if max_pinned_vmids
underflows?

On ARM64 systems with 8-bit VMIDs (NUM_USER_VMIDS=256) and 255 or more CPUs,
the subtraction used to set max_pinned_vmids in kvm_arm_vmid_alloc_init() will
wrap around to a huge unsigned value.

This allows userspace to bypass this check and exhaust all VMIDs by pinning
them. Once fully exhausted, new_vmid()'s fallback find_next_zero_bit() returns
256, and then falls through to the set_vmid label:

arch/arm64/kvm/vmid.c:new_vmid() {
	...
set_vmid:
	__set_bit(vmid, vmid_map);
	...
}

Can this write past the end of the 256-bit bitmap and corrupt adjacent slab
memory?

[ ... ]
> @@ -186,10 +235,24 @@ int __init kvm_arm_vmid_alloc_init(void)
>  	if (!vmid_map)
>  		return -ENOMEM;
>  
> +	pinned_vmid_map = bitmap_zalloc(NUM_USER_VMIDS, GFP_KERNEL);
> +	if (!pinned_vmid_map) {
> +		bitmap_free(vmid_map);
> +		return -ENOMEM;
> +	}
> +	nr_pinned_vmids = 0;
> +
> +	/*
> +	 * Ensure we have at least one empty slot available after rollover when
> +	 * the maximum number of VMIDs are pinned. VMID#0 is reserved.
> +	 */
> +	max_pinned_vmids = NUM_USER_VMIDS - num_possible_cpus() - 2;

[Severity: Critical]
Is it possible for this subtraction to underflow?

If a system has 8-bit VMIDs (NUM_USER_VMIDS = 256) and 255 or more CPUs,
this will result in a negative value that wraps around to ULONG_MAX.

While there is a WARN_ON(NUM_USER_VMIDS - 1 <= num_possible_cpus()) earlier
in kvm_arm_vmid_alloc_init(), it only emits a warning and does not prevent
the initialization from proceeding and setting this underflowed value.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015142552.4128028-1-ryan.roberts@arm.com?part=10