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

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

In vgic_v5_init() we reject init if we have no vCPUs. We cannot make it
this far without any, so this is not an issue.

> 
> > +	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?

Yes, but it is intentionally so. We rely on the hardware itself setting
the valid bit which is does in response to KVM issuing a command via
the IRS MMIO interface (writing to IRS_VMAP_VISTR).

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

No, GICv5's IRS provides an explicit way of making it valid once the
VMTE is valid (see above). If we were to have the IST valid at the
point where VMTE itself is made valid, then the hardware would pick up
the IST. However, as the VMTE is already valid at the stage where this
happens, we need to do it via the IRS MMIO mechanisms (and ONLY via
those).

> 
> > +	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?

As I said above, we can't make it this far without vCPUs. This cannot
happen.

> 
> > +
> > +	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);

At the very most, GICv5 supports 24 bits of ID space. With KVM, we're
further restricting this to 16 bits (and 10 bits for SPIs as of v5).
This would only go wrong if we had 28 bits of ID space (or more).

> 
> [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?

Yes, this is modified by the hardware, but we don't modify it
ourselves. The only time when we modify the ISTs themselves is when
restoring a VM's state, which we explicitly do in such a way that the
hardware shouldn't be accessing the ISTs. When saving the state, we get
the hardware to serialise the virtual ISTs back to memory (via
IRS_SAVE_VMR), then read this out into userspace buffers.

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

No. We only ever free the ISTs once the VM is already invalid. If it is
invalid, the hardware should not be accessing the memory of these
tables.

> 
> [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?

By design, the VMTE fields will be naturally aligned and we are doing a
64-bit store here, which should happen atomically on arm64. However, as
we are using WRITE_ONCE() elsewhere for this, I've made the change in
order to remain consistent.

> 
> > +	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?

No, as the VM is invalid the hardware should not be accessing the
memory.

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

I've made it a WRITE_ONCE() as I did above.

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

No. The hardware is not permitted to use this until we make it valid,
which we do via the IRS MMIO interface. 

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

Yes it does. In general, I've been a bit worried about false sharing
here. When we have a non-coherent IRS, we want to make sure we are
never in the situation where we accidentally clobber something.

With this current version of the series, each VPED is allocated as part
of populating the VPETE which means that we might have allocations that
share cachelines with completely unrelated allocations. If care is not
taken, this can end up with us clobbering data accidentally.

I've reworked this code to allocate a dense VPED array which is aligned
to whole cachelines. This is then wired into the VPETEs.

As part of this, I've also ensured that the VMD (VM Descriptor) is also
at least one cacheline as we want to avoid potential issues there too.

Thanks,
Sascha