Re: [PATCH v6 10/49] KVM: arm64: gic-v5: Introduce guest IST alloc and management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Integer overflow in `l1sz` calculation causes a zero-size allocation and subsequent kernel panic during cache maintenance.
- [High] DMA coherency cacheline-sharing bug in VMTE array causes the host CPU to overwrite and destroy hardware IRS updates.
--

commit 6ec34cb3226f9d34ed3b83ac3a0c85077a487f20
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>

KVM: arm64: gic-v5: Introduce guest IST alloc and management

This commit introduces the machinery to allocate guest Interrupt State Tables
(ISTs) for SPIs and LPIs in KVM for ARM64 GICv5. It provides functions to
allocate linear and two-level ISTs, assign them to a VM via IRS doorbell
commands, and free them on VM teardown.

> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> index 7252d48431a5a..f088006f39674 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c

[ ... ]

> @@ -679,3 +722,511 @@ int vgic_v5_vmte_free_vpe(struct kvm_vcpu *vcpu)

[ ... ]

> +static int vgic_v5_vmte_assign_ist(struct kvm *kvm, phys_addr_t ist_base,
> +				   bool two_level, unsigned int id_bits,
> +				   unsigned int l2sz, unsigned int istsz,
> +				   bool spi_ist)
> +{

[ ... ]

> +	scoped_guard(raw_spinlock_irqsave, &vgic_v5_irs_lock) {
> +		/* Bail if already allocated */
> +		vgic_v5_clean_inval(vmte, sizeof(*vmte));
> +		if (le64_to_cpu(READ_ONCE(vmte->val[section])) &
> +		    GICV5_VMTEL2E_IST_VALID)
> +			return -EINVAL;
> +
> +		WRITE_ONCE(vmte->val[section], cpu_to_le64(tmp));
> +		vgic_v5_clean_inval(vmte, sizeof(*vmte));

[Severity: High]
Does this code inadvertently overwrite concurrent hardware IRS updates?

The struct vmtl2_entry is exactly 32 bytes (__le64 val[4]), meaning two
adjacent entries (e.g., vmte[0] and vmte[1]) share a single 64-byte CPU
cache line. Assigning an IST requires an IRS doorbell operation, which causes
the IRS hardware to write the VALID bit into the VMTE in main memory.

If the host CPU concurrently writes to vmte[1] to configure another VM, the
entire shared cache line becomes dirty in the CPU's cache. When the host
executes vgic_v5_clean_inval() (dc civac), it writes back the entire
dirty cache line to main memory. If the CPU's cache holds a stale copy of
vmte[0] (without the VALID bit set by the IRS), this writeback will
overwrite and destroy the IRS's update to vmte[0].

Should struct vmtl2_entry be padded and aligned to SMP_CACHE_BYTES to
prevent this false-sharing writeback corruption?

> +	}

[ ... ]

> +static int vgic_v5_alloc_linear_ist(struct kvm *kvm, bool spi_ist,
> +				    unsigned int id_bits, unsigned int istsz)
> +{
> +	const size_t n = max(5, id_bits + 1 + istsz);
> +	u32 vm_id = vgic_v5_vm_id(kvm);
> +	struct vgic_v5_vm_info *vmi;
> +	__le64 *ist;
> +	u32 l1sz;
> +
> +	vmi = xa_load(&vm_info, vm_id);
> +	if (!vmi)
> +		return -EINVAL;
> +
> +	/*
> +	 * Allocate the IST. We only have one level, so we just use the L2 ISTE.
> +	 */
> +	l1sz = BIT(n + 1);
> +	ist = kzalloc(l1sz, GFP_KERNEL_ACCOUNT);

[Severity: High]
Can this calculation overflow l1sz and cause a zero-size allocation?

If the hardware IRS reports support for a large LPI ID space (e.g., 32 bits,
idr2.id_bits = 31) but does not support two-level ISTs, the guest can
request 32 bits of LPI IDs. The vgic_v5_alloc_linear_ist() function
calculates n = max(5, 31 + 1 + 3) = 35. The size calculation BIT(n + 1)
evaluates to BIT(36). Because l1sz is typed as a 32-bit integer (u32),
1UL << 36 overflows and truncates to 0, causing kzalloc() to return the
ZERO_SIZE_PTR (0x10).

> +	if (!ist)
> +		return -ENOMEM;
> +
> +	if (spi_ist) {
> +		vmi->h_spi_ist = ist;
> +	} else {
> +		vmi->h_lpi_ist_structure = false;
> +		vmi->h_lpi_ist = ist;
> +	}
> +
> +	vgic_v5_clean_inval(ist, l1sz);

[Severity: High]
If ist is ZERO_SIZE_PTR (0x10) due to the overflow above, doesn't this
cache maintenance operation panic the host?

On arm64, dcache_by_line_op aligns this to 0x0 and executes a dc civac
on address 0x0, which immediately causes an EL1 Translation Fault and a
kernel panic. This appears reachable by an unprivileged guest via the emulated
GICv5 IRS MMIO registers.

> +
> +	return 0;
> +}

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