Re: [PATCH v2 1/4] KVM: arm64: vgic-its: Free the caches when GITS_BASER changes

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

Pre-existing issues:
- [High] Pre-existing race condition in ITS MMIO handler allows MSI injection while ITS is disabled due to a stale translation cache entry.
--

--- Patch [1]: [PATCH v2 1/4] KVM: arm64: vgic-its: Free the caches when GITS_BASER changes ---
commit 98c89973da07ed23e9bd8348bfd2cf61333c4e66
Author: Fuad Tabba <[email protected]>

KVM: arm64: vgic-its: Free the caches when GITS_BASER changes

A guest that disables the ITS and changes GITS_BASER<n> with VALID still set
keeps the mapped devices and collections. This patch frees the list and
drops the translation cache whenever the stored value changes.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 9e782a4fea7e5..ab89b0138efd0 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c

[ ... ]

> @@ -1702,6 +1705,8 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  			vgic_its_free_collection_list(kvm, its);
>  			break;
>  		}
> +		/* A concurrent injection may have cached a translation. */
> +		vgic_its_invalidate_cache(its);

[Severity: High]
This is a pre-existing issue, but the commit message explicitly notes that
an injection can still cache an entry after the ITS is disabled.

Is it possible for a subsequent MSI to trigger an unexpected LPI injection?

If Thread 1 (e.g. irqfd) calls vgic_its_inject_msi(), takes its_lock, and
enters vgic_its_resolve_lpi() in arch/arm64/kvm/vgic/vgic-its.c, it reads
its->enabled as true.

If Thread 2 (guest vCPU) concurrently disables the ITS via
vgic_mmio_write_its_ctlr(), it acquires cmd_lock (but not its_lock), sets
its->enabled to false, and clears the cache.

Thread 1 could then resume, finish resolving the LPI, and call
vgic_its_cache_translation() to insert the entry into the cache, ignoring the
fact that its->enabled is now false.

A subsequent MSI would then hit vgic_its_inject_cached_translation(), which
does a fast-path lookup without checking its->enabled, successfully injecting
the MSI.

Could this violate the GICv3 specification by delivering LPIs while the ITS
is explicitly disabled, potentially crashing a guest OS during kexec?

>  		mutex_unlock(&its->its_lock);
>  	}
>  }

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