Re: [RFC PATCH 2/5] vfio/pci: Implement ZONE_DEVICE-backed P2P registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Registering a BAR for P2P DMA does not check if the BAR contains the MSI-X table, allowing userspace to bypass VFIO MSI-X protections.
- [High] Missing synchronization in `vfio_pci_core_feature_p2p_register` allows concurrent ioctls to corrupt state and leak devm memory.
- [High] Missing synchronization and state conflict between VFIO DMABUF and P2P DMA pool registration.
- [High] P2P BAR registration state and devres allocations persist across VFIO file descriptor closures, breaking device isolation.

Pre-existing issues:
- [High] The error path in `pci_p2pdma_add_resource()` frees `p2p_pgmap` without revoking the devres unmap action.
--

commit d5df237db4cc33de44c5c2d95b83b4fda6fa953a
Author: Pranjal Shrivastava <praan@google.com>

vfio/pci: Implement ZONE_DEVICE-backed P2P registration
    
Implement the VFIO_DEVICE_FEATURE_P2P_REGISTER. A bitmask is added to
track the registered regions.
    
Post-registration, the BAR is added to the device's P2P pool and is
available to be used with standard page-based APIs.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a28f1e99362c1..1e922e3aaeb3a 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1554,6 +1554,43 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> +static int vfio_pci_core_feature_p2p_register(struct vfio_pci_core_device *vdev,
> +					      u32 flags, u32 __user *arg,
> +					      size_t argsz)
> +{
> +	u32 bar_index;
> +	int ret;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(bar_index));
> +	if (ret != 1)
> +		return ret;
> +
> +	if (copy_from_user(&bar_index, arg, sizeof(bar_index)))
> +		return -EFAULT;
> +
> +	if (bar_index >= PCI_STD_NUM_BARS)
> +		return -EINVAL;
> +
> +	if (!(pci_resource_flags(vdev->pdev, bar_index) & IORESOURCE_MEM))
> +		return -EINVAL;

[Severity: Critical]
Does this code allow userspace to bypass VFIO MSI-X protections?

If the requested BAR contains the device's MSI-X table (bar_index ==
vdev->msix_bar), passing the entire BAR to pci_p2pdma_add_resource() exposes
the whole physical memory range to the global P2P DMA pool.

A malicious guest or userspace process could instruct another device to
perform P2P DMA writes directly to the MSI-X table, completely bypassing
VFIO's software protections and allowing arbitrary host interrupt injection.

> +
> +	/* Already registered */
> +	if (vdev->p2p_registered_bars & (1 << bar_index))
> +		return 0;

[Severity: High]
Does this code have a missing synchronization issue that allows concurrent
ioctls to corrupt state and leak devm memory?

The ioctl handler modifies vdev->p2p_registered_bars and calls
pci_p2pdma_add_resource() without holding any serialization lock (such as
vdev->igate or vdev->memory_lock).

Since ioctls can be invoked concurrently on the same file descriptor, multiple
threads can bypass the unregistered check simultaneously, leading to
overlapping duplicate devres allocations in pci_p2pdma_add_resource(), sysfs
duplicate name errors, and bitmask corruption.

[Severity: High]
Is there a missing synchronization and state conflict between VFIO DMABUF and
P2P DMA pool registration?

If vfio_pci_bar_is_dmabuf(vdev, bar_index) is called here to check the DMABUF
state, it is invoked without holding vdev->memory_lock. Because
vfio_pci_bar_is_dmabuf() explicitly uses
lockdep_assert_held(&vdev->memory_lock), executing this unprotected ioctl
will immediately trigger a lockdep kernel splat. Additionally, without the
lock, the state validation is racy and allows conflicting mappings.

> +
> +	ret = pci_p2pdma_add_resource(vdev->pdev, bar_index, 0, 0);

[Severity: High]
This is a pre-existing issue, but does the error path in
pci_p2pdma_add_resource() free p2p_pgmap without revoking the devres unmap
action?

In pci_p2pdma_add_resource(), if gen_pool_add_owner() fails, the code jumps
to pages_free and calls devm_kfree(&pdev->dev, p2p_pgmap). However, it fails
to call devm_remove_action() for the earlier successfully added
pci_p2pdma_unmap_mappings action.

When the device is later detached, devres will execute the unmap action on a
freed p2p_pgmap pointer, causing a Use-After-Free.

> +	if (ret && ret != -EEXIST)
> +		return ret;
> +
> +	vdev->p2p_registered_bars |= (1 << bar_index);

[Severity: High]
Do the P2P BAR registration state and devres allocations persist across VFIO
file descriptor closures, breaking device isolation?

The pci_p2pdma_add_resource() function ties memory allocations and sysfs
entries to the underlying PCI device (&pdev->dev) using devres. However, when
a user closes the VFIO file descriptor, vfio_pci_core_close_device() does not
invoke vfio_pci_zap_bars() to revoke the P2P memory resources or clear
vdev->p2p_registered_bars.

Thus, the P2P sysfs allocations leak across VM sessions until the VFIO driver
unbinds, and subsequent users inherit a polluted, already-registered state.

> +
> +	pci_info(vdev->pdev, "BAR %d registered for ZONE_DEVICE P2P\n",
> +		 bar_index);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610151853.3608948-1-praan@google.com?part=2