Re: [RFC PATCH v3 05/20] vfio/pci: Buffer ROM reads before copying to userspace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] vfio/pci: pci_map_rom() called before
  __vfio_pci_memory_enabled() in vfio_pci_rom_read()
--

commit 95981493fc3ccf127f13d3a0117382988fa550cd
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Buffer ROM reads before copying to userspace

This commit buffers PCI ROM reads into a kernel buffer under memory_lock
and unmaps the ROM before copying the data to userspace.

> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index df88480876815..10cd091031fe4 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c

[ ... ]

> @@ -198,6 +200,96 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
>  
> +static ssize_t vfio_pci_rom_read(struct vfio_pci_core_device *vdev,
> +				 char __user *buf, size_t count, loff_t pos)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +	bool rom_bar = pci_resource_start(pdev, PCI_ROM_RESOURCE);
> +	size_t size, length = 0, done = 0;
> +	void *data = NULL;
> +	void __iomem *io;
> +	ssize_t ret;
> +
> +	/* Protect ROM decoding against concurrent power changes and ROM accesses. */
> +	down_write(&vdev->memory_lock);
> +	if (rom_bar) {
> +		io = pci_map_rom(pdev, &size);
> +	} else {
> +		io = ioremap(pdev->rom, pdev->romlen);
> +		size = pdev->romlen;
> +	}
> +	if (!io) {
> +		ret = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	/* Buffer only ROM data, not unused space in a large ROM BAR. */
> +	if (pos < size)
> +		length = min(count, size - (size_t)pos);
> +	if (length) {
> +		if ((pci_resource_flags(pdev, PCI_ROM_RESOURCE) & IORESOURCE_MEM) &&
> +		    !__vfio_pci_memory_enabled(vdev)) {
> +			ret = -EIO;
> +			goto out_unmap;
> +		}

[Severity: Medium]
Should __vfio_pci_memory_enabled(vdev) be checked before calling
pci_map_rom()?

pci_map_rom() enables ROM decoding and calls pci_get_rom_size(), which
performs MMIO reads on the ROM BAR:

drivers/pci/rom.c:pci_map_rom() {
	...
	if (pci_enable_rom(pdev))
		return NULL;

	rom = ioremap(start, *size);
	if (!rom)
		goto err_ioremap;

	*size = pci_get_rom_size(pdev, rom, *size);
	if (!*size)
		goto invalid_rom;
	...
}

If userspace reads the ROM region while PCI_COMMAND_MEMORY is cleared in
vconfig or the device is in PCI_D3hot, pci_get_rom_size() still issues MMIO
reads to the disabled or D3hot device, which can trigger Unsupported Request
AER errors or platform faults.  When those reads return 0xffff,
pci_map_rom() also logs an invalid ROM signature error and returns NULL, so
vfio_pci_rom_read() fails with -ENOMEM before reaching the
__vfio_pci_memory_enabled() check.

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