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

Hi Eric,

On Fri, 14 Aug 2026 at 14:36, Eric Auger <[email protected]> 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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

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