Re: [PATCH] vfio/pci: read ROM via ACPI _ROM when hardware ROM is unreadable

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 13, 2026 at 11:36:05PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] vfio/pci: O(N^2) overhead in ACPI ROM read via recalculating size
> - [High] vfio/pci: infinite loop in ACPI ROM size calculation
> - [High] pci: NULL pointer dereference in vfio_pci_acpi_rom_read() when _ROM returns no object
> - [Medium] vfio/pci: ACPI ROM reads truncated by unreadable physical BAR size
> - [Medium] vfio/pci: ACPI ROM reads missing 0xFF padding
> --
> 
> commit ef01db8fb34fde6cd15f6043dd6655f51df61bf4
> Author: Michael S. Tsirkin <mst@redhat.com>
> 
> vfio/pci: read ROM via ACPI _ROM when hardware ROM is unreadable
> 
> Some GPUs (e.g. NVIDIA Quadro P2000 Mobile in a laptop) have a ROM
> BAR that exists but is unreadable because the GPU was never POSTed
> by the system BIOS. On these systems the VBIOS is available via the
> ACPI _ROM method provided by the platform firmware.
> 
> > diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> > index df88480876815..9bc0a7df4fd69 100644
> > --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> > +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> [ ... ]
> > @@ -198,6 +199,112 @@ 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);
> >  
> > +#ifdef CONFIG_ACPI
> > +static int vfio_pci_acpi_rom_read(struct pci_dev *pdev, void *buf,
> > +				  loff_t offset, size_t length)
> > +{
> [ ... ]
> > +	status = acpi_evaluate_object(rom_handle, NULL, &rom_args, &buffer);
> > +	if (ACPI_FAILURE(status))
> > +		return -EIO;
> > +
> > +	obj = buffer.pointer;
> > +	if (obj->type != ACPI_TYPE_BUFFER) {
> 
> [Severity: High]
> If a system's ACPI BIOS is flawed and the _ROM method executes successfully
> but returns no object, acpi_evaluate_object() returns AE_OK but leaves
> buffer.pointer as NULL.
> 
> Will this unconditionally assign and dereference a NULL pointer via
> obj->type, crashing the kernel when vfio_pci_acpi_rom_size() is called
> during region info ioctl?

i can harden it sure.

> > +		kfree(buffer.pointer);
> > +		return -EIO;
> > +	}
> [ ... ]
> > +size_t vfio_pci_acpi_rom_size(struct pci_dev *pdev)
> > +{
> > +	void *buf;
> > +	size_t size = 0;
> > +	int ret;
> > +
> > +	buf = kmalloc(0x1000, GFP_KERNEL);
> > +	if (!buf)
> > +		return 0;
> > +
> > +	do {
> > +		ret = vfio_pci_acpi_rom_read(pdev, buf, size, 0x1000);
> > +		if (ret > 0)
> > +			size += ret;
> > +	} while (ret == 0x1000);
> 
> [Severity: High]
> Does this un-bounded loop rely entirely on firmware behavior to terminate?
> If a buggy or malicious ACPI _ROM method ignores the offset parameter and
> always returns a full 4KB buffer, will this cause an infinite loop and a
> kernel soft lockup?

i can limit to 16M as per pci spec.

> Additionally, this loop evaluates the ACPI method repeatedly in 4KB chunks
> to read the entire ROM just to determine its total size.

i will cache it.

>  Since
> vfio_pci_acpi_rom_size() is called in vfio_pci_bar_rw() on every read,
> will this cause severe performance degradation?  For example, reading a
> 128KB ROM in 4-byte chunks would execute the full ROM reading loop
> via ACPI 32,768 times.
> 
> > +
> > +	kfree(buf);
> > +	return size;
> > +}
> > +#endif /* CONFIG_ACPI */
> > +
> >  ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> >  			size_t count, loff_t *ppos, bool iswrite)
> >  {
> > @@ -215,6 +322,14 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> >  		end = pci_resource_len(pdev, bar);
> 
> [Severity: Medium]
> This sets the hard limit for reads based on the physical PCI resource length.
> If pci_map_rom() fails later in this function, we fallback to ACPI ROM.
> However, if the ACPI ROM is larger than the unreadable physical ROM BAR size,
> will reads beyond the physical BAR size be improperly rejected with -EINVAL,
> even though vfio_pci_ioctl_get_region_info() correctly reported the larger
> acpi_size?

end will be 0 then.


> 
> >  	else if (bar == PCI_ROM_RESOURCE && pdev->rom && pdev->romlen)
> >  		end = roundup_pow_of_two(pdev->romlen);
> > +#ifdef CONFIG_ACPI
> > +	else if (bar == PCI_ROM_RESOURCE) {
> > +		end = vfio_pci_acpi_rom_size(pdev);
> 
> [Severity: High]
> As mentioned above, is it too expensive to invoke this size calculation
> for every user read?  Should the ROM size be cached during region
> initialization instead?


it should.

> 
> > +		if (!end)
> > +			return -EINVAL;
> > +		end = roundup_pow_of_two(end);
> > +	}
> > +#endif
> >  	else
> >  		return -EINVAL;
> >  
> [ ... ]
> > @@ -235,6 +350,15 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
> >  			io = ioremap(pdev->rom, pdev->romlen);
> >  			x_start = pdev->romlen;
> >  		}
> > +#ifdef CONFIG_ACPI
> > +		if (!io) {
> > +			done = vfio_pci_acpi_rom_rw(vdev, buf, count,
> > +						    pos, iswrite);
> > +			if (done >= 0)
> > +				*ppos += done;
> > +			return done;
> > +		}
> > +#endif
> 
> [Severity: Medium]
> When reading past the actual size of the ACPI ROM but within the reported
> roundup_pow_of_two region size, does this bypass the standard VFIO
> padding logic?
> 
> If vfio_pci_acpi_rom_rw() returns a short read or 0 (EOF), will this skip
> the vfio_pci_core_do_io_rw() logic that safely pads remaining space with
> 0xFF, potentially breaking virtualization software expecting standard
> hardware behavior?

right better copy

> >  		if (!io)
> >  			return -ENOMEM;
> >  		x_end = end;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/31aa2fb273d7ff7ea535c74e0780e279aa9a119c.1791933554.git.mst@redhat.com?part=1