Re: [PATCH v3 2/8] KVM: add PCI MSI hypercall for VFIO passthrough

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] kvm: vfio: use-after-free on struct kvm_vfio and race on pci_msi_alloc
- [Critical] kvm: vfio: use-after-free on vfio_device, msi_desc, and irq_data
- [High] kvm: deadlock acquiring kvm->lock in kvm_vfio_pci_msi_alloc()
- [High] kvm: vfio: guest-triggerable WARN_ON_ONCE via unvalidated MSI vector
- [High] kvm: vfio: broken Multi-MSI message lookup in kvm_vfio_pci_msi_alloc()
- [High] kvm: vfio: ambiguous and unisolated device lookup in kvm_vfio_pci_msi_alloc()
- [Medium] kvm: vfio: link failure calling vfio_file_find_pci_device() directly
- [Medium] kvm: x86: KVM_HC_PCI_MSI_ALLOC lacks feature enumeration and opt-in
- [Low] kvm: vfio: build failure accessing irq_data.parent_data directly
--

commit dc68213ab54a0729231a44c10317169580ab3c78
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

KVM: add PCI MSI hypercall for VFIO passthrough

This commit introduces a new x86 hypercall, KVM_HC_PCI_MSI_ALLOC, to allow a
guest to query the host physical MSI address and data for a VFIO passthrough
PCI device. It connects arch/x86/kvm/x86.c to a new kvm_vfio_pci_msi_alloc()
helper in virt/kvm/vfio.c via a pci_msi_alloc callback on struct kvm.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 32563a91fba99..2f0cae07f9786 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -7490,6 +7491,44 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, int cpl,
>  		vcpu->arch.complete_userspace_io = complete_hypercall;
>  		return 0;
>  	}
> +	case KVM_HC_PCI_MSI_ALLOC: {
> +		struct msi_msg msg = {};
> +		struct {
> +			u32 address_lo;
> +			u32 address_hi;
> +			u32 data;
> +			u32 reserved;
> +		} resp;
> +
> +		if (vcpu->kvm->pci_msi_alloc == NULL) {
> +			ret = -KVM_ENOSYS;
> +			break;
> +		}

[Severity: Medium]
Should KVM_HC_PCI_MSI_ALLOC be gated behind a paravirtual CPUID feature bit
checked via guest_pv_has() or an explicit userspace capability opt-in?

In ____kvm_emulate_hypercall() in arch/x86/kvm/x86.c, other KVM hypercalls
such as KVM_HC_KICK_CPU, KVM_HC_SEND_IPI, and KVM_HC_SCHED_YIELD check
guest_pv_has(vcpu, KVM_FEATURE_*), and KVM_HC_MAP_GPA_RANGE checks
user_exit_on_hypercall(). Here, kvm_vfio_create() unconditionally sets
dev->kvm->pci_msi_alloc = kvm_vfio_pci_msi_alloc whenever a kvm-vfio device
is created, making this hypercall and the host physical MSI routing
information available to any guest by default without enumeration or
userspace opt-in.

Also, since the guest memory layout of resp is part of the guest-host ABI,
should that structure be defined in include/uapi/linux/kvm_para.h rather than
as an anonymous local struct in ____kvm_emulate_hypercall(), and should
64-bit hypercall arguments a0 and a1 be validated for upper-bit truncation
before being passed as u32 and unsigned int?

> +
> +		/*
> +		 * a0 = PCI vendor:device (vendor << 16 | device)
> +		 * a1 = MSI vector index
> +		 * a2 = GPA of response buffer
> +		 */
> +		ret = vcpu->kvm->pci_msi_alloc(vcpu->kvm, a0, a1, &msg);
[ ... ]
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 19548a4309426..60356173e69f3 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
[ ... ]
> @@ -315,6 +318,8 @@ static void kvm_vfio_release(struct kvm_device *dev)
>  
>  	kvm_vfio_update_coherency(dev);
>  
> +	dev->kvm->pci_msi_alloc = NULL;
> +
>  	kfree(kv);
>  	kfree(dev); /* alloc by kvm_ioctl_create_device, free by .release */
>  }
> @@ -349,6 +354,98 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type)
>  	mutex_init(&kv->lock);
>  
>  	dev->private = kv;
> +	dev->kvm->pci_msi_alloc = kvm_vfio_pci_msi_alloc;
> +
> +	return 0;
> +}
> +
> +/**
> + * kvm_vfio_pci_msi_alloc - Get physical MSI address/data for a VFIO device
> + * @kvm: the VM instance
> + * @devid: PCI vendor:device packed as (vendor << 16 | device)
> + * @vector: MSI vector index
> + * @msg: output MSI message (address + data)
> + *
> + * Walks VFIO files attached to this VM, uses vfio_file_find_pci_device()
> + * to find the PCI device matching the given vendor:device ID, and returns
> + * the host's physical MSI address/data for the requested vector.
> + *
> + * Returns 0 on success, negative errno on failure.
> + */
> +int kvm_vfio_pci_msi_alloc(struct kvm *kvm, u32 devid,
> +			     unsigned int vector, struct msi_msg *msg)
> +{
> +	struct kvm_vfio_file *kvf;
> +	struct kvm_vfio *kv = NULL;
> +	struct vfio_device *vdev;
> +	struct kvm_device *dev;
> +	struct msi_desc *desc;
> +	struct irq_data *irqd;
> +	struct pci_dev *pdev;
> +	u16 vendor = devid >> 16;
> +	u16 device = devid & 0xffff;
> +	int irq;
> +
> +	scoped_guard(mutex, &kvm->lock) {
> +		list_for_each_entry(dev, &kvm->devices, vm_node) {
> +			if (dev->ops == &kvm_vfio_ops) {
> +				kv = dev->private;
> +				break;
> +			}
> +		}
> +	}

[Severity: High]
Can acquiring kvm->lock here in kvm_vfio_pci_msi_alloc() cause a deadlock?

When a guest invokes KVM_HC_PCI_MSI_ALLOC, the call chain is:

  kvm_vcpu_ioctl() [holds vcpu->mutex]
    -> kvm_arch_vcpu_ioctl_run()
      -> vcpu_enter_guest() [holds kvm->srcu via kvm_vcpu_srcu_read_lock()]
        -> handle_exit()
          -> kvm_emulate_hypercall()
            -> ____kvm_emulate_hypercall()
              -> kvm_vfio_pci_msi_alloc()
                -> mutex_lock(&kvm->lock)

According to Documentation/virt/kvm/locking.rst, kvm->lock must be acquired
outside vcpu->mutex (for example, kvm_lock_all_vcpus() and
kvm_vm_ioctl_create_vcpu() in virt/kvm/kvm_main.c take kvm->lock and then
vcpu->mutex). In addition, synchronize_srcu(&kvm->srcu) is called while
holding kvm->lock or vcpu->mutex.

Doesn't taking kvm->lock while holding both vcpu->mutex and the kvm->srcu
read lock introduce an ABBA lock inversion and an SRCU deadlock?

> +
> +	if (!kv)
> +		return -ENODEV;
> +
> +	guard(mutex)(&kv->lock);

[Severity: Critical]
What prevents struct kvm_vfio (kv) and its kv->file_list entries from being
freed before or during kvm_vfio_pci_msi_alloc()?

In kvm_vfio_pci_msi_alloc(), kvm->lock is released at the end of the
scoped_guard block without pinning dev or kv. If userspace closes the
kvm-vfio device fd concurrently:

  kvm_device_release()
    -> mutex_lock(&kvm->lock)
    -> list_del(&dev->vm_node)
    -> mutex_unlock(&kvm->lock)
    -> synchronize_rcu()
    -> kvm_vfio_release(dev)
      -> list_for_each_entry_safe(kvf, tmp, &kv->file_list, node)
           kvm_vfio_file_free(dev, kvf);
      -> dev->kvm->pci_msi_alloc = NULL;
      -> kfree(kv);

Because synchronize_rcu() does not wait for preemptible vCPU threads in
____kvm_emulate_hypercall(), and kvm_vfio_release() frees kv->file_list
without taking kv->lock and then frees kv, can kvm_vfio_pci_msi_alloc()
access freed memory when locking kv->lock or iterating kv->file_list?

Similarly, in ____kvm_emulate_hypercall():

	if (vcpu->kvm->pci_msi_alloc == NULL) {
		ret = -KVM_ENOSYS;
		break;
	}
	ret = vcpu->kvm->pci_msi_alloc(vcpu->kvm, a0, a1, &msg);

could kvm_vfio_release() set dev->kvm->pci_msi_alloc = NULL between the NULL
check and the indirect call, resulting in a NULL function pointer call?

> +
> +	vdev = NULL;
> +	list_for_each_entry(kvf, &kv->file_list, node) {
> +		vdev = vfio_file_find_pci_device(kvf->file, vendor, device);
> +		if (vdev)
> +			break;
> +	}

[Severity: Medium]
Will calling vfio_file_find_pci_device() directly break the build when
CONFIG_KVM=y and CONFIG_VFIO=m (with CONFIG_VFIO_GROUP=y)?

In virt/kvm/vfio.c, all other VFIO helpers (such as vfio_file_set_kvm and
vfio_file_enforced_coherent) are looked up dynamically via symbol_get() and
symbol_put() so that built-in KVM does not have a link-time dependency on
vfio.ko. Calling vfio_file_find_pci_device() directly from virt/kvm/vfio.c
causes an undefined symbol reference at link time when KVM is built-in and
VFIO is a module.

Also, in include/linux/vfio.h, vfio_file_find_pci_device() is guarded by
#if IS_ENABLED(CONFIG_VFIO_GROUP). When CONFIG_VFIO_DEVICE_CDEV=y and
CONFIG_VFIO_GROUP=n, vfio_file_find_pci_device() compiles to the inline stub
returning NULL, causing kvm_vfio_pci_msi_alloc() to always return -ENODEV on
cdev-only builds.

[Severity: High]
Is matching solely by 16-bit vendor and device ID sufficient when looking up
the VFIO device in kvm_vfio_pci_msi_alloc()?

If a VM has two passthrough PCI devices with the same vendor:device ID, this
loop always stops at the first device in kv->file_list and returns its MSI
message for both devices, which will trigger IOMMU source-ID validation
faults on the second device.

In addition, when kvf->file is a VFIO group fd, vfio_file_find_pci_device()
walks group->device_list and matches any PCI device in the host IOMMU group
without checking whether that device is opened or assigned to this VM. Can a
guest use this hypercall to read the host physical MSI address and data of
unassigned sibling PCI devices in the same IOMMU group?

> +
> +	if (!vdev)
> +		return -ENODEV;
> +
> +	pdev = to_pci_dev(vdev->dev);
> +
> +	if (!pdev->msi_enabled && !pdev->msix_enabled) {
> +		memset(msg, 0, sizeof(*msg));
> +		return 0;
> +	}
> +
> +	irq = pci_irq_vector(pdev, vector);

[Severity: High]
Can a guest trigger a host WARN_ON_ONCE in msi_domain_get_virq() by passing
a vector index greater than MSI_MAX_INDEX (0xffff)?

In ____kvm_emulate_hypercall(), the guest-controlled register a1 is passed
directly as vector to kvm_vfio_pci_msi_alloc(), which passes it unvalidated
to pci_irq_vector(pdev, vector) -> msi_get_virq() -> msi_domain_get_virq():

kernel/irq/msi.c:msi_domain_get_virq() {
	if (WARN_ON_ONCE(index > MSI_MAX_INDEX || domid >= MSI_MAX_DEVICE_IRQDOMAINS))
		return 0;
}

Should vector be bounds-checked before calling pci_irq_vector()?

> +	if (irq <= 0) {
> +		memset(msg, 0, sizeof(*msg));
> +		return 0;
> +	}
> +
> +	irqd = irq_get_irq_data(irq);
> +	if (!irqd) {
> +		memset(msg, 0, sizeof(*msg));
> +		return 0;
> +	}
> +
> +	desc = irq_get_msi_desc(irq);
> +	if (desc) {
> +		*msg = desc->msg;

[Severity: Critical]
What protects vdev, pdev, irqd, and desc from concurrent teardown while
kvm_vfio_pci_msi_alloc() accesses them?

First, for a VFIO group file, vfio_file_find_pci_device() drops
group->device_lock before returning vdev without acquiring a reference on
vdev. Holding the group file in kv->file_list does not prevent an individual
vfio_device in group->device_list from being unregistered via
vfio_unregister_group_dev(), so vdev and pdev can be freed while
kvm_vfio_pci_msi_alloc() dereferences them.

Second, pci_irq_vector() only holds msi_descs_lock(&pdev->dev) internally
while looking up the Linux IRQ number. Once it returns irq,
kvm_vfio_pci_msi_alloc() calls irq_get_irq_data(irq), irq_get_msi_desc(irq),
and reads desc->msg (or walks irqd->parent_data) without holding
msi_descs_lock(&pdev->dev), vdev->igate, or the IRQ descriptor lock. If host
userspace concurrently reconfigures or disables MSI/MSI-X via
VFIO_DEVICE_SET_IRQS -> vfio_msi_disable() -> pci_free_irq_vectors(), can
both the irq_desc and struct msi_desc be freed while kvm_vfio_pci_msi_alloc()
is dereferencing desc or irqd?

[Severity: High]
Does reading desc->msg return the wrong MSI message for Multi-MSI vectors
where vector > 0?

For PCI Multi-MSI, __msi_domain_alloc_irqs() in kernel/irq/msi.c attaches the
same shared struct msi_desc to all allocated Linux IRQs in the Multi-MSI
group:

	for (i = 0; i < desc->nvec_used; i++) {
		irq_set_msi_desc_off(virq, i, desc);
		...
	}

And pci_msi_domain_write_msg() in drivers/pci/msi/irqdomain.c only caches
desc->msg for the first vector:

	if (desc->irq == irq_data->irq)
		__pci_write_msi_msg(desc, msg);

Because irq_get_msi_desc(irq) is non-NULL for every MSI/MSI-X vector, the
if (desc) branch is always taken (making the else branch unreachable dead
code), and any Multi-MSI request with vector > 0 receives vector 0's cached
desc->msg instead of the composed message for that vector. Should this use
irq_chip_compose_msi_msg() or adjust the message for Multi-MSI vectors?

> +	} else {
> +		struct irq_data *pos;
> +
> +		for (pos = irqd; pos; pos = pos->parent_data) {
> +			if (pos->chip && pos->chip->irq_compose_msi_msg) {
> +				pos->chip->irq_compose_msi_msg(pos, msg);
> +				break;
> +			}
> +		}

[Severity: Low]
Can accessing pos->parent_data directly fail to compile when
CONFIG_IRQ_DOMAIN_HIERARCHY is disabled?

In include/linux/irq.h, struct irq_data only defines parent_data under
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY:

struct irq_data {
	...
#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
	struct irq_data		*parent_data;
#endif
	void			*chip_data;
};

On architectures that select CONFIG_KVM_VFIO without unconditionally
selecting CONFIG_IRQ_DOMAIN_HIERARCHY (such as s390 or powerpc configs
without CONFIG_PCI_MSI), referencing pos->parent_data directly in
virt/kvm/vfio.c will break the build unless irq_chip_compose_msi_msg() or a
config guard is used.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029163713.1366077-1-jtornosm@redhat.com?part=2