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

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Fri, 2026-07-03 at 16:34 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] Integer truncation of IST allocation size causes zero-sized
> allocations and hardware memory corruption.
> - [Low] Endianness type mismatch when parsing VMTE state using
> `FIELD_GET()`.
> --
> 
> commit 1bc57da799f1125b47ee47d0156802cc04200696
> 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. This commit provisions memory for
> managing a guest's SPI state and shadow LPI ISTs passed to the host
> IRS.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > index db8cb6a384994..79b305d2d5097 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> 
> [ ... ]
> 
> > +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);
> > +	ist = kzalloc(l1sz, GFP_KERNEL_ACCOUNT);
> 
> [Severity: High]
> Can this cause integer truncation and zero-sized allocations if a
> guest
> requests a large LPI ID space?
> 
> In vgic_v5_alloc_linear_ist(), the allocation size is calculated as
> BIT(n + 1)
> and stored in a 32-bit u32 l1sz. The value n is derived from id_bits
> + 1 +
> istsz. If a guest requests a large LPI ID space (e.g., id_bits = 31)
> on a
> system that falls back to linear ISTs, n evaluates to 34.
> 
> BIT(35) evaluates to a 64-bit value that is silently truncated to 0
> when
> assigned to l1sz. The subsequent kzalloc(0) succeeds but returns
> ZERO_SIZE_PTR. KVM then blindly passes the invalid pseudo-physical
> address to
> the hardware VMTE, which can lead to severe host memory corruption or
> IOMMU
> faults when the hardware routes virtual interrupts.

No, this is not an issue.

1. The architecture "only" supports up to 24 ID bits.
2. We limit a guest to at most 16 LPI ID Bits (or fewer if the hardware
itself gives us a lower limit than that).
3. We check the guest-provided ID bits against the limits we've exposed
to the guest in the IRS_IDx registers.

> 
> [ ... ]
> 
> > +static int vgic_v5_two_level_ist_free(struct kvm *kvm, bool spi)
> > +{
> > +	unsigned int id_bits, istsz, l2sz;
> > +	u32 vm_id = vgic_v5_vm_id(kvm);
> > +	struct vgic_v5_vm_info *vmi;
> > +	struct vmtl2_entry *vmte;
> > +	__le64 tmp;
> > +	int section;
> > +
> > +	/* We don't create two-level SPI ISTs, so freeing is a bad
> > idea! */
> > +	if (spi)
> > +		return -EINVAL;
> > +
> > +	vmi = xa_load(&vm_info, vm_id);
> > +	if (!vmi)
> > +		return -EINVAL;
> > +
> > +	section = GICV5_VMTEL2_LPI_SECTION;
> > +
> > +	if (!vmi->h_lpi_ist_structure)
> > +		return -EINVAL;
> > +
> > +	vmte = vgic_v5_get_l2_vmte(vm_id);
> > +	if (IS_ERR(vmte))
> > +		return PTR_ERR(vmte);
> > +
> > +	tmp = le64_to_cpu(READ_ONCE(vmte->val[section]));
> > +
> > +	id_bits = FIELD_GET(GICV5_VMTEL2E_IST_ID_BITS, tmp);
> 
> [Severity: Low]
> This isn't a bug, but does this cause sparse warnings due to an
> endianness
> type mismatch?
> 
> In vgic_v5_two_level_ist_free(), le64_to_cpu() is used to convert the
> VMTE
> entry to native endianness. However, the result is assigned to tmp,
> which is
> declared as __le64. The native value is then passed into FIELD_GET(),
> which
> expects a native unsigned integer type. While this compiles and
> functions
> identically to u64 because __le64 is internally an unsigned 64-bit
> integer,
> it breaks endianness semantic contracts.
> 
Ah, yes. This is an issue. Changed to use u64, not __le64.

Thanks,
Sascha