[PATCH v2 0/4] KVM: arm64: vgic: Fixes for ITS table save and init retry

[PATCH v2 0/4] KVM: arm64: vgic: Fixes for ITS table save and init retry

[Fuad Tabba]

Hi folks,

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)

Three unrelated vgic fixes, and one follow-up to the first of them.

The first is the one that matters the most. A guest that issues MAPD,
MAPTI and then MAPC(V=0) leaves an ITE mapped with no collection, and
the next KVM_DEV_ARM_ITS_SAVE_TABLES from the VMM oopses the host. That
is three ITS commands and an ordinary migration save, with no race and
no allocation failure.

The other two are weaker and come after it. The init fix stops the SPI
array leaking when userspace retries KVM_DEV_ARM_VGIC_CTRL_INIT after a
failure. The collection table fix stops the save writing past the end of
the table, and stops it saving a collection the destination will reject.

Patch 4 is not a fix. It makes the restore scan skip the invalid entries
patch 1 writes instead of stepping through them one at a time.

Patch 1 is unchanged. Oliver also suggested deleting the ITEs when their
collection goes away, but I do not think it removes the need for the
check here: vgic_its_free_collection_list() reaches the same state with
the ITTs intact, and that path is not UNPREDICTABLE.

Patches 1 and 3 are reproduced, each with a selftest that fails on an
unfixed kernel and passes on a fixed one. Those are not part of this
series, happy to post them separately if they are worth having.

Based on Linux 7.2-rc6 (075b74841bd00).

Cheers,
/fuad

[1] https://lore.kernel.org/all/20260805093828.3626610-1-fuad.tabba@linux.dev/

Fuad Tabba (4):
  KVM: arm64: vgic-its: Don't dereference a NULL collection on ITT save
  KVM: arm64: vgic: Don't leak the SPI array when init is retried
  KVM: arm64: vgic-its: Don't save collections the table cannot hold
  KVM: arm64: vgic-its: Point saved ITEs at the next valid entry

 arch/arm64/kvm/vgic/vgic-init.c |  3 +++
 arch/arm64/kvm/vgic/vgic-its.c  | 26 +++++++++++++++++++-------
 2 files changed, 22 insertions(+), 7 deletions(-)


base-commit: 075b74841bd0065a3bda3440873c747938e69b68
-- 
2.39.5

[PATCH v2 1/4] KVM: arm64: vgic-its: Don't dereference a NULL collection on ITT save

[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.
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: stable@vger.kernel.org
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 36ab3e4929154..ed281fbf008b9 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -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.
+	 */
+	if (!ite->collection)
+		return vgic_its_write_entry_lock(its, gpa, 0ULL, ite);
+
 	next_offset = compute_next_eventid_offset(&dev->itt_head, ite);
 	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
 	       ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |
-- 
2.39.5

[PATCH v2 2/4] KVM: arm64: vgic: Don't leak the SPI array when init is retried

[Fuad Tabba]

Nothing latches a failed vgic_init(), so userspace can retry
KVM_DEV_ARM_VGIC_CTRL_INIT after a failure past kvm_vgic_dist_init().
kvm_vgic_setup_default_irq_routing() is the reachable case, running on
every configuration. Each retry overwrites dist->spis and only the last
allocation is freed at teardown, leaking up to 960 struct vgic_irq,
about 90KB, per attempt.

Return early when the array is already allocated, as
vgic_allocate_private_irqs_locked() and vgic_v4_init() do.

Fixes: ad275b8bb1e65 ("KVM: arm/arm64: vgic-new: vgic_init: implement vgic_init")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 907057881b26a..d4cf143f3ae6b 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -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;
+
 	dist->active_spis = (atomic_t)ATOMIC_INIT(0);
 	dist->spis = kzalloc_objs(struct vgic_irq, nr_spis, GFP_KERNEL_ACCOUNT);
 	if (!dist->spis)
-- 
2.39.5

[PATCH v2 3/4] KVM: arm64: vgic-its: Don't save collections the table cannot hold

[Fuad Tabba]

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.

Fixes: ea1ad53e1e31a ("KVM: arm64: vgic-its: Collection table save/restore")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index ed281fbf008b9..1589f06e66904 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2540,6 +2540,9 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
 	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 
 	list_for_each_entry(collection, &its->collection_list, coll_list) {
+		if (!vgic_its_check_id(its, baser, collection->collection_id, NULL))
+			return -EINVAL;
+
 		ret = vgic_its_save_cte(its, collection, gpa);
 		if (ret)
 			return ret;
-- 
2.39.5

[PATCH v2 4/4] KVM: arm64: vgic-its: Point saved ITEs at the next valid entry

[Fuad Tabba]

An ITE whose collection was dropped is saved as an invalid entry, and
vgic_its_restore_ite() has no offset to follow from one, so the scan
steps a single entry at a time until it reaches a valid entry or the
end of the ITT.

Compute the offset to the next ITE that is saved as valid instead.

Suggested-by: Oliver Upton <oupton@kernel.org>
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 1589f06e66904..9e782a4fea7e5 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2035,15 +2035,16 @@ static u32 compute_next_devid_offset(struct list_head *h,
 
 static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
 {
-	struct its_ite *next;
-	u32 next_offset;
+	struct its_ite *next = ite;
 
-	if (list_is_last(&ite->ite_list, h))
-		return 0;
-	next = list_next_entry(ite, ite_list);
-	next_offset = next->event_id - ite->event_id;
+	/* Point at the next ITE that vgic_its_save_ite() stores as valid. */
+	list_for_each_entry_continue(next, h, ite_list) {
+		if (next->collection)
+			return min_t(u32, next->event_id - ite->event_id,
+				     VITS_ITE_MAX_EVENTID_OFFSET);
+	}
 
-	return min_t(u32, next_offset, VITS_ITE_MAX_EVENTID_OFFSET);
+	return 0;
 }
 
 /**
-- 
2.39.5

Re: [PATCH v2 3/4] KVM: arm64: vgic-its: Don't save collections the table cannot hold

[Marc Zyngier]

On Fri, 07 Aug 2026 11:41:01 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> 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?

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

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.

	M.

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