Re: [PATCH] fbdev: sstfb: use managed PCI device enable

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unregistering the framebuffer after tearing down hardware mappings creates an exploitable use-after-free/unmap race condition.
- [High] Hardware is left in an active, initialized state on probe error paths because `sst_shutdown()` is missing.
- [High] `__sst_wait_idle()` contains an unbounded busy loop that will silently hang the CPU if the device becomes unresponsive.
--

commit 9516997f46dea14ef20c216ac89acefd0aaf2e36
Author: Myeonghun Pak <mhun512@gmail.com>

fbdev: sstfb: use managed PCI device enable

This commit updates sstfb_probe() to use pcim_enable_device() instead of
pci_enable_device(). This ensures that the PCI device is automatically
disabled by the PCI core after a probe failure or driver removal. The driver
still explicitly releases its other resources.

> diff --git a/drivers/video/fbdev/sstfb.c b/drivers/video/fbdev/sstfb.c
> index 2745557822f76..039279feb5bf4 100644
> --- a/drivers/video/fbdev/sstfb.c
> +++ b/drivers/video/fbdev/sstfb.c
> @@ -1332,7 +1332,8 @@ static int sstfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return err;
>  
>  	/* Enable device in PCI config. */
> -	if ((err=pci_enable_device(pdev))) {
> +	err = pcim_enable_device(pdev);
> +	if (err) {
>  		printk(KERN_ERR "cannot enable device\n");
>  		return err;
>  	}

[Severity: High]
This is a pre-existing issue, but hardware is left in an active, initialized
state on probe error paths because sst_shutdown() is missing.

During sstfb_probe(), sst_init() is called to configure the hardware. If a
subsequent initialization step fails, the error path unwinds memory but omits
calling sst_shutdown():

drivers/video/fbdev/sstfb.c:sstfb_probe() {
    ...
    if (!sst_init(info, par)) {
        printk(KERN_ERR "sstfb: Init failed\n");
        goto fail;
    }
    ...
fail:
    iounmap(info->screen_base);
    ...
}

Would this leave the PCI device fully active and configured, potentially
breaking system functionality like leaving VGA pass-through disabled?

[Severity: High]
This is a pre-existing issue, but unregistering the framebuffer after tearing
down hardware mappings creates an exploitable use-after-free/unmap race
condition.

If a user opens the framebuffer device, and then the driver is unbound,
sstfb_remove() unmaps the memory before unregistering the framebuffer:

drivers/video/fbdev/sstfb.c:sstfb_remove() {
    ...
    iounmap(info->screen_base);
    iounmap(par->mmio_vbase);
    ...
    unregister_framebuffer(info);
    ...
}

Could a user then issue an ioctl (like SSTFB_SET_VGAPASS), which calls
sstfb_ioctl() and accesses the now-unmapped memory?

[Severity: High]
This is a pre-existing issue, but __sst_wait_idle() contains an unbounded
busy loop that could silently hang the CPU if the device becomes unresponsive.

__sst_wait_idle() is called repeatedly during device operation and waits for
the busy status to clear:

drivers/video/fbdev/sstfb.c:__sst_wait_idle() {
    ...
    while(1) {
        if (__sst_read(vbase, STATUS) & STATUS_FBI_BUSY) {
    ...
}

If the PCI device is removed (e.g., via surprise unplug) or fails, the MMIO
read will return ~0. Since STATUS_FBI_BUSY is BIT(7), the condition would
remain true indefinitely. Should there be a timeout or fallback here to
prevent a permanent kernel hang?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911000835.88909-1-mhun512@gmail.com?part=1