Re: [PATCH] KVM: arm64: vgic-its: Fix O(C*I) loop in vgic_its_free_collection_list

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Unconditional dereference of `ite->collection` during ITS table save leads to host kernel panic.
- [High] Unmitigated O(Total ITEs) loops during ITS command processing lead to guest-triggered soft lockups and RCU stalls.
--

--- Patch [1]: [PATCH] KVM: arm64: vgic-its: Fix O(C*I) loop in vgic_its_free_collection_list ---
commit 57034db6ba413048ab15ade32e11d1ba6a290388
Author: Jing Zhang <[email protected]>

KVM: arm64: vgic-its: Fix O(C*I) loop in vgic_its_free_collection_list

When destroying the vgic-its collection list,
vgic_its_free_collection_list() iterates over every collection and for
each, calls vgic_its_free_collection(). This function walks every
Interrupt Translation Entry (ITE) across all devices via
for_each_lpi_its() to nullify the collection pointer.

[ ... ]

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 4477f870c7b36..5e907a34226e3 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -1122,9 +1122,21 @@ static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
>  static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
>  {
>  	struct its_collection *cur, *temp;
> +	struct its_device *device;
> +	struct its_ite *ite;
>  
> -	list_for_each_entry_safe(cur, temp, &its->collection_list, coll_list)
> -		vgic_its_free_collection(its, cur->collection_id);
> +	/*
> +	 * Bulk-clear the collection pointers for all ITEs.
> +	 * This transforms the teardown complexity from O(Collections * ITEs)
> +	 * to O(Collections + ITEs), avoiding guest-triggered host RCU stalls.
> +	 */

[Severity: High]
This isn't a bug introduced by this patch, but does this fix cover the same RCU
stall vulnerability in the ITS command ring processing?

A guest can queue up to 32,768 MAPC commands to remap an existing collection,
which vgic_its_process_commands() processes sequentially under cmd_lock
without yielding. For each command, update_affinity_collection() loops over
all ITEs:

arch/arm64/kvm/vgic/vgic-its.c:update_affinity_collection() {
	for_each_lpi_its(device, ite, its) {
		if (ite->collection != coll)
			continue;

		update_affinity_ite(kvm, ite);
	}
}

With a large number of ITEs, can this unmitigated loop still cause soft lockups
and RCU stalls on the host?

> +	for_each_lpi_its(device, ite, its)
> +		ite->collection = NULL;

[Severity: Critical]
This is a pre-existing issue, but can setting ite->collection to NULL here
cause a kernel panic during state saving?

If a guest clears GITS_BASER1 (triggering this teardown) or unmaps a collection
via MAPC with valid=0, the ITEs are left in the device list with a NULL
collection pointer.

Later, if host userspace triggers VM state saving, vgic_its_save_ite() blindly
dereferences ite->collection:

arch/arm64/kvm/vgic/vgic-its.c:vgic_its_save_ite() {
	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
	       ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) |
		ite->collection->collection_id;
}

Does vgic_its_save_ite() need to verify ite->collection is non-NULL before
attempting to access its collection_id?

> +
> +	list_for_each_entry_safe(cur, temp, &its->collection_list, coll_list) {
> +		list_del(&cur->coll_list);
> +		kfree(cur);
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1