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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware-modified VMT and VPET tables are allocated as Cacheable memory and managed with CPU cache maintenance, leading to false sharing and loss of hardware writes on systems with a non-coherent IRS.
- [High] Incorrect calculation of `vpe_id_bits` tells the hardware the VPET is twice its actual size, allowing a guest to trigger out-of-bounds IRS memory accesses.
- [High] Failure to check if `vped_size` is zero before allocation leads to `ZERO_SIZE_PTR` usage, resulting in bogus physical addresses and a kernel panic during cache maintenance.
- [High] The L1 Virtual Machine Table and linear VMT allocations lack the 4KB page alignment required by the GICv5 hardware for `IRS_VMT_BASER`.
- [Medium] Error paths in `vgic_v5_vmte_release` return early after freeing VPE tracking pointers, leaving dangling pointers in the globally reachable `vmi` structure.
--

commit f0a39157639262cdd2140f7a3d5512f31e4004d2
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>
KVM: arm64: gic-v5: Create and manage VM and VPE tables

This patch adds support for allocating the Virtual Machine Table (VMT) and
Virtual PE Table (VPET) to track VM state for GICv5. It introduces lifecycle
management and tracking for unused VMTEs via an IDA.

> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> index db8cb6a384994..xxxxxxxxxxxxx 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
[ ... ]
> 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 this allocation guarantee the 4KB alignment required by the GICv5
hardware for IRS_VMT_BASER?

When the base address is written to VMT_BASER in vgic_v5_irs_assign_vmt(),
the lower 12 bits are stripped:

vmt_baser = FIELD_PREP(...) | (vmt_base & GICV5_IRS_VMT_BASER_ADDR);

Since kzalloc_objs() only guarantees SLUB alignment (e.g., 8 or 16 bytes
for small allocations), won't the truncation of the lower bits cause the
hardware to read from the wrong address at the start of the 4KB page?

[ ... ]
> static int vgic_v5_alloc_vmt_two_level(unsigned int num_entries)
> {
[ ... ]
> 	vmt_info->l2.vmt_base = kzalloc_objs(*vmt_info->l2.vmt_base,
> 					     vmt_info->l2.num_l1_ents);

[Severity: High]
Should these hardware-modified tables be allocated from Non-Cacheable memory
instead of standard Cacheable memory (via kzalloc_objs)?

For systems with a non-coherent IRS, vgic_v5_clean_inval() uses
dcache_clean_inval_poc() which executes DC CIVAC. Because VMT and VPET
entries are tightly packed, multiple entries can share a single 64-byte
cache line.

If the hardware writes to these tables (like setting VALID bits) and the
CPU later invalidates or cleans an adjacent entry, could the CIVAC operation
write back the CPU's stale cache state and silently overwrite concurrent
hardware updates?

[ ... ]
> int vgic_v5_vmte_init(struct kvm *kvm)
> {
[ ... ]
> 	/*
> 	 * Allocate and assign the VPE Table. Round up the number of CPUs to a
> 	 * whole power of two as we cannot describe non-powers-of-two in the
> 	 * VMTE field as it conveys the number of ID bits used and not the
> 	 * number of vPEs.
> 	 *
> 	 * The IRS encodes the number of IAFFID bits as N - 1, so a VM with a
> 	 * single vCPU must still allocate two VPET entries and expose 1 bit.
> 	 */
> 	nr_cpus = max(2UL, roundup_pow_of_two(nr_cpus));
> 	vmi->vpe_id_bits = fls(nr_cpus) - 1;

[Severity: High]
Is there an off-by-one calculation error here for vmi->vpe_id_bits?

The hardware expects the number of ID bits to be encoded as N - 1 for a
table of 2^N entries. Since nr_cpus is already rounded up to a power of 2,
fls(nr_cpus) - 1 evaluates to ilog2(nr_cpus), which is N rather than N - 1.

By writing N into the VPE_ID_BITS field, does this instruct the hardware
that the VPET has 2^(N+1) entries instead of 2^N? A guest could potentially
trigger out-of-bounds IRS memory accesses by targeting an interrupt at a
VPE ID in the non-existent upper half.

[ ... ]
> int vgic_v5_vmte_release(struct kvm *kvm)
> {
[ ... ]
> 	for (int i = 0; i < BIT(vmi->vpe_id_bits); i++)
> 		kfree(vmi->vped_ptrs[i]);
> 	kfree(vmi->vped_ptrs);
> 	kfree(vmi->vpet_base);
> 	kfree(vmi->vmd_base);
> 
> 	/* Unlikely, but possible. Avoid leaking the memory. */
> 	vgic_v5_drain_pending_irqs(kvm, vmi, false);
> 
> 	/* If we have an LPI IST, free it */
> 	if (vmi->h_lpi_ist) {
> 		ret = vgic_v5_lpi_ist_free(kvm);
> 		if (ret)
> 			return ret;
> 	}

[Severity: Medium]
Does returning early here leave dangling pointers in the globally reachable
vmi structure?

The pointers for vped_ptrs, vpet_base, and vmd_base are unconditionally
freed just above this check. If vgic_v5_lpi_ist_free() (or
vgic_v5_spi_ist_free() below it) fails, the function returns immediately
without erasing the vmi object from the vm_info XArray or setting the
freed pointers to NULL.

If the teardown is retried, will this result in a double-free of these
pointers when vgic_v5_vmte_release() runs again?

[ ... ]
> int vgic_v5_vmte_alloc_vpe(struct kvm_vcpu *vcpu)
> {
[ ... ]
> 	/* Alloc VPE Descriptor. Only used by IRS. */
> 	vped = kzalloc(vmt_info->vped_size, GFP_KERNEL);
> 	if (!vped)
> 		return -ENOMEM;

[Severity: High]
Could vmt_info->vped_size be zero here, leading to a ZERO_SIZE_PTR
allocation?

Unlike the VM descriptor allocation which explicitly checks if
vmt_info->vmd_size != 0, this allocation proceeds unconditionally. If the
IRS does not require a VPE descriptor and specifies a size of 0, kzalloc()
will return the non-null ZERO_SIZE_PTR (0x10).

If that happens, won't virt_to_phys() generate a bogus physical address for
the hardware, and won't the subsequent vgic_v5_clean_inval(vped, ...)
cause a kernel panic when dcache_clean_inval_poc() executes a cache
maintenance instruction on the unmapped 0x10 address?

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