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

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.

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

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

[Eric Auger]

Hi Fuad,

On 8/7/26 12:40 PM, Fuad Tabba wrote:
> 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!
> 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"
> 
> 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.
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.


> +	 */
> +	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);
>  	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
>  	       ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |

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

[Fuad Tabba]

Hi Eric,

On Fri, 14 Aug 2026 at 14:36, Eric Auger <eauger@redhat.com> wrote:
>
> Hi Fuad,
>
> On 8/7/26 12:40 PM, Fuad Tabba wrote:
> > 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!
> > 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.

...

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

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.

>
>
> > +      */
> > +     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.

> 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/178621413198.380376.1272590130654213471.b4-ty@kernel.org/
[2] https://lore.kernel.org/all/anQQ3dcbk72N1g1a@kernel.org/

> Thanks
>
> Eric>   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) |
>

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

[Eric Auger]

Hi Fuad,

On 8/15/26 2:18 AM, Fuad Tabba wrote:
> Hi Eric,
> 
> On Fri, 14 Aug 2026 at 14:36, Eric Auger <eauger@redhat.com> wrote:
>>
>> Hi Fuad,
>>
>> On 8/7/26 12:40 PM, Fuad Tabba wrote:
>>> 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!
>>> 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.
sorry I did not notice. That's not a big deal either.
> 
> ...
> 
>>> 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.
>> 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.
> 
> 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.
OK
> 
>>
>>
>>> +      */
>>> +     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
ok makes sense
> doesn't make the write unnecessary either: an orphaned ITE with no
> valid predecessor is read directly, regardless of the offsets.
indeed

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

Yes effectively this would imply to null the associated ITEs... On the
other end, overwriting GITS_BASER<n>  while V already set looks rather
far fetched but well ...

Thanks

Eric
> Cheers,
> /fuad
> 
> [1] https://lore.kernel.org/all/178621413198.380376.1272590130654213471.b4-ty@kernel.org/
> [2] https://lore.kernel.org/all/anQQ3dcbk72N1g1a@kernel.org/
> 
>> Thanks
>>
>> Eric>   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) |
>>
> 

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

[Fuad Tabba]

Hi Marc,

Sorry for the slow reply, I was away.

On Sat, 8 Aug 2026 at 09:09, Marc Zyngier <maz@kernel.org> 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?

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.


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

Cheers,
/fuad

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 16:58, Marc Zyngier <maz@kernel.org> wrote:
>
> 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.

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

Cheers,
/fuad

>
> Thanks,
>
>         M.
>
> --
> Jazz isn't dead. It just smells funny.

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

[Fuad Tabba]

Hi again Marc,

On Mon, 17 Aug 2026 at 19:40, Fuad Tabba <fuad.tabba@linux.dev> wrote:
...
> > 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?
/fuad

[1] https://lore.kernel.org/all/86jypv1lxo.wl-maz@kernel.org/
>
> Cheers,
> /fuad
>
> >
> > Thanks,
> >
> >         M.
> >
> > --
> > Jazz isn't dead. It just smells funny.

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

[Marc Zyngier]

On Mon, 17 Aug 2026 19:21:24 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> Hi Marc,
> 
> Sorry for the slow reply, I was away.

No worries.

> 
> On Sat, 8 Aug 2026 at 09:09, Marc Zyngier <maz@kernel.org> 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?
> 
> 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.

Whatever is the simplest thing. I don't think it is common to update
the BASER registers (Linux certainly does it exactly once), and if
that means there will be a small hickup on things like kexec handover,
so be it.

Thinking of it a bit more: the format of the table is not architected.
So a write to the register with a different value is allowed to result
in data loss (for all the guest knows, this could be a linked list
starting from the last page...). So purging all collections (and
therefore all mappings) would be acceptable, as there no guarantee
this is now fit for purpose.

This would make both save and restore succeed, and be really quick!

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

Skipping the devices that fall into an unmapped page looks fine. After
all, the guest is messing with itself, and I have no sympathy for it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Marc Zyngier]

On Tue, 18 Aug 2026 14:22:09 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> Hi again Marc,
> 
> On Mon, 17 Aug 2026 at 19:40, Fuad Tabba <fuad.tabba@linux.dev> wrote:
> ...
> > > 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?

Hard to picture when described like this. Please post the patches!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.