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

On Fri, 2026-07-24 at 11:45 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 9 potential
> issue(s) to consider:
> - [High] Unallocated `vgic_v5_irs_data` leads to a NULL pointer
> dereference on initialization.
> - [High] Bumping `nr_spis` locally in `kvm_vgic_v5_irs_init()`
> without updating `kvm->arch.vgic.nr_spis` leads to inaccessible SPIs.
> - [High] Missing RCU read lock around `xa_load` in
> `vgic_v5_vmte_vpe_id_bits()`.
> - [High] Unconditional IPI storm triggered by guest writes to
> GICV5_IRS_CR0.
> - [High] 64-bit IRS registers cannot be accessed via 32-bit UAPI or
> guest MMIO.
> - [Medium] Unratelimited host kernel log spam via invalid
> IRS_IST_CFGR allows a malicious guest to flood the host log.
> - [Medium] Data races and torn reads on shared VM-wide IRS MMIO
> registers (`ist_baser`, `ist_cfgr`, `cr1`, `pe_selr.iaffid`).
> - [Medium] Underflow in `IAFFID_BITS` calculation exposes bogus IDR1
> values before first vCPU run.
> - [Medium] IST valid state desynchronizes from hardware on free
> failure.
> --
> 
> --- Patch [20]: [PATCH v4 20/48] KVM: arm64: gic-v5: Add GICv5 IRS
> IODEV and MMIO emulation ---
> commit 89cd01e35340acb046b6ed1ef9d6c28883308945
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation
> 
> This commit introduces an IODEV for the GICv5 IRS and an associated
> initialization function to set up SPIs and initial state. The MMIO
> emulation
> tracks selector state and configures LPI ISTs on behalf of the guest.
> 
> > 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..da8914dd682fc
> > --- /dev/null
> > +++ b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> > @@ -62,4 +62,5 @@ static unsigned long
> > vgic_v5_mmio_read_irs_misc(struct kvm_vcpu *vcpu,
> > +		/*
> > +		 * IRS_IDR1 encodes IAFFID_BITS as N - 1.
> > +		 */
> > +		vpe_id_bits = vgic_v5_vmte_vpe_id_bits(vcpu);
> > +		value |= FIELD_PREP(GICV5_IRS_IDR1_IAFFID_BITS,
> > vpe_id_bits - 1);
> 
> [Severity: Medium]
> If userspace reads GICV5_IRS_IDR1 before the VM runs,
> vgic_v5_vmte_vpe_id_bits() returns 0 because the VMTE is not yet
> allocated.

Userspace cannot access these registers until the vgic is initialised.
Therefore, that state is not reachable.

> 
> Does vpe_id_bits - 1 underflow here, exposing an incorrect
> IAFFID_BITS
> value before it is properly initialized?

I think that this was already discussed elsewhere, but this is also not
an issue. The VPE_ID_BITS field in the VMTE doesn't have that -1 offset
that other such registers have. Given that we can't represent fewer
than a single ID bit for IAFFID, we have the same limit in the VPET
code - we always have at least 1 for VPE_ID_BITS. Hence, this doesn't
underflow.

> 
> [ ... ]
> > @@ -174,4 +174,5 @@ static void vgic_v5_mmio_write_irs_misc(struct
> > kvm_vcpu *vcpu, gpa_t addr,
> > +	case GICV5_IRS_CR0:
> > +		mutex_lock(&vcpu->kvm->arch.config_lock);
> > +		WRITE_ONCE(vgic->enabled, !!(val &
> > GICV5_IRS_CR0_IRSEN));
> > +		kvm_make_all_cpus_request(vcpu->kvm,
> > KVM_REQ_RELOAD_GICv5);
> 
> [Severity: High]
> Does this unconditional kick allow a guest to overload physical CPUs
> with
> IPIs? 
> 
> Should this verify if the IRSEN bit actually changed before making
> the all-cpus request?

I've made this conditional on an edge, i.e., only when the value of
vgic->enabled is changed.

> 
> > +		mutex_unlock(&vcpu->kvm->arch.config_lock);
> > +
> > +		return;
> [ ... ]
> > +	case GICV5_IRS_PE_SELR:
> > +		irs->pe_selr.iaffid =
> > FIELD_GET(GICV5_IRS_PE_SELR_IAFFID, val);
> 
> [Severity: Medium]
> Are atomic primitives or locks required when accessing this VM-wide
> state?
> 
> If multiple vCPUs concurrently write irs->pe_selr.iaffid locklessly,
> can it
> lead to torn writes and corrupted hardware emulation state?

No, this would not lead to torn writes. I have, however, moved this to
use READ_ONCE()/WRITE_ONCE() as technically once vCPU could write it
while another is using it. Although this would be poorly written guest
software, we want to be sure to snapshot the value to at least be
consistent with our usage of it.

> 
> > +		return;
> [ ... ]
> > @@ -367,5 +367,6 @@ static unsigned long
> > vgic_v5_mmio_read_irs_ist(struct kvm_vcpu *vcpu,
> > +	case GICV5_IRS_IST_BASER:
> > +		value = FIELD_PREP(GICV5_IRS_IST_BASER_ADDR_MASK,
> > +				   irs->ist_baser.addr >>
> > GICV5_IRS_IST_BASER_ADDR_SHIFT);
> > +		if (irs->ist_baser.valid)
> > +			value |= GICV5_IRS_IST_BASER_VALID;
> 
> [Severity: Medium]
> Similarly to pe_selr, does reading the 64-bit ist_baser locklessly
> risk
> torn reads if another vCPU is concurrently modifying it?

I don't think that we need to do anything here. This would only happen
with broken guest software. Software should not be racing to read/write
the same IRS MMIO region from multiple vCPUs. Just as on real hardware,
we shouldn't provide any guarantees as to what the guest would see
here.

We do need to use the arch_config lock on guest WRITES to the IRS_IST_*
registers as we allocate or free the LPI IST in response to this. For
reads, the guest just gets whatever the most recent state is at the
time the read arrives. Just as it would on real hardware.

> 
> > +		break;
> [ ... ]
> > @@ -414,3 +414,4 @@ static void vgic_v5_mmio_write_irs_ist(struct
> > kvm_vcpu *vcpu, gpa_t addr,
> > +	switch (offset) {
> > +	case GICV5_IRS_IST_CFGR:
> 
> [Severity: High]
> The switch statement checks the base offset, but lacks handling for
> the
> upper 32-bit offset (offset + 4) for 64-bit registers like
> GICV5_IRS_IST_BASER. 
> 
> Will 32-bit guest writes to the upper half hit the default case and
> be
> ignored?

At this stage, 32-bit accesses to 64-bit registers are not supported. A
32-bit write is rejected earlier and doesn't make it this far.

I'll look into addressing this in the future.

> 
> > +		scoped_guard(mutex, &vcpu->kvm->arch.config_lock)
> > {
> > +			if (irs->ist_baser.valid)
> [ ... ]
> > +		/* Valid -> Invalid */
> > +		if (irs->ist_baser.valid && !valid) {
> > +			/* Make the LPI IST invalid and then ...
> > */
> > +			if
> > (irq_set_vcpu_affinity(vgic_v5_vpe_db(vcpu), &cmd))
> > +				break;
> > +
> > +			/*
> > +			 * ... free the host IST if we
> > successfully marked the
> > +			 * IST as invalid. Frankly, if we failed
> > to make the
> > +			 * guest's IST as invalid, we're cooked
> > because it means
> > +			 * that the IRS may still be using the
> > memory that we
> > +			 * want to free. Hence, we leave it
> > allocated and skip
> > +			 * the clearing of valid bit in the baser.
> > +			 */
> > +			if (vgic_v5_lpi_ist_free(vcpu->kvm))
> > +				break;
> 
> [Severity: Medium]
> If vgic_v5_lpi_ist_free() fails after successfully issuing
> LPI_VIST_MAKE_INVALID, the code breaks out of the switch statement
> early.
> 
> Does this leave the guest-visible irs->ist_baser.valid bit out of
> sync with
> the actual hardware state?

I've changed this to update the valid bit. This was already being done
in a later shuffle of this code (when adding support for userspace
accesses), but hadn't been correctly "backported" to this patch.

> 
> > +		} else if (!irs->ist_baser.valid && valid) { /*
> > Invalid -> Valid */
> > +			if (!vgic_v5_ist_cfgr_valid(irs)) {
> > +				kvm_err("Guest programmed invalid
> > IRS_IST_CFGR\n");
> 
> [Severity: Medium]
> Can a malicious guest flood the host kernel log by repeatedly writing
> an
> invalid configuration to GICV5_IRS_IST_BASER? 
> 
> Should this use a ratelimited printk to prevent a denial of service?

I've dropped this print altogether. It was useful for early debugging,
but now if the guest chooses to do something it shouldn't do, then
that's on the guest.

> 
> > +				break;
> > +			}
> [ ... ]
> > @@ -490,4 +490,5 @@ static const struct vgic_register_region
> > vgic_v5_irs_registers[] = {
> > +	REGISTER_DESC_WITH_LENGTH(GICV5_IRS_IST_BASER,
> > vgic_v5_mmio_read_irs_ist,
> > +				  vgic_v5_mmio_write_irs_ist, 8,
> > +				  VGIC_ACCESS_64bit),
> 
> [Severity: High]
> Since GICV5_IRS_IST_BASER is defined exclusively with
> VGIC_ACCESS_64bit,
> how does the 32-bit KVM UAPI access it during VM save and restore? 
> 
> Will those accesses be rejected?

Yes, see my comment above.

> 
> > +	REGISTER_DESC_WITH_LENGTH(GICV5_IRS_IST_CFGR,
> > vgic_v5_mmio_read_irs_ist,
> [ ... ]
> > @@ -603,3 +603,4 @@ int vgic_v5_register_irs_iodev(struct kvm *kvm,
> > gpa_t irs_base_address)
> > +{
> > +	struct vgic_io_device *io_device = &kvm-
> > >arch.vgic.vgic_v5_irs_data->iodev;
> 
> [Severity: High]
> Is vgic_v5_irs_data allocated anywhere before it is dereferenced
> here?

Not in this commit, but this code is unreachable right now.

> 
> Since it doesn't appear to be allocated via kzalloc or kcalloc in
> this
> patch, does this lead to a NULL pointer dereference when accessed?

This code is not reachable in this commit.

> 
> The same dereference appears to occur in kvm_vgic_v5_irs_init() as
> well:
> 
> kvm_vgic_v5_irs_init() {
> 	struct vgic_v5_irs *irs = dist->vgic_v5_irs_data;
>     ...
>     irs->idr0.domain = GICV5_IRS_IDR0_INT_DOM_NON_SECURE;

The subsequent commit in this series plumbs in the allocation and
calling of IRS init.

> 
> > +	unsigned int len;
> [ ... ]
> > @@ -642,4 +642,4 @@ int kvm_vgic_v5_irs_init(struct kvm *kvm,
> > unsigned int nr_spis)
> > +	if (nr_spis && nr_spis < VGIC_V5_DEFAULT_NR_SPIS)
> > +		nr_spis = VGIC_V5_DEFAULT_NR_SPIS;
> 
> [Severity: High]
> If the requested nr_spis is bumped locally to VGIC_V5_DEFAULT_NR_SPIS
> here,
> how does core KVM know about the updated count? 
> 
> Could this prevent the guest from accessing the newly padded SPIs
> since
> kvm->arch.vgic.nr_spis remains un-bumped?

We support a minimum of 32 SPIs, which is set as the default as part of
vgic_v5_init() if userspace has not provided us with something else.
The userspace path filters out all non-multiples of 32.

I've actually dropped this as it is redundant.

> 
> > +
> > +	if (nr_spis) {
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > --- a/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5-tables.c
> > @@ -656,3 +656,4 @@ u8 vgic_v5_vmte_vpe_id_bits(struct kvm_vcpu
> > *vcpu)
> > +
> > +	vmi = xa_load(&vm_info, vm_id);
> 
> [Severity: High]
> Does this XArray lookup need RCU protection? 

No. The vm_info (vmi) is population during vgic_v5_init(). The
vpe_id_bits field is not updated after this point (until we tear down
the VM again).

> 
> Since this is called from the MMIO read handler which runs under kvm-
> >srcu
> but not rcu_read_lock(), could this violate the XArray API contracts
> and
> cause a use-after-free?

No. We're not doing anything like xa_erase().

> 
> > +	if (!vmi)
> > +		return 0;
> 
Thanks,
Sascha