Re: [PATCH 3/3] KVM: arm64: vgic-its: Don't read the pending table when mapping an LPI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: arm64: vgic-its: its_sync_lpi_pending_table() reads unrelated LPIs
- [Medium] KVM: arm64: vgic-its: uncleaned queued LPIs in vgic_its_restore_tables_v0()
--

commit 5977878e13acf988213f1898fd84a41cd479ed52
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: vgic-its: Don't read the pending table when mapping an LPI

This commit stops reading the redistributor LPI pending table when mapping an
LPI in vgic_add_lpi() and removes vgic_v3_lpi_sync_pending_status(). Instead,
it synchronizes the pending tables for all vCPUs with LPIs enabled at the end
of vgic_its_restore_tables_v0() via its_sync_lpi_pending_table().

> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 5ce485485a117..ede205e05dc04 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c

[ ... ]

> @@ -2710,6 +2704,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>   */
>  static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  {
> +	struct kvm *kvm = its->dev->kvm;
> +	struct kvm_vcpu *vcpu;
> +	unsigned long i;
>  	int ret;
>  
>  	ret = vgic_its_restore_collection_table(its);
> @@ -2717,9 +2714,25 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  		return ret;
>  
>  	ret = vgic_its_restore_device_tables(its);
> -	if (ret)
> -		vgic_its_free_collection_list(its->dev->kvm, its);
> -	return ret;
> +	if (ret) {
> +		vgic_its_free_collection_list(kvm, its);
> +		return ret;
> +	}
> +
> +	/* Enabling LPIs read the pending tables before these LPIs existed. */
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (!vgic_lpis_enabled(vcpu))
> +			continue;
> +
> +		ret = its_sync_lpi_pending_table(vcpu);

[Severity: Medium]
Can calling its_sync_lpi_pending_table() here cause KVM_DEV_ARM_ITS_RESTORE_TABLES
to fail with -EFAULT when a vCPU with LPIs enabled has a pending table that
does not cover LPI INTIDs targeting other vCPUs or unmapped collections?

Previously, vgic_add_lpi() called vgic_v3_lpi_sync_pending_status(), which
checked irq->target_vcpu (returning 0 if NULL) and only read the pending table
of the redistributor that the LPI actually targeted.

In contrast, its_sync_lpi_pending_table() iterates over every LPI in
dist->lpi_xa and reads from the current vCPU's pendbase before checking
whether the LPI targets that vCPU:

arch/arm64/kvm/vgic/vgic-its.c:its_sync_lpi_pending_table() {
	xa_for_each(&dist->lpi_xa, intid, irq) {
		...
		if (byte_offset != last_byte_offset) {
			ret = kvm_read_guest_lock(vcpu->kvm,
						  pendbase + byte_offset,
						  &pendmask, 1);
			if (ret)
				return ret;

			last_byte_offset = byte_offset;
		}

		irq = vgic_get_irq(vcpu->kvm, intid);
		if (!irq)
			continue;

		raw_spin_lock_irqsave(&irq->irq_lock, flags);
		if (irq->target_vcpu == vcpu)
			irq->pending_latch = pendmask & (1U << bit_nr);
		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
		vgic_put_irq(vcpu->kvm, irq);
	}
}

If a guest enables LPIs on a vCPU that has no LPIs targeting it (or only
lower-INTID LPIs) with an unmapped or smaller pending table in GICR_PENDBASER,
while LPIs in dist->lpi_xa target other vCPUs or unmapped collections,
its_sync_lpi_pending_table() will read that vCPU's pending table at the byte
offsets of those unrelated LPIs and return -EFAULT. That also performs
O(nr_vcpus * nr_lpis) guest memory reads and vgic_queue_irq_unlock() calls
across all vCPUs during restore.

> +		if (ret) {
> +			vgic_its_free_device_list(kvm, its);
> +			vgic_its_free_collection_list(kvm, its);
> +			return ret;
> +		}

[Severity: Medium]
If its_sync_lpi_pending_table() fails partway through syncing the vCPUs'
pending tables, does this error path leave already-queued LPIs referenced in
dist->lpi_xa and on vcpu->arch.vgic_cpu.ap_list_head?

During the documented restore sequence, the distributor and redistributors are
restored before KVM_DEV_ARM_ITS_RESTORE_TABLES is invoked. When
its_sync_lpi_pending_table() sees a pending bit set for an enabled LPI, it
sets irq->pending_latch and calls vgic_queue_irq_unlock(), which takes an
additional reference via vgic_get_irq_ref() and adds irq->ap_list to
vcpu->arch.vgic_cpu.ap_list_head.

If a later kvm_read_guest_lock() call fails (on a subsequent byte offset or a
subsequent vCPU), vgic_its_free_device_list() frees the ITEs via
its_free_ite():

arch/arm64/kvm/vgic/vgic-its.c:its_free_ite() {
	if (irq) {
		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
			if (irq->hw)
				its_unmap_vlpi(ite->irq->host_irq);

			irq->hw = false;
			irq->mapped = false;
		}

		vgic_put_irq(kvm, ite->irq);
	}
}

Because its_free_ite() only drops the single ITE reference without clearing
irq->pending_latch or flushing ap_list_head, any LPI that was already queued
retains its ap_list reference and stays in dist->lpi_xa and on ap_list_head
with irq->mapped = false. If the vCPU runs, vgic_target_oracle() does not
check irq->mapped and can inject the unmapped LPI, or if
KVM_DEV_ARM_ITS_RESTORE_TABLES is retried, vgic_add_lpi() will find the
existing vgic_irq in dist->lpi_xa and return it early without calling
update_lpi_config().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105110050.1244525-1-fuad.tabba@linux.dev?part=3