Re: [PATCH 09/16] KVM: arm64: gic-v5: Add ITS MSI injection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] kvm/vgic: raw_spinlock deadlock due to missing IRQ disable in its_cache_add_translation
- [High] kvm/vgic: userspace-triggerable WARN_ON_ONCE causes host DoS in vgic_v5_its_inject_msi
- [High] arm64: vgic-v5: User-triggerable WARN_ON_ONCE in MSI injection
- [High] kvm: arm64: ABI violation on guest-driven MSI mapping errors
- [Medium] kvm: arm64: gic-v5: MSI injection aborted on translation cache allocation failure
- [Medium] arm64: vgic-v5: MSI injection fails if ITS cache insertion fails
--

commit 65afd53542f7c52bea1da09f7e70c85b22c50b33
Author: Sascha Bischoff <sascha.bischoff@arm.com>

    KVM: arm64: gic-v5: Add ITS MSI injection
    
    Now that KVM can resolve GICv5 ITS translations from guest memory,
    allow userspace and irqfd MSI injection to target a virtual GICv5 ITS.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> index e4c0415137605..5920fa1bcb2cf 100644
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c

[ ... ]

>  static int its_cache_add_translation(struct kvm *kvm, struct vgic_v5_its *its,
>  				     u32 device_id, u16 event_id,
>  				     struct vgic_irq *irq)
>  {
>  	u64 key = ((u64)device_id << 16) | (u64)event_id;
>  	struct its_cache_translation_entry *entry;
>  	struct its_cache_translation_entry *old;
>  	struct hlist_node *tmp;
>  	HLIST_HEAD(removed);
>  
>  	entry = kzalloc_obj(*entry, GFP_KERNEL);
>  	if (!entry)
>  		return -ENOMEM;
>  
>  	entry->key = key;
>  	entry->irq = irq;
>  
>  	scoped_guard(raw_spinlock, &its->translation_cache_lock) {

[Severity: High]
Could this lock acquisition deadlock?

The translation_cache_lock is a raw_spinlock_t that is also acquired in
hardirq context by its_cache_look_up_translation(). Since
scoped_guard(raw_spinlock) does not disable local interrupts, a hardirq could
preempt this process-context section and attempt to acquire the same lock,
spinning forever.

Should this use the raw_spinlock_irqsave variant?

[ ... ]

>  static int its_cache_look_up_translation(struct vgic_v5_its *its,
>  					 u32 device_id, u16 event_id,
>  					 struct vgic_irq **irq)
>  {
>  	u64 key = ((u64)device_id << 16) | (u64)event_id;
>  	struct its_cache_translation_entry *entry;
>  
>  	guard(raw_spinlock)(&its->translation_cache_lock);

[Severity: High]
Does this also need to disable interrupts?

Since this is called from the fast path (kvm_arch_set_irq_inatomic) which can
run in hardirq context, and the same lock is taken in process context, should
both sides use the irqsave/irqrestore variants to prevent deadlocks?

[ ... ]

> @@ -531,7 +572,8 @@ static int __vgic_v5_its_resolve_lpi(struct kvm *kvm, struct vgic_v5_its *its,
>  		return PTR_ERR(*irq);
>  
>  	vgic_get_irq_ref(*irq);
> -	ret = its_cache_add_translation(kvm, its, device_id, event_id, *irq);
> +	ret = its_cache_add_translation(kvm, its, device_id, (u16)event_id,
> +					 *irq);
>  	if (ret) {
>  		vgic_put_irq(kvm, *irq);
>  		vgic_put_irq(kvm, *irq);
>  		*irq = NULL;
>  	}
>  
>  	return ret;
>  }

[Severity: Medium]
Is it correct to abort the injection if the translation cache insertion fails?

The translation cache is intended to be a best-effort optimization. If
its_cache_add_translation() fails (e.g., due to -ENOMEM), propagating the
error and dropping the successfully resolved LPI will silently lose the
interrupt, which could hang guest drivers under host memory pressure.

Should the error be swallowed here instead so that injection proceeds?

[ ... ]

> +int vgic_v5_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
> +{
> +	struct vgic_v5_its *its;
> +	struct vgic_irq *irq;
> +	bool borrowed_mm = false;
> +	int ret;
> +
> +	ret = vgic_v5_check_msi(kvm, msi);
> +	if (ret)
> +		return ret;
> +
> +	its = vgic_v5_msi_to_its(kvm, msi);
> +	if (IS_ERR(its))
> +		return PTR_ERR(its);
> +
> +	/* An irqfd cache miss is resolved by a worker outside the VM's mm. */
> +	if (current->mm != kvm->mm) {
> +		if (WARN_ON_ONCE(current->mm || !(current->flags & PF_KTHREAD)))
> +			return -EWOULDBLOCK;

[Severity: High]
Could this WARN_ON_ONCE be triggered by unprivileged userspace? 

If a normal user process (other than the VM creator) calls the KVM_SIGNAL_MSI
ioctl, current->mm will not equal kvm->mm, and since it is a user thread,
current->mm is non-NULL and it lacks PF_KTHREAD. Triggering a kernel warning
can crash the host if panic_on_warn is enabled, leading to a Denial of Service.

> +
> +		if (!mmget_not_zero(kvm->mm))
> +			return -EFAULT;
> +
> +		kthread_use_mm(kvm->mm);
> +		borrowed_mm = true;
> +	}
> +
> +	ret = vgic_v5_its_resolve_lpi(kvm, its, msi->devid, msi->data, &irq);
> +	if (borrowed_mm) {
> +		kthread_unuse_mm(kvm->mm);
> +		mmput(kvm->mm);
> +	}
> +
> +	if (ret)
> +		return ret;

[Severity: High]
Does this violate the KVM_SIGNAL_MSI ABI by returning negative error codes?

When the guest provides an invalid mapping (like an out-of-bounds Event ID),
vgic_v5_its_resolve_lpi() returns errors like -E2BIG or -EINVAL. Propagating
these directly to the ioctl caller instead of returning 0 (to indicate the
guest blocked or unmapped the MSI) can cause the VMM to abort, creating a
guest-to-host Denial of Service.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016155741.979985-1-sascha.bischoff@arm.com?part=9