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

On Fri, 2026-07-24 at 11:19 +0000, [email protected] wrote:
> 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 <[email protected]>
> 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.

The way that this was structured it was possible for two adjacent VMTEs
to be manipulated at the same time. I've reworked things a little to
move these under the locking we already have for the IRS MMIO
interface. This interface is already used to make VMTEs valid, for
example, so it is logical to use the same lock for the VMTEs.

With that change we can no longer be in the situation where we are
modifying more than one VMTE at any one time. We make sure to clean and
invalidate in any place where we have modified it, and also explicitly
make sure to read the latest copy before reading it.

> 
> [ ... ]
> 
> > +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?

No. We fail GICv5 init if there are no online vcpus. it isn't possible
to make it this far without vcpu0 existing.

> 
> 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.

We will never make it this far, so this is a non-issue.

> 
> [ ... ]
> 
> > +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.

There's no way for there to be a concurrent update. We call
vgic_v5_get_l2_vmte() for each VM as part of initialising the VMTE. We
only ever change this entry once, which takes it from a NULL pointer to
a valid pointer, and it remains as that forevermore. The only time when
we might tear things down is when there has been a probe failure, and
therefore 

We should never be in the situation where the first and second load
would differ. Either both are NULL or both are valid. READ_ONCE() is
not needed here.

> 
> [ ... ]
> 
> > +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?

Yes, it could potentially do that. I've moved this to use the same
locking as I mentioned above.

> 
> 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.

Which is precisely what we are trying to avoid. As with the VMTEs, I've
re-worked this to avoid the situation where we might be modifying
things concurrently with the hardware. Additionally, I've grown the
allocation to be at least once cache line to avoid sharing the line
with something else that could end up clobbering the data.

> 
> [ ... ]
> 
> > +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.

I've added a local bool to track if we allocated or not. We now only
call xa_erase() if we successfully inserted.

> 
> [ ... ]
> 
> > +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?

I don't believe so, no.

If those fail, then we need to bypass other parts as we cannot cleanly
tear things down.  In that case, we return and error and the VM ID is
left allocated. We lose the memory (we already failed to free it
properly) and lose a usable VM ID, but that's better than accidentally
reusing memory that the hardware might still have a handle to.

> 
> 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.

If we don't have a VMTE, then there is nothing to do to begin with and
hence we return. If the VM is still marked as valid, we can't tear
things down, so again we return having done nothing. Beyond that, we
tear down everything that is valid, unless we explicitly fail to tear
down the ISTs.

> 
> [ ... ]
> 
> > +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.
> 

We are only doing this on the teardown path, and only reach this point
once the VM itself is marked as invalid. The hardware should not be
writing to the VPET here, and we are simply tearing down the state.
This should be fine.

Thanks,
Sascha