Re: [RFC PATCH 08/19] vfio/pci: Serialize BAR and ROM access with recovery

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent accesses to PCI_ROM_RESOURCE map and unmap the ROM, which concurrently read/modify/write the ROM_ENABLE bit in PCI config space without serialization. One thread's unmap can disable the ROM while another thread is still performing IO, leading to Master Aborts or AER hardware errors.
--

commit 56bec71818cf734e5593b7903748915f73efa3b3
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Serialize BAR and ROM access with recovery

This commit holds the recovery_lock for reading around trapped BAR reads
and writes, as well as around ROM mapping. It defers unmapping configuration
changes during host recovery to prevent accesses while recovery blocks them.

> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
[ ... ]
> @@ -230,7 +275,11 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  		 * filling large ROM BARs much faster.
>  		 */
>  		if (pci_resource_start(pdev, bar)) {
> +			ret = vfio_pci_core_access_begin(vdev);
> +			if (ret)
> +				return ret;
>  			io = pci_map_rom(pdev, &x_start);
> +			vfio_pci_core_access_end(vdev);
>  		} else {
>  			io = ioremap(pdev->rom, pdev->romlen);
>  			x_start = pdev->romlen;
[ ... ]
> @@ -269,7 +318,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  
>  	if (bar == PCI_ROM_RESOURCE) {
>  		if (pci_resource_start(pdev, bar))
> -			pci_unmap_rom(pdev, io);
> +			vfio_pci_unmap_rom(vdev, io);

[Severity: High]
This is a pre-existing issue, but does the lack of serialization around ROM
mapping and unmapping expose the host to hardware errors?

When two threads in userspace concurrently call pread on the VFIO device file
descriptor targeting the PCI_ROM_RESOURCE BAR, both threads will enter
vfio_pci_bar_rw and call pci_map_rom. This enables the ROM address decoder in
the PCI config space.

When the first thread finishes reading, it calls vfio_pci_unmap_rom, which
disables ROM decode by clearing the ROM_ENABLE bit in config space.

If the second thread is concurrently executing vfio_pci_core_do_io_rw and
performing IO reads on the memory-mapped ROM, the disabled ROM address decoder
will cause the PCI read transaction to result in a Master Abort or an
Unsupported Request.

Depending on the host AER configuration and hardware, this could trigger
an Advanced Error Reporting interrupt, leading to a system freeze or panic.

Given that unprivileged userspace like a QEMU process can reach this path,
is there a need to serialize these concurrent accesses to prevent a Denial of
Service?

>  		else
>  			iounmap(io);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=8