Re: [PATCH v7 4/5] vfio/pci: implement get_tph and DMA_BUF_TPH feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional validation of the flag-gated `ph` parameter outside its flag gate.
- [Medium] VFIO_DEVICE_FEATURE_PROBE incorrectly reports that the DMA_BUF_TPH feature is supported for devices that lack TPH completer capabilities.
--

commit cadaa92f5a8da2be3b081b506699064b7bf68059
Author: Zhiping Zhang <zhipingz@meta.com>

vfio/pci: implement get_tph and DMA_BUF_TPH feature

Implement dma-buf get_tph for vfio-pci exported dma-bufs and add
VFIO_DEVICE_FEATURE_DMA_BUF_TPH so userspace can publish TPH metadata
for a VFIO-owned device.

> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> index 1a177ce7de546..0a0705c8dbea6 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c

[ ... ]

> @@ -333,6 +360,71 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
>  	return ret;
>  }
>  
> +int vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev,
> +				      u32 flags,
> +				      struct vfio_device_feature_dma_buf_tph __user *arg,
> +				      size_t argsz)
> +{
> +	struct vfio_device_feature_dma_buf_tph set_tph;
> +	struct vfio_pci_dma_buf *priv;
> +	struct dma_buf *dmabuf;
> +	u8 comp;
> +	int ret;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(set_tph));
> +	if (ret != 1)
> +		return ret;

[Severity: Medium]
Will VFIO_DEVICE_FEATURE_PROBE incorrectly report that the feature is
supported on all devices?

vfio_check_feature() returns 0 for probe requests, which means this code
will return 0 (success) before checking the hardware's TPH completer
capabilities below. Should the hardware capability check happen before
returning the probe result?

> +
> +	if (copy_from_user(&set_tph, arg, sizeof(set_tph)))
> +		return -EFAULT;
> +
> +	if (set_tph.flags & ~(VFIO_DMA_BUF_TPH_ST | VFIO_DMA_BUF_TPH_ST_EXT))
> +		return -EINVAL;
> +
> +	if (set_tph.ph & ~0x3)
> +		return -EINVAL;

[Severity: Medium]
Does this code reject valid clear requests if the unused ph field contains
uninitialized data?

When userspace sets flags == 0 to clear the metadata, ph is effectively
unused and might contain garbage. Validating it unconditionally outside of
its flag gate could cause valid API usage to fail with -EINVAL. Should the
ph validation only happen if VFIO_DMA_BUF_TPH_ST or
VFIO_DMA_BUF_TPH_ST_EXT are set?

> +
> +	comp = pcie_tph_completer_type(vdev->pdev);
> +	if (comp == PCI_EXP_DEVCAP2_TPH_COMP_NONE)
> +		return -EOPNOTSUPP;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611161546.4075580-1-zhipingz@meta.com?part=4