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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Missing hardware invalidation commands allow DMA Use-After-Free.
- [Critical] Integer truncation in `l1sz` calculation leads to zero-sized IST allocation and host memory corruption.
- [High] VPE descriptor pointer is published to hardware before its initialization is visible.
- [High] Null pointer dereference of `vcpu0` in `vgic_v5_vmte_assign_ist`.
- [High] Missing `GICV5_VMTEL2E_IST_VALID` bit leaves IST configurations invalid.
- [High] Unsafe DMA buffer allocations using `kzalloc()` cause cacheline sharing and lost hardware updates.
- [Medium] Missing `WRITE_ONCE` when zeroing the hardware-shared VMTE entry.
--

--- Patch [10]: [PATCH v4 10/48] KVM: arm64: gic-v5: Introduce guest IST alloc and management ---
commit 014722137813a4073b0da36f6aceff4c3e396990
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>

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

GICv5 guests use Interrupt State Tables (ISTs) to track and manage the
interrupt state for SPIs and LPIs. These ISTs are provided to the host's
IRS via the VMTE.

> diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> index 22cd03e0f1963..aa737972f525c 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c

[ ... ]

> @@ -628,3 +671,500 @@ int vgic_v5_vmte_free_vpe(struct kvm_vcpu *vcpu)
>  
>  	return 0;
>  }
> +
> +/*
> + * Assign an already allocated IST to the VM by populating the fields in the
> + * corresponding VMTE. We re-use this code for both an SPI IST and LPI IST, even
> + * if the paths to reach it might be vastly different.
> + */
> +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)
> +{
> +	struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);

[Severity: High]
Could kvm_get_vcpu(kvm, 0) return NULL here if userspace performs VGIC
initialization operations before any vCPUs have been created?

> +	u32 vm_id = vgic_v5_vm_id(kvm);
> +	enum gicv5_vcpu_cmd cmd;
> +	struct vmtl2_entry *vmte;
> +	unsigned int section;
> +	u64 tmp;
> +	int ret;

[ ... ]

> +	tmp = FIELD_PREP(GICV5_VMTEL2E_IST_L2SZ, l2sz);
> +	tmp |= FIELD_PREP(GICV5_VMTEL2E_IST_ADDR,
> +			ist_base >> GICV5_VMTEL2E_IST_ADDR_SHIFT);
> +	tmp |= FIELD_PREP(GICV5_VMTEL2E_IST_ISTSZ, istsz);
> +	tmp |= FIELD_PREP(GICV5_VMTEL2E_IST_ID_BITS, id_bits);
> +	if (two_level)
> +		tmp |= GICV5_VMTEL2E_IST_STRUCTURE;
> +
> +	WRITE_ONCE(vmte->val[section], cpu_to_le64(tmp));

[Severity: High]
Is the GICV5_VMTEL2E_IST_VALID bit missing when constructing this IST entry?

It appears the valid bit is never set in tmp before it is written to the VMTE,
which might leave the configuration architecturally invalid and break interrupt
routing.

> +	vgic_v5_clean_inval(vmte, sizeof(*vmte));
> +
> +	/* 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]
If kvm_get_vcpu(kvm, 0) returned NULL above, does passing it directly to
vgic_v5_vpe_db() here cause a NULL pointer dereference?

> +
> +	return ret;
> +}
> +
> +/*
> + * Allocate a Linear IST - always used for SPIs and potentially LPIs.
> + *
> + * The calculation for n has been taken from section 11.2.2 of the GICv5 EAC0
> + * spec.
> + *
> + * NOTE: istsz is the FIELD used by GICv5, not the actual size (or log2() of the
> + * size).
> + */
> +static int vgic_v5_alloc_linear_ist(struct kvm *kvm, bool spi_ist,
> +				    unsigned int id_bits, unsigned int istsz)
> +{
> +	const size_t n = 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);

[Severity: Critical]
Could the 32-bit truncation of l1sz evaluate to zero for large values of n
(such as when a guest requests a large LPI ID space)?

If n + 1 >= 32, the 64-bit BIT() macro result will be truncated to exactly 0
when assigned to the u32 l1sz. This would result in a zero-sized allocation
that returns ZERO_SIZE_PTR, potentially leading to host memory corruption.

> +	ist = kzalloc(l1sz, GFP_KERNEL_ACCOUNT);

[Severity: High]
Is this memory structure modified by the non-coherent GIC hardware?

If so, does allocating it with kzalloc() risk cacheline sharing with adjacent
slab objects, potentially causing lost hardware updates during CPU cache
eviction?

[ ... ]

> +/*
> + * Free a Linear IST. Can only happen once the VM is dead.
> + */
> +static int vgic_v5_linear_ist_free(struct kvm *kvm, bool spi)
> +{

[ ... ]

> +	/* The VM should be dead here, so we can just zero the VMT section */
> +	vmte->val[section] = cpu_to_le64(0);

[Severity: Critical]
Does freeing the backing memory and zeroing the VMTE without first issuing
hardware invalidation commands allow the hardware to continue DMAing into
freed slab objects?

[Severity: Medium]
Additionally, since this is a hardware-shared data structure, does clearing
the VMTE entry with a plain assignment risk store tearing? Should this use
WRITE_ONCE() to ensure an atomic 64-bit write?

> +	vgic_v5_clean_inval(vmte, sizeof(*vmte));
> +
> +	return 0;
> +}
> +
> +/*
> + * Free a Two-Level IST. Can only happen once the VM is dead.
> + */
> +static int vgic_v5_two_level_ist_free(struct kvm *kvm, bool spi)
> +{

[ ... ]

> +	/* The VM must be dead, so we can just zero the VMT section */
> +	vmte->val[section] = cpu_to_le64(0);

[Severity: Critical]
Does freeing the backing memory and zeroing the VMTE without first issuing
hardware invalidation commands allow the hardware to continue DMAing into
freed slab objects?

[Severity: Medium]
Similarly, does clearing the VMTE entry with a plain assignment risk store
tearing here?

> +	vgic_v5_clean_inval(vmte, sizeof(*vmte));
> +
> +	return 0;
> +}

Looking at vgic_v5_vmte_alloc_vpe() from the broader context of this file:

vgic_v5_vmte_alloc_vpe() {
    ...
    vped = kzalloc(vmt_info->vped_size, GFP_KERNEL);
    ...
    tmp = FIELD_PREP(GICV5_VPED_ADDR, virt_to_phys(vped) >> GICV5_VPED_ADDR_SHIFT);
    WRITE_ONCE(vpet_base[vpe_id], cpu_to_le64(tmp));

    vgic_v5_clean_inval(vped, vmt_info->vped_size);
    ...
}

[Severity: High]
Is it possible for the hardware to observe the VPE descriptor pointer before
the zeroed memory of the descriptor is flushed to the Point of Coherency?

If a CPU cache eviction writes the pointer to main memory before the cache
clean, non-coherent hardware might consume uninitialized memory.

[Severity: High]
Additionally, since vped is allocated with kzalloc(), does this risk cacheline
sharing with adjacent objects, potentially leading to lost hardware updates?

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