Re: [PATCH] fbdev: tdfxfb: fix PCI enable cleanup with pcim_enable_device()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling `fb_destroy_modelist()` on a zero-initialized list head causes a kernel NULL pointer dereference during probe error cleanup.
--

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

fbdev: tdfxfb: fix PCI enable cleanup with pcim_enable_device()

tdfxfb_probe() enables the PCI device with pci_enable_device(), but
several failure paths after that point return without disabling it.
This patch uses pcim_enable_device() so the PCI device is disabled
automatically on probe failure and driver detach.

> diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
> index cc6a074f3165b..9a06cef756994 100644
> --- a/drivers/video/fbdev/tdfxfb.c
> +++ b/drivers/video/fbdev/tdfxfb.c
> @@ -1385,7 +1385,7 @@ static int tdfxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (err)
>  		return err;
>  
> -	err = pci_enable_device(pdev);
> +	err = pcim_enable_device(pdev);
>  	if (err) {
>  		printk(KERN_ERR "tdfxfb: Can't enable pdev: %d\n", err);
>  		return err;

[Severity: High]
This is a pre-existing issue, but does the out_err_iobase error path cause a 
NULL pointer dereference during device probe?

If CONFIG_FB_3DFX_I2C is enabled but the monitor's EDID does not provide a
mode database (specs->modedb == NULL), fb_videomode_to_modelist() is skipped.
This leaves the info->modelist list head zero-initialized (with next and prev
pointers set to NULL) as originally allocated by framebuffer_alloc().

If a subsequent operation fails (e.g., info->var.yres_virtual < info->var.yres
or fb_alloc_cmap() returning -ENOMEM), the driver jumps to out_err_iobase:

drivers/video/fbdev/tdfxfb.c:tdfxfb_probe() {
    ...
out_err_iobase:
#ifdef CONFIG_FB_3DFX_I2C
    fb_destroy_modelist(&info->modelist);
...
}

This unconditionally calls fb_destroy_modelist(), which uses
list_for_each_safe():

drivers/video/fbdev/core/modedb.c:fb_destroy_modelist() {
    struct list_head *pos, *n;

    list_for_each_safe(pos, n, head) {
    ...
}

Because head->next is NULL, pos evaluates to NULL when the macro evaluates
n = pos->next, resulting in an immediate NULL pointer dereference.

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