Re: [PATCH] drm/cirrus-qemu: Validate BAR0 size during probe

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The exact equality check for BAR0 size (`!= CIRRUS_VRAM_SIZE`) incorrectly rejects valid QEMU configurations with larger VRAM.
- [High] The VRAM size validation check is placed after `aperture_remove_conflicting_pci_devices()`, meaning the firmware framebuffer is permanently destroyed even if the driver ultimately fails to bind.
--

--- Patch [1]: [PATCH] drm/cirrus-qemu: Validate BAR0 size during probe ---
commit 75467ead62a494b6afbc057c666ab59f7e4b8915
Author: Slawomir Stepien <[email protected]>

drm/cirrus-qemu: Validate BAR0 size during probe

This commit adds a check during PCI probe to verify that the BAR0 resource
length equals CIRRUS_VRAM_SIZE. This is intended to prevent supervisor write
page faults caused by writing past the end of the mapped I/O memory when
smaller BARs are presented.

Link: https://syzkaller.appspot.com/ai_job?id=ba262a3a-bccf-4ad8-a1b0-583c55d34fd6

> diff --git a/drivers/gpu/drm/tiny/cirrus-qemu.c b/drivers/gpu/drm/tiny/cirrus-qemu.c
> index 075221b431d37..70c995025f508 100644
> --- a/drivers/gpu/drm/tiny/cirrus-qemu.c
> +++ b/drivers/gpu/drm/tiny/cirrus-qemu.c
> @@ -602,6 +602,9 @@ static int cirrus_pci_probe(struct pci_dev *pdev,
>  
>  	dev = &cirrus->dev;
>  
> +	if (pci_resource_len(pdev, 0) != CIRRUS_VRAM_SIZE)
> +		return -ENODEV;
> +

[Severity: High]
Does this strict equality check unintentionally break valid QEMU
configurations that have more than 4 MB of VRAM?

When a VM is booted with a configuration like -device cirrus-vga,vgamem_mb=8,
the BAR0 size will be 8 MB. The check in cirrus_pci_probe() evaluates
to true (since 8 MB is not equal to 4 MB) and rejects the device by
returning -ENODEV.

Would it be safer to check if pci_resource_len(pdev, 0) < CIRRUS_VRAM_SIZE
instead of requiring an exact match?

[Severity: High]
Is it safe to place this validation check after the call to
aperture_remove_conflicting_pci_devices() earlier in cirrus_pci_probe()?

If this new size check fails (either due to a malformed device or a larger
valid VRAM size), cirrus_pci_probe() aborts with -ENODEV. However, because
aperture_remove_conflicting_pci_devices() was already executed at the start
of the probe function, the firmware framebuffers (like efifb or simplefb)
have been permanently destroyed.

Could this leave the system with a dead display output? Would it make sense
to move this PCI configuration check to the very beginning of the probe
function, before any destructive actions occur?

>  	cirrus->vram = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0),
>  				    pci_resource_len(pdev, 0));
>  	if (cirrus->vram == NULL)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1