Re: [PATCH] PCI: vgaarb: Include 0x0380 devices in default selection

[sashiko-bot@kernel.org]

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

New issues:
- [High] Including non-VGA display devices (0x0380) in the VGA arbiter incorrectly subjects them to legacy VGA arbitration, breaking standard MMIO accesses.

Pre-existing issues:
- [High] Concurrent calls to `vga_set_default_device` and unprotected reads in `vga_default_device` cause double-puts and use-after-free vulnerabilities.
- [High] A race condition during early boot between `bus_register_notifier` and PCI device enumeration causes a kernel panic (`BUG_ON`).
--

commit fe1f6cb8be8bef939febe2357fcf713ab76eb751
Author: Aaron Ma <aaron.ma@canonical.com>

PCI: vgaarb: Include 0x0380 devices in default selection

This commit registers 0x0380 (PCI_CLASS_DISPLAY_OTHER) devices in vgaarb
alongside legacy VGA devices. This allows firmware default devices on systems
with class 0x0380 GPUs to be properly considered by the VGA arbiter for
correct boot device selection.

> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index c360eee11dd9e..0e0878189e3d8 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -796,7 +796,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>  	}
>  
>  	if (vga_is_boot_device(vgadev)) {
> -		vgaarb_info(&pdev->dev, "setting as boot VGA device%s\n",
> +		vgaarb_info(&pdev->dev, "setting as boot display device%s\n",
>  			    vga_default_device() ?
>  			    " (overriding previous)" : "");
>  		vga_set_default_device(pdev);

[Severity: High]
This isn't a bug introduced by this patch, but does the concurrent execution
of vga_set_default_device() and vga_default_device() have protection against
double-puts and use-after-free?

Looking at vga_set_default_device(), the global vga_default pointer is swapped
and its reference count decremented without internal locking:

drivers/pci/vgaarb.c:vga_set_default_device() {
        ...
        pci_dev_put(vga_default);
        vga_default = pci_dev_get(pdev);
}

If two threads bypass the check concurrently, they could both execute
pci_dev_put() on the same pointer. At the same time, a reader calling
vga_default_device() can dereference the pointer just as it is freed.

[ ... ]

> @@ -1530,12 +1530,12 @@ static int __init vga_arb_device_init(void)
>  
>  	bus_register_notifier(&pci_bus_type, &pci_notifier);

[Severity: High]
This is a pre-existing issue, but does registering the bus notifier before
the initialization loop finishes introduce a race condition?

In vga_arb_device_init(), the pci_notifier is registered right before
iterating over the PCI devices. If a VGA device is hotplugged in this narrow
window, pci_notify() will trigger vga_arbiter_add_pci_device(), adding it to
the list.

When the enumeration loop subsequently finds the same device, it calls
vga_arbiter_add_pci_device() again. The code explicitly panics if the
device is already present:

drivers/pci/vgaarb.c:vga_arbiter_add_pci_device() {
        ...
        if (vgadev_find(pdev) != NULL) {
                BUG_ON(1);
                ...
}

> -	/* Add all VGA class PCI devices by default */
> +	/* Add legacy VGA and other display controller PCI devices by default */
>  	pdev = NULL;
>  	while ((pdev =
>  		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>  			       PCI_ANY_ID, pdev)) != NULL) {
> -		if (pci_is_vga(pdev))
> +		if (pci_is_vga_or_other_display(pdev))

[Severity: High]
Does registering 0x0380 non-VGA display devices here inadvertently subject
them to legacy VGA arbitration, potentially breaking their MMIO access?

In vga_arbiter_add_pci_device(), the arbiter unconditionally assumes the
device decodes legacy VGA IO and MEM:

drivers/pci/vgaarb.c:vga_arbiter_add_pci_device() {
        ...
        vgadev->decodes = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM | ...
}

Non-VGA DRM drivers (like amdgpu for 0x0380 devices) typically do not
register a VGA client to drop these legacy decodes. When another device
requests VGA resources, __vga_tryget() believes there is a conflict and
forcefully calls pci_set_vga_state(), which disables PCI_COMMAND_MEMORY:

drivers/pci/vgaarb.c:__vga_tryget() {
        ...
        if ((match & conflict->decodes) & VGA_RSRC_LEGACY_MEM)
                pci_bits |= PCI_COMMAND_MEMORY;
        ...
        err = pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
}

Could this clear the memory enable bit on the 0x0380 device and lead to
immediate driver crashes or system hangs?

>  			vga_arbiter_add_pci_device(pdev);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618081803.2790848-1-aaron.ma@canonical.com?part=1