Re: [PATCH v4 3/4] vfio-pci/zdev: Add VFIO FMB device features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Holding `zdev->fmb_lock` across user memory accesses allows a malicious userspace process to stall host device management.
- [Medium] Enabling the FMB feature via `VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE` will unconditionally fail with `-EINVAL` on a newly opened device.
--

commit 67b6ef61b40cb8ad6b8fa03e4587c893d168c03d
Author: Omar Elghoul <oelghoul@linux.ibm.com>

vfio-pci/zdev: Add VFIO FMB device features

Introduce new VFIO features for zPCI devices to provide FMB passthrough to
userspace. Allow the user to enable or disable the FMB using the SET-only
feature VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE and read it using the GET-only
feature VFIO_DEVICE_FEATURE_ZPCI_FMB_READ.

[ ... ]

> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index 0990fdb146b78..ad1bcaf52ff24 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> @@ -167,3 +167,58 @@ void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
>  	if (zpci_kvm_hook.kvm_unregister)
>  		zpci_kvm_hook.kvm_unregister(zdev);
>  }
> +
> +int vfio_pci_zdev_feature_fmb_enable(struct vfio_pci_core_device *vdev, u32 flags,
> +				     void __user *arg, size_t argsz)
> +{
> +	struct zpci_dev *zdev;
> +	struct vfio_device_feature_zpci_fmb_enable fmb_enable;
> +	int ret;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET, sizeof(fmb_enable));
> +	if (ret != 1)
> +		return ret;
> +
> +	zdev = to_zpci(vdev->pdev);
> +	if (!zdev)
> +		return -ENODEV;
> +
> +	if (copy_from_user(&fmb_enable, arg, sizeof(fmb_enable)))
> +		return -EFAULT;
> +
> +	guard(mutex)(&zdev->fmb_lock);
> +
> +	if (fmb_enable.enabled)
> +		return zpci_fmb_enable_device(zdev);

[Severity: Medium]
Will enabling this feature fail with -EINVAL on a newly opened device?

When userspace opens a VFIO device, vfio_pci_core_enable() automatically
enables the physical device via pci_enable_device(). On s390,
pcibios_enable_device() unconditionally calls zpci_fmb_enable_device(),
which successfully allocates zdev->fmb.

If userspace subsequently attempts to enable the FMB feature via this ioctl,
it calls into zpci_fmb_enable_device() again:

arch/s390/pci/pci.c:zpci_fmb_enable_device() {
        ...
        if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
                return -EINVAL;
        ...
}

Does this make it impossible for userspace to explicitly enable the feature
without first disabling it?

> +	return zpci_fmb_disable_device(zdev);
> +}
> +
> +int vfio_pci_zdev_feature_fmb_read(struct vfio_pci_core_device *vdev, u32 flags,
> +				   void __user *arg, size_t argsz)
> +{
> +	struct zpci_dev *zdev;
> +	struct vfio_device_feature_zpci_fmb_read fmb_read;
> +	int ret;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET, sizeof(fmb_read));
> +	if (ret != 1)
> +		return ret;
> +
> +	zdev = to_zpci(vdev->pdev);
> +	if (!zdev)
> +		return -ENODEV;
> +
> +	guard(mutex)(&zdev->fmb_lock);
> +
> +	if (!zdev->fmb)
> +		return -ENOMSG;
> +	if (copy_from_user(&fmb_read, arg, sizeof(fmb_read)))
> +		return -EFAULT;
> +	if (!fmb_read.data)
> +		return -EINVAL;
> +
> +	if (copy_to_user((struct zpci_fmb __user *) fmb_read.data, zdev->fmb, zdev->fmb_length))
> +		return -EFAULT;

[Severity: High]
Can holding this lock across user memory accesses cause a denial of service?

A malicious userspace process could provide a userfaultfd-backed memory
region for arg or fmb_read.data to intentionally stall the thread
indefinitely while holding zdev->fmb_lock.

Since pcibios_disable_device() also requires this lock during device
teardown, would this stall permanently block the host from hot-unplugging
the PCI device or unbinding the VFIO driver?

> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612181048.91548-1-oelghoul@linux.ibm.com?part=3