Re: [PATCH v3 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] The UAPI structure `vfio_device_feature_zpci_fmb_read` uses a `void __user *` pointer instead of an `__aligned_u64`, breaking 32-bit compat userspace execution on 64-bit kernels.
- [Medium] Double enabling the FMB succeeds and silently resets the state, violating the explicitly documented UAPI contract that specifies it should fail.
--

commit 3703a22be992c7a39e18f49c3a21771a62d41260
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. Likewise allow the user to read
the latest FMB 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..09454495ee230 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
[ ... ]
> @@ -167,3 +167,60 @@ void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
[ ... ]
> +int vfio_pci_zdev_feature_fmb_enable(struct vfio_pci_core_device *vdev, u32 flags,
> +				     void __user *arg, size_t argsz)
> +{
[ ... ]
> +	if (fmb_enable.enabled)
> +		return zpci_fmb_reenable_device(zdev);

[Severity: Medium]
Does this violate the explicitly documented UAPI contract? 

The UAPI documentation for VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE states that
the feature "fails on attempt to double enable/disable". However,
zpci_fmb_reenable_device() unconditionally succeeds and resets the hardware
block if already enabled.

Will userspace applications expecting an error on double-enable instead
silently lose counter data?

> +	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;
> +	struct zpci_fmb fmb_temp = {0};
> +	int ret;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET, sizeof(fmb_read));

[Severity: High]
Can this check reject valid 32-bit process payloads on 64-bit kernels?

Because vfio_device_feature_zpci_fmb_read defines data as a pointer type,
it has different sizes for 32-bit and 64-bit userspace (4 bytes vs 8 bytes).
When a 32-bit process computes argsz and passes it to the ioctl,
vfio_check_feature() computes the minimum required payload size based on the
64-bit kernel structure size (8 bytes).

Since the 32-bit process provides a smaller 4-byte payload size, will this
cause the ioctl to unconditionally fail with -EINVAL and break compat
userspace execution?

> +	if (ret != 1)
> +		return ret;
> +
[ ... ]
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 5de618a3a5eee..3988e8690e0b3 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
[ ... ]
> +/**
> + * Upon VFIO_DEVICE_FEATURE_SET, enable or disable FMB for the VFIO zPCI device.
> + *
> + * enabled is treated as a bool, so any non-zero value evaluates to true. This
> + * feature fails on attempt to double enable/disable.
> + *
> + * Returns: 0 on success, -1 and errno set appropriately on error.
> + */
> +#define VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE 13
[ ... ]
> +struct vfio_device_feature_zpci_fmb_read {
> +	void __user *data;
> +};

[Severity: High]
Is it safe to use a variable-sized pointer type in a UAPI structure?

As mentioned above, this pointer will be 4 bytes for 32-bit userspace and
8 bytes for 64-bit kernels. Should this UAPI structure use a fixed-size type
like __aligned_u64 for the pointer instead?

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