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

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

[Fuad Tabba]

Hi folks,

Three unrelated vgic fixes.

These came out of reviewing the GIC-related postings on the list lately,
from Sebastian's ITS hardening series [1] to Sascha's GICv5 IRS series
[2]. These three are from the ITS table save paths and vgic init. Stare
into the GIC long enough and it stares back!

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 bounds a walk that a guest can drive
past the end of the table KVM computed, which stays inside guest
memory, so the guest only corrupts itself. Its error code is a
judgement call, and the commit message says which one and why.

The first and the last 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.
The init leak needs an observed allocation failure rather than a crash,
so there is no reproducer for it.

Based on Linux 7.2-rc6 (075b74841bd00).

Cheers,
/fuad

[1] https://lore.kernel.org/all/20260310124933.830025-1-sebastianene@google.com/
[2] https://lore.kernel.org/all/20260724104819.1296803-1-sascha.bischoff@arm.com/

Fuad Tabba (3):
  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 write past the end of the collection table

 arch/arm64/kvm/vgic/vgic-init.c |  3 +++
 arch/arm64/kvm/vgic/vgic-its.c  | 11 +++++++++++
 2 files changed, 14 insertions(+)


base-commit: 075b74841bd0065a3bda3440873c747938e69b68
-- 
2.39.5

[PATCH 1/3] 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

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

[Oliver Upton]

Hi Fuad,

Thanks for fixing this.

On Wed, Aug 05, 2026 at 10:38:26AM +0100, Fuad Tabba wrote:
> @@ -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);
> +

Writing a zero entry results in a suboptimal scan on the target,
compute_next_eventid_offset() should just skip ITEs that are known
invalid. Or better yet, we should just make the ITS emulation consistent
with the migration behavior and delete ITEs when their corresponding
collection is deleted. As you note, this is UNPRED after all :)

Thanks,
Oliver

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

[Fuad Tabba]

Hi Oliver,

On Thu, 6 Aug 2026 at 05:43, Oliver Upton <oupton@kernel.org> wrote:
>
> Hi Fuad,
>
> Thanks for fixing this.
>
> On Wed, Aug 05, 2026 at 10:38:26AM +0100, Fuad Tabba wrote:
> > @@ -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);
> > +
>
> Writing a zero entry results in a suboptimal scan on the target,
> compute_next_eventid_offset() should just skip ITEs that are known
> invalid.

Agreed, I can do that on top on the respin (I still owe Sashiko one
for patch 3/3 [1]). The predecessor's next_offset still points at the
zeroed slot today, so the restore crawls from there to the next valid
ITE.

I don't think it replaces the write, though. Restore starts at event 0
and walks forward until it finds a valid entry, so an orphaned ITE at
or before that point is read directly, with no predecessor to skip it.
Leave the slot alone and an earlier save's entry is still sitting in
it: restore fails find_collection() and the migration dies with
-EINVAL on the destination. Zeroing is what DISCARD already does.

> Or better yet, we should just make the ITS emulation consistent
> with the migration behavior and delete ITEs when their corresponding
> collection is deleted. As you note, this is UNPRED after all :)

Happy to do that for MAPC(V=0), but I think it might not remove the
need for the check. vgic_its_free_collection() has a second caller:
with the ITS disabled, clearing GITS_BASER<coll>.Valid frees every
collection through vgic_its_free_collection_list() and NULLs
ite->collection, while the device list and the ITTs survive and
vgic_its_save_device_tables() only looks at the device BASER. I have a
selftest that oopses in vgic_its_save_ite() that way, with no MAPC in
it. And that one isn't UNPRED, so I'm not sure we can answer it by
deleting ITEs and zeroing their ITTs.

Cheers,
/fuad

[1] https://lore.kernel.org/all/CA+EHjTxuYLBwNsDnZd4B_jYWBDvGxXMWSDKPiMqjJjdCMYySaw@mail.gmail.com/

>
> Thanks,
> Oliver

[PATCH 2/3] 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 3/3] KVM: arm64: vgic-its: Don't write past the end of the collection table

[Fuad Tabba]

vgic_its_save_collection_table() computes max_size but uses it only to
decide whether to append a terminator, leaving the walk over
its->collection_list unbounded. 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, because KVM stores the
new BASER unconditionally and frees the list only when VALID is
cleared. A save then writes up to 448K past the end of the table.

The writes stay in guest memory, as vgic_write_guest_lock() validates
every gfn, so the guest only corrupts itself. Stop at the boundary
regardless and return -EINVAL, which is what
vgic_its_save_device_tables() returns when a device falls outside its
own table. -ENOSPC describes the condition better, but -EINVAL is
already in the error set documented for KVM_DEV_ARM_VGIC_GRP_CTRL,
and -ENOSPC is not.

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..c709fc5e17cda 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 (filled == max_size)
+			return -EINVAL;
+
 		ret = vgic_its_save_cte(its, collection, gpa);
 		if (ret)
 			return ret;
-- 
2.39.5

Re: [PATCH 3/3] KVM: arm64: vgic-its: Don't write past the end of the collection table

[sashiko-bot]

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

Pre-existing issues:
- [Medium] vgic_its_save_collection_table() saves collections with IDs that exceed the current GITS_BASER size, generating an invalid migration state that fails on restore.
--

--- Patch [3]: [PATCH 3/3] KVM: arm64: vgic-its: Don't write past the end of the collection table ---
commit d0eca2d07d6d3c4d13e7fff1329f9d14363bb0f5
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: vgic-its: Don't write past the end of the collection table

This commit limits the number of collection table entries saved during
migration to avoid writing past the end of the guest memory table. It
aborts the save operation with -EINVAL if the guest has mapped more
collections than fit in the current table.

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index ed281fbf008b9..c709fc5e17cda 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 (filled == max_size)
> +			return -EINVAL;
> +
>  		ret = vgic_its_save_cte(its, collection, gpa);

[Severity: Medium]
This is a pre-existing issue, but does this code save collections with IDs
that exceed the current GITS_BASER size?

If a guest maps a collection with a large ID, and then shrinks the table size
in GITS_BASER while keeping VALID=1, the collection list isn't freed. Since
the total number of mapped collections might still be small enough to pass the
filled == max_size check, it appears the collection with the large ID will
be successfully saved here.

On the destination side, when vgic_its_restore_cte() reads this entry, it
calls vgic_its_check_id() which checks the ID against the shrunken boundaries,
returning -EINVAL and failing the migration.

Should this path also validate the collection ID against the current table
size, similar to how vgic_its_save_device_tables() uses vgic_its_check_id()?

>  		if (ret)
>  			return ret;
>  		gpa += cte_esz;
>  		filled += cte_esz;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805093828.3626610-1-fuad.tabba@linux.dev?part=3

Re: [PATCH 3/3] KVM: arm64: vgic-its: Don't write past the end of the collection table

[Fuad Tabba]

Sashiko my good LLM friend,

On Wed, 5 Aug 2026 at 10:55, <sashiko-bot@kernel.org> wrote:
...
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index ed281fbf008b9..c709fc5e17cda 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 (filled == max_size)
> > +                     return -EINVAL;
> > +
> >               ret = vgic_its_save_cte(its, collection, gpa);
>
> [Severity: Medium]
> This is a pre-existing issue, but does this code save collections with IDs
> that exceed the current GITS_BASER size?
>
> If a guest maps a collection with a large ID, and then shrinks the table size
> in GITS_BASER while keeping VALID=1, the collection list isn't freed. Since
> the total number of mapped collections might still be small enough to pass the
> filled == max_size check, it appears the collection with the large ID will
> be successfully saved here.
>
> On the destination side, when vgic_its_restore_cte() reads this entry, it
> calls vgic_its_check_id() which checks the ID against the shrunken boundaries,
> returning -EINVAL and failing the migration.

You're right, vgic_its_check_id() validates the collection ID against
both the 16-bit maximum and the current table capacity, so it's the
better check here:

+        if (!vgic_its_check_id(its, baser, collection->collection_id, NULL))
+            return -EINVAL;

This also bounds the walk, and the filled == max_size check can go.
I'll switch to it in the respin.

Cheers,
/fuad

>
> Should this path also validate the collection ID against the current table
> size, similar to how vgic_its_save_device_tables() uses vgic_its_check_id()?
>
> >               if (ret)
> >                       return ret;
> >               gpa += cte_esz;
> >               filled += cte_esz;
> >       }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260805093828.3626610-1-fuad.tabba@linux.dev?part=3