[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

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

[sashiko-bot]

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 <fuad.tabba@linux.dev>

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.

> 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;
> +

[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?

>  	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/20260807104102.2410744-1-fuad.tabba@linux.dev?part=2

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

[Fuad Tabba]

On Fri, 7 Aug 2026 at 11:56, <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

No, thank _you_ Sashiko!

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

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.

Cheers,
/fuad





/fuad

> --
>
> --- Patch [2]: [PATCH v2 2/4] KVM: arm64: vgic: Don't leak the SPI array when init is retried ---
> commit 11b9c8f9971b11e30ffc049a56071fafe4c7df73
> Author: Fuad Tabba <fuad.tabba@linux.dev>
>
> 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.
>
> > 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;
> > +
>
> [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?
>
> >       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/20260807104102.2410744-1-fuad.tabba@linux.dev?part=2

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

[Marc Zyngier]

On Fri, 07 Aug 2026 13:15:52 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> On Fri, 7 Aug 2026 at 11:56, <sashiko-bot@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> No, thank _you_ Sashiko!
> 
> >
> > 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.
> 
> 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.

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.

[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

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.

[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 0/4] KVM: arm64: vgic: Fixes for ITS table save and init retry

[Oliver Upton]

On Fri, 07 Aug 2026 11:40:58 +0100, Fuad Tabba wrote:
> 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)
> 
> [...]

Applied to next, thanks!

[1/4] KVM: arm64: vgic-its: Don't dereference a NULL collection on ITT save
      https://git.kernel.org/kvmarm/kvmarm/c/c6c156d931c3
[2/4] KVM: arm64: vgic: Don't leak the SPI array when init is retried
      https://git.kernel.org/kvmarm/kvmarm/c/52d044d6e76f
[3/4] KVM: arm64: vgic-its: Don't save collections the table cannot hold
      https://git.kernel.org/kvmarm/kvmarm/c/9b10fb74e4b6
[4/4] KVM: arm64: vgic-its: Point saved ITEs at the next valid entry
      https://git.kernel.org/kvmarm/kvmarm/c/ad1e686e2378

--
Best,
Oliver