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

[sashiko-bot@kernel.org]

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?

> +		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?

Additionally, this loop evaluates the ACPI method repeatedly in 4KB chunks
to read the entire ROM just to determine its total size.  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?

>  	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?

> +		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?

>  		if (!io)
>  			return -ENOMEM;
>  		x_end = end;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/31aa2fb273d7ff7ea535c74e0780e279aa9a119c.1791933554.git.mst@redhat.com?part=1