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@xxxxxxxxxxxxxxx
> Signed-off-by: Fuad Tabba <fuad.tabba@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxx/

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