Re: [PATCH v4 09/48] KVM: arm64: gic-v5: Create and manage VM and VPE tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `vgic_v5_alloc_l2_vmt()` unconditionally dereferences `kvm_get_vcpu(kvm, 0)`, causing a host kernel panic if no vCPUs are online.
- [High] Allocating hardware-modified tables (VMTE and VPETE) densely in normal cacheable memory causes silent data corruption on non-coherent IRS systems due to cacheline false sharing.
- [High] Missing cache invalidation before zeroing the VPE table entry causes false-sharing corruption of neighboring hardware state.
- [High] The error handling path in `vgic_v5_vmte_init()` incorrectly calls `xa_erase()` when `xa_insert()` fails with `-EBUSY`, destroying pre-existing valid state.
- [High] Early returns on error in `vgic_v5_vmte_release()` bypass critical cleanup steps, permanently leaking memory and leaving hardware state active.
- [Medium] Lockless read of `l2ptrs` array without `READ_ONCE()` can lead to observing a stale or partially updated pointer.
--

--- Patch [9]: [PATCH v4 09/48] KVM: arm64: gic-v5: Create and manage VM and VPE tables ---
commit 6d18ab27d2875b7a3e267a39ae7efb4397ebac92
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>
Subject: KVM: arm64: gic-v5: Create and manage VM and VPE tables

GICv5 uses a set of in-memory tables to track and manage VM
state. These must be allocated by the hypervisor, and provided to the
IRS to use.

> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> new file mode 100644
> index 0000000000000..22cd03e0f1963
> --- /dev/null
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> @@ -0,0 +1,630 @@

[ ... ]

> +static int vgic_v5_alloc_vmt_linear(unsigned int num_entries)
> +{
> +	vmt_info->linear.vmt_base = kzalloc_objs(*vmt_info->linear.vmt_base,
> +						 num_entries);

[Severity: High]
Does allocating the hardware-modified VMTE table densely in normal cacheable
memory cause silent data corruption on non-coherent IRS systems?

Since kzalloc_objs() allocates the 32-byte VMTE entries contiguously,
multiple entries will share a 64-byte CPU cache line. If a CPU modifies one
entry, the subsequent DC CIVAC cache maintenance could write the entire cache
line back to memory, overwriting any concurrent hardware updates to
neighboring entries.

[ ... ]

> +static int vgic_v5_alloc_l2_vmt(struct kvm *kvm)
> +{
> +	struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);

[ ... ]

> +	/*
> +	 * VMAP in the L2 VMT via the IRS. We use any of the VM's CPUs as a
> +	 * conduit for interacting with the host's IRS. In the current case,
> +	 * this lets us resolve the VM ID to pass to the hardware.
> +	 */
> +	ret = irq_set_vcpu_affinity(vgic_v5_vpe_db(vcpu0), &cmd);

[Severity: High]
Can vcpu0 be NULL here, leading to a host kernel panic?

If vgic_v5_vmte_init() is called before any vCPUs are online (which it appears
to handle by rounding nr_cpus up to 2), kvm_get_vcpu(kvm, 0) will return NULL.
vgic_v5_vpe_db(vcpu0) unconditionally dereferences the vCPU pointer, which
would cause an immediate crash.

[ ... ]

> +static struct vmtl2_entry *vgic_v5_get_l2_vmte(u32 vm_id)
> +{

[ ... ]

> +	if (!vmt_info->l2.l2ptrs[l1_index])
> +		return ERR_PTR(-EINVAL);
> +
> +	l2_table = vmt_info->l2.l2ptrs[l1_index];

[Severity: Medium]
Could this lockless read of l2ptrs observe a stale or partially updated
pointer without READ_ONCE()?

Since the array element is accessed locklessly and read twice (once for the
NULL check and once for the assignment), the compiler might emit two separate
loads. A concurrent update in vgic_v5_alloc_l2_vmt() could cause these loads
to return inconsistent states.

[ ... ]

> +int vgic_v5_vmte_init(struct kvm *kvm)
> +{

[ ... ]

> +	ret = xa_insert(&vm_info, vm_id, vmi, GFP_KERNEL);
> +	if (ret)
> +		goto out_fail;

[ ... ]

> +	vpet = kzalloc_objs(*vpet, nr_cpus);

[Severity: High]
Does allocating the 8-byte VPETE table entries densely cause the same false
sharing and data corruption issues as with the VMTE table?

Since the entries are tightly packed into cache lines, a flush for one entry
could destroy concurrent hardware modifications to neighboring entries on
non-coherent IRS systems.

[ ... ]

> +out_fail:
> +	/* kfree(NULL) is safe so we can just kfree() at leisure */
> +	kfree(vmd);
> +	kfree(vpet);
> +	kfree(vped_ptrs);
> +	if (vmi)
> +		xa_erase(&vm_info, vm_id);

[Severity: High]
Is it safe to unconditionally erase the XArray entry if xa_insert() fails?

If xa_insert() fails with -EBUSY because an entry already exists for this
vm_id (e.g., due to concurrent initialization), the code jumps to out_fail.
Since vmi was successfully allocated and is non-NULL, this cleanup block calls
xa_erase(), blindly deleting the pre-existing valid entry and corrupting KVM's
tracking of an active VM.

[ ... ]

> +int vgic_v5_vmte_release(struct kvm *kvm)
> +{

[Severity: High]
Are there early return paths in this function (such as when freeing LPI or
SPI IST structures) that bypass critical cleanup steps?

If freeing the IST structures fails and returns an error early, it bypasses
the rest of the teardown logic. It fails to free the vped_ptrs, vpet_base,
and vmd_base allocations, and skips calling vgic_v5_reset_vmte(). This leaves
the hardware state active and permanently leaks memory.

[ ... ]

> +int vgic_v5_vmte_free_vpe(struct kvm_vcpu *vcpu)
> +{

[ ... ]

> +	vpet_base = vmi->vpet_base;
> +	WRITE_ONCE(vpet_base[vpe_id], 0ULL);
> +
> +	vgic_v5_clean_inval(vpet_base + vpe_id, sizeof(vpe_entry));

[Severity: High]
Does this sequence need a cache invalidation before the write to avoid false
sharing corruption?

If the CPU holds a stale, clean copy of this cache line, the WRITE_ONCE()
dirties it without fetching the latest hardware state from RAM. The subsequent
cache flush writes this stale line back to memory, destroying concurrent
hardware modifications to other VPE entries sharing the same cache line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724104819.1296803-1-sascha.bischoff@arm.com?part=9