KVM: arm64: vgic: Fixes for ITS table save and init retry

MAPC with V=0 drops ite->collection but leaves the ITE on the device's ITT list, and vgic_its_save_ite() dereferences it unconditionally. A guest that issues MAPD, MAPTI and then MAPC(V=0) therefore oopses the host when the VMM issues KVM_DEV_ARM_ITS_SAVE_TABLES to migrate it.

That sequence is UNPREDICTABLE per the architecture, but KVM already handles the resulting state in the translate, MOVI and DISCARD paths.

Save a zeroed entry, which vgic_its_restore_ite() reads back as invalid. Skipping the ITE instead would leave the ITT slot holding whatever is in guest memory, and restore rejects an entry naming a collection the restored collection table does not have.

Fixes eff484e0298da ("KVM: arm64: vgic-its: ITT save and restore")

Cc [email protected]

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/kvm/vgic/vgic-its.c | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm64/kvm/vgic/vgic-its.c +0 -0

index 36ab3e4929154..ed281fbf008b9 100644

arch/arm64/kvm/vgic/vgic-its.c +0 -0

 	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
 	       ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |

Hi Fuad,

Quoted 4 lines · Fuad Tabba

MAPC with V=0 drops ite->collection but leaves the ITE on the device's ITT list, and vgic_its_save_ite() dereferences it unconditionally. A guest that issues MAPD, MAPTI and then MAPC(V=0) therefore oopses the host when the VMM issues KVM_DEV_ARM_ITS_SAVE_TABLES to migrate it.

Thank you for fixing this bug!

Quoted 2 lines

That sequence is UNPREDICTABLE per the architecture, but KVM already handles the resulting state in the translate, MOVI and DISCARD paths.

Maybe: "its_is_collection_mapped() already checks the collection is non null in translate, movi and discard"

arch/arm64/kvm/vgic/vgic-its.c static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, · line 2125

Quoted 12 lines

arch/arm64/kvm/vgic/vgic-its.c +4 -0

index 36ab3e4929154..ed281fbf008b9 100644
@@ -2119,6 +2119,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
      u32 next_offset;
      u64 val;
 
+     /*
+      * MAPC with V=0 keeps the ITEs mapped but drops their collection,
+      * and with it the ICID. Save a zeroed entry, which the restore path
+      * reads back as invalid.

in

https://lore.kernel.org/all/CA+EHjTymdanCKoQO3ATmUZ=7Ph2wv4ES8SfkSsTgmOYAYBWayQ@mail.gmail.com/

you also mentionned this can also happen on clearing GITS_BASER<coll>.Valid clear. So maybe don't specifically focus on MAPC in in the comment.

arch/arm64/kvm/vgic/vgic-its.c static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, · line 2128

Quoted 5 lines

arch/arm64/kvm/vgic/vgic-its.c +3 -0

+      */
+     if (!ite->collection)
+             return vgic_its_write_entry_lock(its, gpa, 0ULL, ite);

I would rather squash 4/4 here to emphasize we are unlinking that entry from the scan chain.

Besides I also wonder if we should directly remove those entries from the &device->itt_head on MAPC and GITS_BASER<coll>.Valid reset.

Thanks

Eric> next_offset = compute_next_eventid_offset(&dev->itt_head, ite);

Quoted 2 lines

arch/arm64/kvm/vgic/vgic-its.c +0 -0

      val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
             ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |

Hi Eric,

Quoted 9 lines · Eric Auger

Hi Fuad,

Quoted 4 lines · Fuad Tabba

MAPC with V=0 drops ite->collection but leaves the ITE on the device's ITT list, and vgic_its_save_ite() dereferences it unconditionally. A guest that issues MAPD, MAPTI and then MAPC(V=0) therefore oopses the host when the VMM issues KVM_DEV_ARM_ITS_SAVE_TABLES to migrate it.

Thank you for fixing this bug!

Quoted 4 lines

Quoted 2 lines

That sequence is UNPREDICTABLE per the architecture, but KVM already handles the resulting state in the translate, MOVI and DISCARD paths.

Maybe: "its_is_collection_mapped() already checks the collection is non null in translate, movi and discard"

Agreed, that's more accurate. This went into kvmarm/next on the 8th [1], so it's the comment rather than the commit message I can still fix.

...

Quoted 14 lines

arch/arm64/kvm/vgic/vgic-its.c static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, · line 2125

Quoted 12 lines

arch/arm64/kvm/vgic/vgic-its.c +4 -0

index 36ab3e4929154..ed281fbf008b9 100644
@@ -2119,6 +2119,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
      u32 next_offset;
      u64 val;
 
+     /*
+      * MAPC with V=0 keeps the ITEs mapped but drops their collection,
+      * and with it the ICID. Save a zeroed entry, which the restore path
+      * reads back as invalid.

in

Quoted 5 lines

https://lore.kernel.org/all/CA+EHjTymdanCKoQO3ATmUZ=7Ph2wv4ES8SfkSsTgmOYAYBWayQ@mail.gmail.com/

you also mentionned this can also happen on clearing GITS_BASER<coll>.Valid clear. So maybe don't specifically focus on MAPC in in the comment.

Yes, and the BASER case is the one that justifies the check: MAPC(V=0) is UNPREDICTABLE, so it could be answered by dropping the ITEs, while clearing GITS_BASER<coll>.Valid on a disabled ITS cannot. The comment names only the case that doesn't. I'll send a patch on top to reword it.

Quoted 8 lines

arch/arm64/kvm/vgic/vgic-its.c static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, · line 2128

Quoted 6 lines

arch/arm64/kvm/vgic/vgic-its.c +3 -0

+      */
+     if (!ite->collection)
+             return vgic_its_write_entry_lock(its, gpa, 0ULL, ite);

I would rather squash 4/4 here to emphasize we are unlinking that entry from the scan chain.

1/4 has Cc: stable and 4/4 isn't a fix, so I kept them apart. 4/4

doesn't make the write unnecessary either: an orphaned ITE with no valid predecessor is read directly, regardless of the offsets.

Quoted 3 lines

Besides I also wonder if we should directly remove those entries from the &device->itt_head on MAPC and GITS_BASER<coll>.Valid reset.

For MAPC(V=0) I think it works, and it would stop the event slot being
wedged: an orphaned ITE today can't be DISCARD-ed or remapped with MAPTI. But the ITT entry has to be zeroed with it, the way DISCARD does, or an earlier save's entry stays in the slot and restore fails find_collection() on the destination. Oliver suggested the same on v1 [2].

That pairing is what makes me doubt the BASER reset. There we would be zeroing every ITE of every mapped device from the GITS_BASER write handler, which is an unbounded number of guest memory writes on a register access, and unlike MAPC(V=0) that write isn't UNPREDICTABLE to begin with.

Cheers,
/fuad

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

Quoted 6 lines

Thanks

Eric> next_offset = compute_next_eventid_offset(&dev->itt_head, ite);

Quoted 2 lines

arch/arm64/kvm/vgic/vgic-its.c +0 -0

      val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
             ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] If vgic_init() fails after vgic_v4_init() succeeds, the VPE array is left allocated but vgic_initialized remains false. Userspace can then create additional VCPUs and retry vgic_init(). The second vgic_v4_init() returns early because the VPE array is already allocated, leaving the new VCPUs without initialized VPE IRQs.

When a new VCPU runs, vgic_v4_load() calls irq_set_affinity() with an uninitialized IRQ (0), which fails and triggers a host WARN_ON, causing a host-side denial of service.

--

--- Patch [2]: [PATCH v2 2/4] KVM: arm64: vgic: Don't leak the SPI array when init is retried --- commit 11b9c8f9971b11e30ffc049a56071fafe4c7df73
Author: Fuad Tabba <[email protected]>

KVM: arm64: vgic: Don't leak the SPI array when init is retried

This patch fixes a memory leak where up to 960 struct vgic_irq could be leaked if kvm_vgic_setup_default_irq_routing() fails and userspace retries KVM_DEV_ARM_VGIC_CTRL_INIT. It resolves this by returning early from kvm_vgic_dist_init() if dist->spis is already allocated.

arch/arm64/kvm/vgic/vgic-init.c static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis) · line 215

Quoted 11 lines

arch/arm64/kvm/vgic/vgic-init.c +3 -0

index 907057881b26a..d4cf143f3ae6b 100644
@@ -210,6 +210,9 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
      struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0);
      int i;
 
+     if (dist->spis)
+             return 0;
+

[Severity: High]
This is a pre-existing issue, but since the commit message mentions mirroring the early return behavior of vgic_v4_init(), does that original function have a stale state bug on retry?

If vgic_init() fails after vgic_v4_init() successfully allocates dist->its_vm.vpes, vgic_initialized remains false. If userspace then creates an additional vCPU and retries KVM_DEV_ARM_VGIC_CTRL_INIT, vgic_v4_init() returns early on the second attempt without allocating a VPE for the newly created vCPU.

When the new vCPU runs, vgic_v3_load() unconditionally calls vgic_v4_load(). Because the VPE was never properly initialized for this new vCPU, vgic_v4_load() passes an uninitialized vpe->irq (0) to irq_set_affinity():

arch/arm64/kvm/vgic/vgic-v4.c:vgic_v4_load() {
        ...
        err = irq_set_affinity(vpe->irq, cpumask_of(smp_processor_id()));
        ...
}

This fails with -EINVAL and triggers a WARN_ON in vgic_v3_load():

arch/arm64/kvm/vgic/vgic-v3.c:vgic_v3_load() {
        ...
        WARN_ON(vgic_v4_load(vcpu));
        ...
}

Could an unprivileged VMM repeatedly trigger this host WARN_ON by deliberately failing the first initialization and adding a vCPU before retrying?

Is this something that should be addressed alongside or before this leak fix?

arch/arm64/kvm/vgic/vgic-init.c static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis) · line 217

Quoted 2 lines

arch/arm64/kvm/vgic/vgic-init.c +0 -0

      dist->active_spis = (atomic_t)ATOMIC_INIT(0);
      dist->spis = kzalloc_objs(struct vgic_irq, nr_spis, GFP_KERNEL_ACCOUNT);

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Quoted 2 lines · On Fri, 7 Aug 2026 at 11:56, <[email protected]> wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

No, thank _you_ Sashiko!

Quoted 3 lines

Pre-existing issues:
- [High] If vgic_init() fails after vgic_v4_init() succeeds, the VPE array is left allocated but vgic_initialized remains false. Userspace can then create additional VCPUs and retry vgic_init(). The second vgic_v4_init() returns early because the VPE array is already allocated, leaving the new VCPUs without initialized VPE IRQs.

...

Quoted 1 line

When a new VCPU runs, vgic_v4_load() calls irq_set_affinity() with an uninitialized IRQ (0), which fails and triggers a host WARN_ON, causing a host-side denial of service.

This is a separate fix. But you're right about the state. kvm_arch_vcpu_precreate() only refuses vCPUs once vgic_initialized() is true, so the retry admits one the VPE array does not cover.

I don't think a VMM can drive it. kvm_vgic_setup_default_irq_routing() is the only failure after vgic_v4_init(), and it builds its own routing entries, so nothing short of a real -ENOMEM fails it.

The fix I have in mind keeps vgic_v4_init() idempotent, as this patch
is: return early only while the vCPU count is unchanged, rebuild when it grew. That needs its_free_vcpu_irqs() to clear vm->domain and vm->fwnode first, which it does not today, so its_alloc_vcpu_irqs() can reach its error path with a domain it has already removed.

Let me think about it some more and I'll send something once others have had a chance to digest this as well.

Quoted 21 lines · Fuad Tabba

Quoted 2 lines · On Fri, 7 Aug 2026 at 11:56, <[email protected]> wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

No, thank _you_ Sashiko!

Quoted 3 lines

Pre-existing issues:
- [High] If vgic_init() fails after vgic_v4_init() succeeds, the VPE array is left allocated but vgic_initialized remains false. Userspace can then create additional VCPUs and retry vgic_init(). The second vgic_v4_init() returns early because the VPE array is already allocated, leaving the new VCPUs without initialized VPE IRQs.

...

Quoted 1 line

When a new VCPU runs, vgic_v4_load() calls irq_set_affinity() with an uninitialized IRQ (0), which fails and triggers a host WARN_ON, causing a host-side denial of service.

This is a separate fix. But you're right about the state. kvm_arch_vcpu_precreate() only refuses vCPUs once vgic_initialized() is true, so the retry admits one the VPE array does not cover.

I don't think a VMM can drive it. kvm_vgic_setup_default_irq_routing() is the only failure after vgic_v4_init(), and it builds its own routing entries, so nothing short of a real -ENOMEM fails it.

Quoted 8 lines

The fix I have in mind keeps vgic_v4_init() idempotent, as this patch
is: return early only while the vCPU count is unchanged, rebuild when it grew. That needs its_free_vcpu_irqs() to clear vm->domain and vm->fwnode first, which it does not today, so its_alloc_vcpu_irqs() can reach its error path with a domain it has already removed.

Let me think about it some more and I'll send something once others have had a chance to digest this as well.

I don't think we should grow anything opportunistically.

The GICv4 code expects the number of vcpus to be fixed when first initialised, and if we must fail it one way or another, then everything should be torn down and reinitialised from scratch.

I really don't want to have to think about the intermediate state.

Thanks,

        M.

--
Jazz isn't dead. It just smells funny.

Quoted 1 line

I really don't want to have to think about the intermediate state.

Agreed. I'll drop the idempotent version I sketched and do teardown and reinit instead.

Cheers,
/fuad

Quoted 7 lines

Thanks,

        M.

--
Jazz isn't dead. It just smells funny.

Hi again Marc,

On Mon, 17 Aug 2026 at 19:40, Fuad Tabba <[email protected]> wrote:
...

Quoted 4 lines

Quoted 1 line

I really don't want to have to think about the intermediate state.

Agreed. I'll drop the idempotent version I sketched and do teardown and reinit instead.

Here are my thoughts. Three patches, all fixes.

vgic_init() will undo what it created when it fails, so KVM_DEV_ARM_VGIC_CTRL_INIT is all or nothing and a retry starts from scratch. Same thing you asked Karl for on the redistributor rollback [1].

That drops the count check I sketched. vgic_v4_init()'s early return stays, since vgic_its_create() still calls it for an ITS created after init.

irq-gic-v4.c goes first, as two patches. It removes the domains and frees the fwnodes without clearing the pointers, at four sites, and the teardown above makes that reachable on a path where it was not. The second is a leak in the same error path: when its_alloc_vcpu_sgis() fails partway, the SGI domains already allocated below it are never unwound.

What do you think?

A guest that disables the ITS and rewrites GITS_BASER with fewer pages, VALID still set, keeps every collection it mapped against the larger
table: KVM stores the new BASER unconditionally and frees the list only when VALID is cleared.

vgic_its_save_collection_table() then walks the
whole list, writing up to 448K past the end of the table, and saves collection IDs that vgic_its_restore_cte() rejects, so the save succeeds and the restore fails with -EINVAL on the destination. The overrun stays in guest memory, as vgic_write_guest_lock() validates every gfn.

Validate each collection against the current table with vgic_its_check_id() and return -EINVAL, as vgic_its_save_device_tables() does for devices. Collection IDs are unique and the collection table is never indirect, so the check also bounds the walk.

Quoted 4 lines

A guest that disables the ITS and rewrites GITS_BASER with fewer pages, VALID still set, keeps every collection it mapped against the larger
table: KVM stores the new BASER unconditionally and frees the list only when VALID is cleared.

But isn't that the *real* problem? Shouldn't we instead nuke the collections entirely and go through a reload sequence?

Quoted 4 lines

Validate each collection against the current table with vgic_its_check_id() and return -EINVAL, as vgic_its_save_device_tables() does for devices. Collection IDs are unique and the collection table is never indirect, so the check also bounds the walk.

I think returning -EINVAL here was a mistake, as it aborts the save procedure that userspace should be able to issue reliably, even if that means the state is crap. I don't think we should expand that behaviour any further.

Hi Marc,

Sorry for the slow reply, I was away.

...

Quoted 7 lines

Quoted 4 lines

A guest that disables the ITS and rewrites GITS_BASER with fewer pages, VALID still set, keeps every collection it mapped against the larger
table: KVM stores the new BASER unconditionally and frees the list only when VALID is cleared.

But isn't that the *real* problem? Shouldn't we instead nuke the collections entirely and go through a reload sequence?

Agreed, that is the problem. The fix belongs in vgic_mmio_write_its_baser().

Should it drop everything, or only the collections that no longer fit? On a shrink in place the low ICIDs are still in the table. Or is no realistic guest doing this, so the simpler rule wins? I can do the unconditional drop if you prefer.

Quoted 9 lines

Quoted 4 lines

Validate each collection against the current table with vgic_its_check_id() and return -EINVAL, as vgic_its_save_device_tables() does for devices. Collection IDs are unique and the collection table is never indirect, so the check also bounds the walk.

I think returning -EINVAL here was a mistake, as it aborts the save procedure that userspace should be able to issue reliably, even if that means the state is crap. I don't think we should expand that behaviour any further.

Fair enough. Either way the check goes with the fix rather than being
expanded: MAPC, MAPTI and the restore path already validate against the current table.

vgic_its_save_device_tables() is harder: with an indirect table the guest can invalidate an L1 entry without touching GITS_BASER, so that check can still fire. Skip the device rather than fail the save?

Respinning today since I am travelling until the 17th and will be slow to reply.

Changes since v1 [1]:
- Patch 3: check the collection ID with vgic_its_check_id() instead of bounding the walk by the table size, so a collection the table cannot address is not saved either. Retitled to match. (sashiko)
- New patch 4: skip the ITEs that are saved as invalid when computing an ITE's next-event offset. (Oliver)

[...]