Re: [PATCH v5 11/49] KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unconditional NULL pointer dereference in `vgic_v5_alloc_l2_vmt` when the VM has zero vCPUs.
- [High] Hardware Use-After-Free (UAF) when IST validation times out.
- [High] GICv5 probe permanently aborts on kexec if the previous kernel left the VMT_BASER register valid.
- [High] Missing bounds check on `max_vpe_id` allows userspace to program out-of-bounds `vpe_id_bits` into hardware.
- [Low] Mixing goto-based error handling with scoped_guard() violates the cleanup subsystem guidelines.
--

--- Patch [11]: [PATCH v5 11/49] KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops ---
commit 47ef012a3a648a37c053cce44ccc9973a562fddf
Author: Sascha Bischoff <[email protected]>

KVM: arm64: gic-v5: Implement VMT/vIST IRS MMIO Ops
    
This commit adds support for making GICv5 VM and IST tables valid via
the IRS MMIO interface. It introduces functions to interact with IRS
hardware and manage VM/IST validity.

> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
[ ... ]
> @@ -785,8 +793,16 @@ static int vgic_v5_vmte_assign_ist(struct kvm *kvm, phys_addr_t ist_base,
>  	/* Finally, mark the entry as valid */
>  	cmd = spi_ist ? SPI_VIST_MAKE_VALID : LPI_VIST_MAKE_VALID;
>  	ret = irq_set_vcpu_affinity(vgic_v5_vpe_db(vcpu0), &cmd);

[Severity: High]
Can this result in a null pointer dereference if the VM has zero vCPUs?

The vcpu0 pointer is retrieved via kvm_get_vcpu(kvm, 0), which can return
null if kvm->online_vcpus == 0. Unconditionally passing vcpu0 to
vgic_v5_vpe_db() here, and similarly in vgic_v5_alloc_l2_vmt(), seems like
it could trigger a host crash if invoked by KVM initialization ioctls before
any vCPUs are created.

> +	if (ret) {
> +		scoped_guard(raw_spinlock_irqsave, &vgic_v5_irs_lock) {
> +			WRITE_ONCE(vmte->val[section], 0ULL);
> +			vgic_v5_clean_inval(vmte, sizeof(*vmte));
> +		}
>  
> -	return ret;
> +		return ret;

[Severity: High]
Could this lead to a hardware use-after-free if the validation command times
out?
    
If the MMIO command sent in vgic_v5_vmte_assign_ist() times out, it returns
an error here. Callers like vgic_v5_spi_ist_alloc() and
vgic_v5_lpi_ist_alloc() will react by freeing the IST memory:

arch/arm64/kvm/vgic/vgic-v5-tables.c:vgic_v5_spi_ist_alloc() {
    ...
    if (ret) {
        vgic_v5_free_allocated_spi_ist(kvm);
        return ret;
    }
    ...
}
    
Since there is no SPI_VIST_MAKE_INVALID command, freeing this memory while
the hardware is still processing the command (or caching the memory pointer)
might corrupt host memory. Should KVM intentionally leak the memory on
timeout instead?

> +	}
> +
> +	return 0;
>  }
>  
>  /*
> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
[ ... ]
> @@ -83,13 +102,52 @@ int vgic_v5_probe(const struct gic_kvm_info *info)
>  	vgic_v5_irs_cache_id_regs(info);
>  	vgic_v5_get_implemented_ppis();
>  
[ ... ]
> +	ret = vgic_v5_irs_assign_vmt(vgic_v5_irs_two_level_vmt_support(&irs_caps),
> +				     ilog2(vgic_v5_irs_max_vms(&irs_caps)),
> +				     vgic_v5_get_vmt_base());
> +	if (ret) {
> +		kvm_err("Failed to assign the GICv5 VM tables to the IRS; no GICv5 support\n");
> +		if (!vgic_v5_irs_clear_vmt())
> +			vgic_v5_vmt_free();
> +		return -ENODEV;
> +	}

[Severity: High]
Will this permanently abort the GICv5 probe across kexec?

If booting via kexec while a VM was running in the previous kernel, the
hardware VMT_BASER register retains its valid state. When this occurs,
vgic_v5_irs_assign_vmt() unconditionally returns -EBUSY:

arch/arm64/kvm/vgic/vgic-v5.c:vgic_v5_irs_assign_vmt() {
    ...
    vmt_baser = irs_readq_relaxed(GICV5_IRS_VMT_BASER);
    if (!!FIELD_GET(GICV5_IRS_VMT_BASER_VALID, vmt_baser))
        return -EBUSY;
    ...
}

This error path clears the stale state using vgic_v5_irs_clear_vmt(), but
then returns -ENODEV. Should the probe retry the assignment instead of
failing entirely?

[ ... ]

While reviewing the VM table setup, I also noticed a few potential regressions
in vgic_v5_vmte_init():

arch/arm64/kvm/vgic/vgic-v5-tables.c:vgic_v5_vmte_init() {
    ...
    kvm_for_each_vcpu(i, vcpu, kvm) {
        u16 vpe_id = vgic_v5_vpe_id(vcpu);
        if (vpe_id > max_vpe_id)
            max_vpe_id = vpe_id;
    }

    nr_cpus = max(2UL, roundup_pow_of_two(max_vpe_id + 1));
    vmi->vpe_id_bits = fls(nr_cpus) - 1;
    ...
    tmp |= FIELD_PREP(GICV5_VMTEL2E_VPE_ID_BITS, vmi->vpe_id_bits);
    vmte_val1 = tmp;
    ...
}

[Severity: High]
Is there a missing bounds check on max_vpe_id here?

The function verifies the total number of online vCPUs against hardware
limits, but calculates the hardware configuration based on the maximum sparse
vcpu_id. Since KVM allows sparse IDs up to KVM_MAX_VCPU_IDS, an unprivileged
userspace could create a single vCPU with a large ID.

This allows calculating an oversized vpe_id_bits and writing it directly to
the VMTE, exposing a configuration to the hardware that exceeds its supported
capabilities. Should max_vpe_id be checked against vmt_info->max_vpes?

Additionally, in the same function:

arch/arm64/kvm/vgic/vgic-v5-tables.c:vgic_v5_vmte_init() {
    ...
    scoped_guard(raw_spinlock_irqsave, &vgic_v5_irs_lock) {
        WRITE_ONCE(vmte->val[0], cpu_to_le64(vmte_val0));
        ...
    }
    ...
out_fail:
    kfree(vmd);
    ...
}

[Severity: Low]
Does this mix goto-based error handling with scoped_guard()?

According to the cleanup subsystem guidelines, if a function uses guard() or
scoped_guard(), all resources should be converted to scope-based cleanup,
avoiding mixed usage with goto-based cleanup labels.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11