Re: [PATCH] PCI: vgaarb: Remove stale "not implemented" notes from docs

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

New issues:
- [Medium] The commit incorrectly removes the 'not implemented' TODO for the 'unlock all' command, even though its implementation remains fundamentally broken.

Pre-existing issues:
- [High] Use-After-Free in vga_arb_release due to unreferenced pci_dev pointers.
- [High] Permanent global VGA lock resource leak when MAX_USER_CARDS is exceeded.
- [High] Data race and Use-After-Free during unprotected vga_list iteration and concurrent FD accesses.
--

--- Patch [1]: [PATCH] PCI: vgaarb: Remove stale "not implemented" notes from docs ---
commit bbd78d4ec38a7ceee963b0e74ebab5896fceb191
Author: Christian Melendez Nuñez <[email protected]>

PCI: vgaarb: Remove stale "not implemented" notes from docs

This commit updates the vgaarb documentation and kerneldoc to remove
"TODO: not implemented yet" notes for the "unlock all" and "target
default" commands. It states that these commands are already fully handled
in vga_arb_write().

> - "unlock all" is handled in vga_arb_write() via the "all"
>   strncmp branch, which releases both VGA_RSRC_LEGACY_IO and
>   VGA_RSRC_LEGACY_MEM.

[Severity: Medium]
Is the "unlock all" command fully implemented and working as expected?

Looking at vga_arb_write(), when the "all" branch is taken, it forces both
flags:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    if (strncmp(curr_pos, "all", 3) == 0)
        io_state = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
    ...
}

But later in the same function, it checks if the user actually holds these
locks:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    if (io_state & VGA_RSRC_LEGACY_MEM && uc->mem_cnt == 0) {
        ret_val = -EINVAL;
        goto done;
    }
    ...
}

If the user holds only one type of lock, wouldn't this fail with -EINVAL
instead of unlocking the resources? Should the TODO disclaimer remain until
this logic is fixed?


> diff --git a/Documentation/gpu/vgaarbiter.rst b/Documentation/gpu/vgaarbiter.rst
> index d1e953712cc29..c3a728130b545 100644
> --- a/Documentation/gpu/vgaarbiter.rst
> +++ b/Documentation/gpu/vgaarbiter.rst
> @@ -65,8 +65,7 @@ write
>          unlock <io_state>
>                  release locks on target
>          unlock all
> -                release all locks on target held by this user (not implemented
> -                yet)
> +                release all locks on target held by this user

[Severity: High]
This is a pre-existing issue, but does closing the file descriptor lead to a
use-after-free here?

In vga_arb_write(), when a "target" command is processed, the code drops the
reference to the PCI device:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    pci_dev_put(pdev);
    ...
}

If the target PCI device is subsequently hot-unplugged and freed by the PCI
subsystem, the pointers in priv->target and priv->cards[i].pdev are left
dangling. 

When the user later closes the file descriptor, vga_arb_release() accesses
the device unconditionally:

drivers/pci/vgaarb.c:vga_arb_release() {
    ...
    vgaarb_dbg(&uc->pdev->dev, "uc->io_cnt == %d, uc->mem_cnt == %d\n",
               uc->io_cnt, uc->mem_cnt);
    ...
}

Could this dereference the freed pointer if the device was removed?


[Severity: High]
This is a pre-existing issue, but can we permanently leak a global VGA lock
if MAX_USER_CARDS is exceeded?

When vga_arb_write() processes the "target" command, it sets priv->target
before checking if the tracking array is full:

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    priv->target = pdev;
    for (i = 0; i < MAX_USER_CARDS; i++) {
        ...
    if (i == MAX_USER_CARDS) {
        ...
        ret_val =  -ENOMEM;
        goto done;
    }
    ...
}

If the user has 16 devices and targets a 17th, the function returns -ENOMEM
but leaves priv->target set to the new device.

If the user then sends a "lock io" command, it will acquire the global lock
for this new device. Because the device isn't tracked in priv->cards, does
vga_arb_release() fail to release this lock on close, leaking it permanently?


[Severity: High]
This is a pre-existing issue, but is it safe to traverse the global vga_list
without holding vga_lock?

In vga_arb_write(), when resolving a target device, it calls vgadev_find():

drivers/pci/vgaarb.c:vga_arb_write() {
    ...
    vgadev = vgadev_find(pdev);
    ...
}

Which traverses the global list:

drivers/pci/vgaarb.c:vgadev_find() {
    ...
    list_for_each_entry(vgadev, &vga_list, list) {
        ...
}

If a VGA device is concurrently hot-plugged or unplugged, could this
concurrent modification cause the iteration to dereference freed list nodes?

Additionally, should concurrent accesses to the user's private struct,
such as priv->target and priv->cards in vga_arb_write(), be protected by
priv->lock to prevent data corruption when accessed by multiple threads?

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