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

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

[Myeonghun Pak]

tdfxfb_probe() enables the PCI device with pci_enable_device(), but
several failure paths after that point return without disabling it. The
framebuffer_alloc() failure path returns -ENOMEM directly, and the later
shared out_err path releases the framebuffer and returns -ENXIO without
balancing the PCI enable state.

The successful probe path has the same imbalance because tdfxfb_remove()
releases the framebuffer, mappings and regions, but never calls
pci_disable_device().

Use pcim_enable_device() so the PCI device is disabled automatically on
probe failure and driver detach.

This issue was identified during our ongoing static-analysis research while
reviewing kernel code.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/video/fbdev/tdfxfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index cc6a074f3165..7f7c268c3b0d 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;
-- 
2.47.1

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

[sashiko-bot]

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