Re: [PATCH v20 12/16] vfio/pci: Add basic NO-ST TPH VFIO device feature support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional release of the DMA reservation lock in `vfio_pci_get_dmabuf_tph()` when acquisition fails.
- [Medium] Missing validation for the reserved field `rsv` in `struct vfio_device_feature_tph_resolve`.
--

--- Patch [12]: [PATCH v20 12/16] vfio/pci: Add basic NO-ST TPH VFIO device feature support ---
commit e2b4dfe99fe4b36b6dce86a257a0fa1605e500b6
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Add basic NO-ST TPH VFIO device feature support

Implement basic PCIe TPH support in NO-ST mode, adding VFIO_DEVICE_FEATURE_TPH
and VFIO_DEVICE_FEATURE_TPH_RESOLVE to allow TPH operations after opt-in.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 4e5e34a77b76d..82582c8ef69da 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1607,6 +1611,123 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
[ ... ]
> +static int vfio_pci_get_dmabuf_tph(int fd, bool extended, u16 *st, u8 *ph)
> +{
> +	struct dma_buf *dmabuf;
> +	int ret;
> +
> +	dmabuf = dma_buf_get(fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
> +	if (ret == 0)
> +		ret = dma_buf_get_pci_tph(dmabuf, extended, st, ph);
> +	dma_resv_unlock(dmabuf->resv);

[Severity: High]
Does this code unconditionally unlock the DMA reservation lock even if the
lock acquisition fails? 

If a signal interrupts dma_resv_lock_interruptible(), it will return -EINTR,
skipping the read but still calling dma_resv_unlock(). Can this underflow the
WW mutex state and lead to a panic?

> +
> +	dma_buf_put(dmabuf);
> +	return ret;
> +}
> +
> +static int vfio_pci_core_feature_tph_resolve(struct vfio_pci_core_device *vdev,
> +			u32 flags,
> +			struct vfio_device_feature_tph_resolve __user *arg,
> +			size_t argsz)
> +{
> +	struct vfio_device_feature_tph_resolve resolve;
> +	u32 user_flags, allow_src;
> +	bool extended;
> +	u16 tag = 0;
> +	u8 ph = 0;
> +	int ret;
> +
> +	if (!vdev->tph_opt_in)
> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET,
> +				 sizeof(resolve));
> +	if (ret != 1)
> +		return ret;
> +
> +	if (copy_from_user(&resolve, arg, sizeof(resolve)))
> +		return -EFAULT;
> +
> +	user_flags = resolve.flags;

[Severity: Medium]
Does this code need to validate that the reserved field resolve.rsv is zero?

Without checking it here, userspace could pass garbage data, which might
prevent safely extending this UAPI structure in the future without breaking
existing applications.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714104325.54444-1-fengchengwen@huawei.com?part=12