Re: [PATCH] PCI/VGA: vgaarb: Hold pci_dev references for per-file cards

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent `lock`, `unlock`, and `trylock` commands in `vga_arb_write` cause data races on `uc->io_cnt` and `uc->mem_cnt`, leading to global resource leaks or severe CPU stalls (Denial of Service).
--

commit 17b88a219897b83c9eda1acce8ef44e9f9f96033
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

PCI/VGA: vgaarb: Hold pci_dev references for per-file cards
    
The VGA arbiter stores pci_dev pointers in each /dev/vga_arbiter file's
private state to track the target device and per-card lock counts. This
patch ensures each tracked non-NULL cards[] entry owns a pci_dev reference
to prevent KASAN use-after-free bugs if the device is removed via sysfs.

Closes: https://lore.kernel.org/r/330FD8DD-ECBF-4531-900E-5B976FA9DF90@gmail.com/

> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -1422,8 +1468,9 @@ static int vga_arb_release(struct inode *inode, struct file *file)
[ ... ]
> @@ -1439,9 +1486,15 @@ static int vga_arb_release(struct inode *inode, struct file *file)
>  		if (uc->pdev == NULL)
>  			continue;
>  		vgaarb_dbg(&uc->pdev->dev, "uc->io_cnt == %d, uc->mem_cnt == %d\n",
>  			uc->io_cnt, uc->mem_cnt);
>  		while (uc->io_cnt--)
>  			vga_put(uc->pdev, VGA_RSRC_LEGACY_IO);

[Severity: High]
This is a pre-existing issue, but can concurrent calls to vga_arb_write()
with unlock commands cause a denial of service here?

If userspace uses pwrite64 or io_uring to issue concurrent unlock commands
on the same file descriptor, two threads can simultaneously pass the
uc->io_cnt != 0 validation check in vga_arb_write().

Because the decrement happens locklessly after a blocking call to vga_put():

vga_arb_write() {
        if (io_state & VGA_RSRC_LEGACY_IO && uc->io_cnt == 0) {
                ret_val = -EINVAL;
                goto done;
        }
        ...
        vga_put(pdev, io_state);
        
        if (io_state & VGA_RSRC_LEGACY_IO)
                uc->io_cnt--;
}

Both threads can execute uc->io_cnt--, which underflows the counter to
UINT_MAX.

When the file is closed, vga_arb_release() executes this loop:

while (uc->io_cnt--)
        vga_put(uc->pdev, VGA_RSRC_LEGACY_IO);

This loop executes ~4.29 billion times while holding the vga_user_lock
spinlock with interrupts disabled, completely stalling the CPU.

Does this resource counting need to be properly serialized or converted to
atomic types to prevent a hard lockup?

>  		while (uc->mem_cnt--)
>  			vga_put(uc->pdev, VGA_RSRC_LEGACY_MEM);
> +		pdevs[nr_pdevs++] = uc->pdev;
> +		uc->pdev = NULL;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/86061f17ea5f1fb76791ca5cc787ce84b6752f46.1782788237.git.shuangpeng.kernel@gmail.com?part=1