Re: [PATCH v3 1/8] vfio: add helper to find PCI device by vendor/device ID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] vfio: unreferenced vfio_device return in vfio_file_find_pci_device()
- [High] vfio: broken cdev lookup in vfio_file_find_pci_device() when !VFIO_GROUP
- [Medium] vfio: ambiguous device lookup by vendor/device ID in vfio_file_find_pci_device()
--

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

vfio: add helper to find PCI device by vendor/device ID

This commit adds vfio_file_find_pci_device() and vfio_file_device() to allow
other subsystems to locate a VFIO-owned PCI device by its vendor and device
IDs from a VFIO group or cdev file descriptor.

> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index 185f359f75d38..0b6cf90edda30 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
[ ... ]
> @@ -894,6 +895,46 @@ bool vfio_file_has_dev(struct file *file, struct vfio_device *device)
>  }
>  EXPORT_SYMBOL_GPL(vfio_file_has_dev);
>  
> +struct vfio_device *vfio_file_find_pci_device(struct file *file,
> +					      u16 vendor, u16 device_id)
> +{
> +	struct vfio_group *group;
> +	struct vfio_device *vdev;
> +	struct vfio_device *ret = NULL;
> +
> +	group = vfio_group_from_file(file);
> +	if (group) {
> +		mutex_lock(&group->device_lock);
> +		list_for_each_entry(vdev, &group->device_list, group_next) {
> +			struct pci_dev *pdev;
> +
> +			if (!dev_is_pci(vdev->dev))
> +				continue;
> +
> +			pdev = to_pci_dev(vdev->dev);
> +			if (pdev->vendor == vendor &&
> +			    pdev->device == device_id) {
> +				ret = vdev;
> +				break;
> +			}

[Severity: Medium]
What happens if a VFIO group contains multiple PCI devices or functions
with the same vendor and device IDs (for example, two identical cards
behind a PCIe switch or bridge without ACS isolation, or a multi-function
device with identical function IDs)?

Since this loop matches only on vendor and device_id and breaks on the
first match in group->device_list without BDF/devfn disambiguation or
checking whether the device is opened by the VM, could this return an
unopened sibling device in the same group or cause multiple assigned
devices to resolve to the first entry in the list?

> +		}
> +		mutex_unlock(&group->device_lock);
> +		return ret;
> +	}

[Severity: Critical]
Can returning ret here after dropping group->device_lock lead to a
use-after-free of struct vfio_device and vdev->dev in the caller?

Unlike a VFIO device file, which holds a registration reference until
vfio_device_fops_release(), an open VFIO group file only pins struct
vfio_group and does not hold a reference on the struct vfio_device entries
in group->device_list. Existing group list lookup code in
drivers/vfio/group.c acquires a reference while holding group->device_lock:

drivers/vfio/group.c:vfio_device_get_from_name() {
	...
		if (ret && vfio_device_try_get_registration(it)) {
			device = it;
			break;
		}
	}
	mutex_unlock(&group->device_lock);
}

If the matched device in the group does not have an open device fd (or its
device fd is concurrently closed while the group fd remains open), a
concurrent driver unbind or hot-unplug via:

vfio_pci_remove()
  -> vfio_pci_core_unregister_device()
    -> vfio_unregister_group_dev()
      -> vfio_device_group_unregister()
      -> vfio_device_put_registration()
  -> vfio_put_device()
    -> vfio_device_release()
      -> kvfree(device)

can remove vdev from group->device_list and free it as soon as
mutex_unlock(&group->device_lock) is called. Should this acquire a
reference on vdev before unlocking and provide a corresponding put helper
for callers?

[ ... ]
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 0cc91c6f96d2b..a62fbf42458a4 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -364,6 +364,8 @@ struct iommu_group *vfio_file_iommu_group(struct file *file);
>  #if IS_ENABLED(CONFIG_VFIO_GROUP)
>  bool vfio_file_is_group(struct file *file);
>  bool vfio_file_has_dev(struct file *file, struct vfio_device *device);
> +struct vfio_device *vfio_file_find_pci_device(struct file *file,
> +					      u16 vendor, u16 device_id);
>  #else
>  static inline bool vfio_file_is_group(struct file *file)
>  {
> @@ -374,10 +376,18 @@ static inline bool vfio_file_has_dev(struct file *file, struct vfio_device *devi
>  {
>  	return false;
>  }
> +
> +static inline struct vfio_device *vfio_file_find_pci_device(struct file *file,
> +							    u16 vendor,
> +							    u16 device_id)
> +{
> +	return NULL;
> +}
>  #endif

[Severity: High]
Will vfio_file_find_pci_device() work for cdev-based device files when
CONFIG_VFIO_GROUP is disabled and CONFIG_VFIO_DEVICE_CDEV is enabled?

Because vfio_file_find_pci_device() is defined in drivers/vfio/group.c and
stubbed to return NULL when !CONFIG_VFIO_GROUP, the vfio_file_device(file)
cdev lookup path is never compiled in a cdev-only kernel configuration
(and symbol_get(vfio_file_find_pci_device) callers will not find the
exported symbol).

Should vfio_file_find_pci_device() be placed in drivers/vfio/vfio_main.c
and declared unconditionally in include/linux/vfio.h alongside
vfio_file_is_valid(), vfio_file_enforced_coherent(), and
vfio_file_set_kvm()?

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