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 <[email protected]> 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/[email protected]?part=4
