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 <[email protected]> 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/[email protected]?part=10
