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

[Eric Auger <eauger@redhat.com>]

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