Re: [PATCH v6 20/49] KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `vgic_irq_is_mapped_level()` check when changing SPI trigger mode allows a guest to reconfigure a hardware-mapped level interrupt to edge-triggered.
- [Medium] Unconditional allocation of `dist->spis` in `kvm_vgic_v5_irs_init` leaks memory if VGIC initialization fails and is retried.
--

commit 7fdccc0d6d5320405b4f8a3add8b9e063ba810fc
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>

KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation

This commit introduces an IODEV for the GICv5 IRS and its associated MMIO
emulation to handle guest accesses. It adds support for querying and
manipulating IRS registers, tracking selector states, and allocating or freeing
host ISTs based on guest IST lifecycle.

> diff --git a/arch/arm64/kvm/vgic/vgic-irs-v5.c b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> new file mode 100644
> index 0000000000000..76f5f40ecc1ce
> --- /dev/null
> +++ b/arch/arm64/kvm/vgic/vgic-irs-v5.c
[ ... ]
> +static void vgic_v5_mmio_write_irs_spi(struct kvm_vcpu *vcpu, gpa_t addr,
> +				       unsigned int len, unsigned long val)
> +{
> +	struct vgic_v5_irs *irs = vgic_v5_get_irs(vcpu);
> +	const size_t offset = addr & (SZ_64K - 1);
> +	struct vgic_irq *irq;
> +
> +	switch (offset) {
> +	case GICV5_IRS_SPI_SELR:
> +		WRITE_ONCE(irs->spi_selr.id,
> +			   FIELD_GET(GICV5_IRS_SPI_SELR_ID, val));
> +		return;
> +	case GICV5_IRS_SPI_CFGR: {
> +		u32 id = READ_ONCE(irs->spi_selr.id);
> +
> +		if (!vgic_v5_is_spi_selr_valid(irs, id))
> +			return;
> +
> +		/*
> +		 * Find KVM's representation of the interrupt - we need to make
> +		 * sure that KVM's view agrees with the guest's, else interrupt
> +		 * injection won't work properly for level-triggered interrupts
> +		 * (we fail to handle the clearing of the pending state if KVM
> +		 * thinks that the interrupt is edge-triggered, which is the
> +		 * default.)
> +		 */
> +		irq = vgic_get_irq(vcpu->kvm, vgic_v5_make_spi(id));
> +		if (!irq)
> +			return;
> +
> +		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
> +			if (val & GICV5_IRS_SPI_CFGR_TM)
> +				irq->config = VGIC_CONFIG_LEVEL;
> +			else
> +				irq->config = VGIC_CONFIG_EDGE;
> +		}

[Severity: High]
Could this assignment allow a guest to reconfigure a hardware-mapped level
interrupt to edge-triggered?

By omitting a check against vgic_irq_is_mapped_level(irq) in
vgic_v5_mmio_write_irs_spi() within arch/arm64/kvm/vgic/vgic-irs-v5.c, this
update unconditionally modifies irq->config when the guest writes to the
GICV5_IRS_SPI_CFGR MMIO register.

For physical interrupts mapped into the guest (e.g., via VFIO), KVM requires
the virtual trigger mode to remain level-triggered to properly manage physical
interrupt deactivation upon guest EOI. In GICv2/v3, vgic_mmio_write_icfgr()
explicitly guards against this.

This could lead to desynchronization of virtual and physical interrupt state
machines, resulting in physical interrupt storms that can lock up the host.

> +
> +		vgic_put_irq(vcpu->kvm, irq);
> +
> +		return;
> +	}
[ ... ]
> +int kvm_vgic_v5_irs_init(struct kvm *kvm, unsigned int nr_spis)
> +{
> +	struct vgic_dist *dist = &kvm->arch.vgic;
> +	struct vgic_v5_irs *irs = dist->vgic_v5_irs_data;
> +	struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);
> +	size_t nr_spi_bits;
> +	u64 mmfr0;
> +	int ret, i;
> +
> +	/*
> +	 * We (KVM) allocate an Interrupt State Table (IST) for SPIs. The
> +	 * hardware mandates that lower 6 bits of the address are 0. Each ISTE
> +	 * is 4 bytes in size (or larger if metadata storage is required), so 16
> +	 * entries would be enough for alignment. We require userspace to
> +	 * provide at least 32 SPIs, so we always meet alignment requirements.
> +	 */
> +	if (nr_spis) {
> +		dist->spis = kcalloc(nr_spis, sizeof(struct vgic_irq),
> +				     GFP_KERNEL_ACCOUNT);
> +		if (!dist->spis)
> +			return -ENOMEM;

[Severity: Medium]
Is it possible for this allocation to permanently leak memory if VGIC
initialization fails and is retried?

In kvm_vgic_v5_irs_init() within arch/arm64/kvm/vgic/vgic-irs-v5.c, the
dist->spis pointer is unconditionally overwritten here. If a failure occurs
later in the initialization sequence (for example, during
kvm_vgic_setup_default_irq_routing()), dist->initialized remains false, which
allows host userspace to retry the KVM_DEV_ARM_VGIC_CTRL_INIT ioctl.

If userspace retries the ioctl, kvm_vgic_v5_irs_init() will be called again and
overwrite the dist->spis pointer without freeing the previous allocation.

> +
> +		/*
> +		 * In the following code we do not take the irq struct lock since
> +		 * no other action on irq structs can happen while the VGIC is
> +		 * not initialized yet.
> +		 */
> +		for (i = 0; i < nr_spis; i++) {
> +			struct vgic_irq *irq = &dist->spis[i];
[ ... ]

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