[PATCH 00/14] fbdev: fix various memory leaks

[PATCH 00/14] fbdev: fix various memory leaks

[Abdun Nihaal]

This patchset fixes some memory leak issues present in fbdev drivers.

Since commit 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
fb_deferred_io_init() allocated memory for pagerefs and returned an
error code, but the existing drivers which call fb_deferred_io_init()
were not updated to do cleanup. The first three commits address this.
- fbdev: hecubafb: fix potential memory leak in hecubafb_probe()
- fbdev: broadsheetfb: fix potential memory leak in broadsheetfb_probe()
- fbdev: metronomefb: fix potential memory leak in metronomefb_probe()

Probe functions that call fb_add_videomode() or fb_videomode_to_modelist() 
sometimes did not call fb_destry_modelist() to free the allocated
memory. The following patches address this issue.
- fbdev: radeon: fix potential memory leak in radeonfb_pci_register()
- fbdev: carminefb: fix potential memory leak in alloc_carmine_fb()
- fbdev: i740fb: fix potential memory leak in i740fb_probe()
- fbdev: nvidia: fix potential memory leak in nvidiafb_probe()
- fbdev: s3fb: fix potential memory leak in s3_pci_probe()
- fbdev: tdfxfb: fix potential memory leak in tdfxfb_probe()
- fbdev: tridentfb: fix potential memory leak in trident_pci_probe()
- fbdev: uvesafb: fix potential memory leak in uvesafb_probe()

Since commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; clarify ownership")
the fb_get_options() function transfers ownership of the memory
allocated for options, and so the caller is expected to free it. The
following two patches address this issue.
- fbdev: efifb: fix memory leak in efifb_probe()
- fbdev: vesafb: fix memory leak in vesafb_probe()

The following commit fixes a simple memory leak.
- fbdev: sm501fb: fix potential memory leak in sm501fb_probe()

All the patches were only compile tested.
The issues were found using static analysis.

Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
Abdun Nihaal (14):
      fbdev: hecubafb: fix potential memory leak in hecubafb_probe()
      fbdev: broadsheetfb: fix potential memory leak in broadsheetfb_probe()
      fbdev: metronomefb: fix potential memory leak in metronomefb_probe()
      fbdev: radeon: fix potential memory leak in radeonfb_pci_register()
      fbdev: carminefb: fix potential memory leak in alloc_carmine_fb()
      fbdev: i740fb: fix potential memory leak in i740fb_probe()
      fbdev: nvidia: fix potential memory leak in nvidiafb_probe()
      fbdev: s3fb: fix potential memory leak in s3_pci_probe()
      fbdev: tdfxfb: fix potential memory leak in tdfxfb_probe()
      fbdev: tridentfb: fix potential memory leak in trident_pci_probe()
      fbdev: uvesafb: fix potential memory leak in uvesafb_probe()
      fbdev: efifb: fix memory leak in efifb_probe()
      fbdev: vesafb: fix memory leak in vesafb_probe()
      fbdev: sm501fb: fix potential memory leak in sm501fb_probe()

 drivers/video/fbdev/aty/radeon_base.c | 1 +
 drivers/video/fbdev/broadsheetfb.c    | 8 ++++++--
 drivers/video/fbdev/carminefb.c       | 1 +
 drivers/video/fbdev/efifb.c           | 1 +
 drivers/video/fbdev/hecubafb.c        | 6 +++++-
 drivers/video/fbdev/i740fb.c          | 1 +
 drivers/video/fbdev/metronomefb.c     | 8 ++++++--
 drivers/video/fbdev/nvidia/nvidia.c   | 1 +
 drivers/video/fbdev/s3fb.c            | 1 +
 drivers/video/fbdev/sm501fb.c         | 3 +++
 drivers/video/fbdev/tdfxfb.c          | 1 +
 drivers/video/fbdev/tridentfb.c       | 1 +
 drivers/video/fbdev/uvesafb.c         | 4 ++--
 drivers/video/fbdev/vesafb.c          | 1 +
 14 files changed, 31 insertions(+), 7 deletions(-)
---
base-commit: ba2e787b4814ebf9ab5f6a84181678b67eb3677b
change-id: 20260513-fbdev-99a53dc0754f

Best regards,
-- 
Abdun Nihaal <nihaal@cse.iitm.ac.in>

[PATCH 01/14] fbdev: hecubafb: fix potential memory leak in hecubafb_probe()

[Abdun Nihaal]

The memory allocated for pagerefs in fb_deferred_io_init() is not freed
on the error path. Fix it by calling fb_deferred_io_cleanup().

Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/hecubafb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/hecubafb.c b/drivers/video/fbdev/hecubafb.c
index 3547d58a29cf..dd2af980f3d8 100644
--- a/drivers/video/fbdev/hecubafb.c
+++ b/drivers/video/fbdev/hecubafb.c
@@ -192,7 +192,9 @@ static int hecubafb_probe(struct platform_device *dev)
 	info->flags = FBINFO_VIRTFB;
 
 	info->fbdefio = &hecubafb_defio;
-	fb_deferred_io_init(info);
+	retval = fb_deferred_io_init(info);
+	if (retval)
+		goto err_fbdefio;
 
 	retval = register_framebuffer(info);
 	if (retval < 0)
@@ -209,6 +211,8 @@ static int hecubafb_probe(struct platform_device *dev)
 
 	return 0;
 err_fbreg:
+	fb_deferred_io_cleanup(info);
+err_fbdefio:
 	framebuffer_release(info);
 err_fballoc:
 	vfree(videomemory);

-- 
2.43.0

[PATCH 02/14] fbdev: broadsheetfb: fix potential memory leak in broadsheetfb_probe()

[Abdun Nihaal]

The memory allocated for pagerefs in fb_deferred_io_init() is not freed
on the error path. Fix it by calling fb_deferred_io_cleanup().

Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/broadsheetfb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index c8ba098a8c42..582f1ee4c9b6 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -1072,12 +1072,14 @@ static int broadsheetfb_probe(struct platform_device *dev)
 	info->flags = FBINFO_VIRTFB;
 
 	info->fbdefio = &broadsheetfb_defio;
-	fb_deferred_io_init(info);
+	retval = fb_deferred_io_init(info);
+	if (retval)
+		goto err_vfree;
 
 	retval = fb_alloc_cmap(&info->cmap, 16, 0);
 	if (retval < 0) {
 		dev_err(&dev->dev, "Failed to allocate colormap\n");
-		goto err_vfree;
+		goto err_fbdefio;
 	}
 
 	/* set cmap */
@@ -1121,6 +1123,8 @@ static int broadsheetfb_probe(struct platform_device *dev)
 	board->cleanup(par);
 err_cmap:
 	fb_dealloc_cmap(&info->cmap);
+err_fbdefio:
+	fb_deferred_io_cleanup(info);
 err_vfree:
 	vfree(videomemory);
 err_fb_rel:

-- 
2.43.0

[PATCH 03/14] fbdev: metronomefb: fix potential memory leak in metronomefb_probe()

[Abdun Nihaal]

The memory allocated for pagerefs in fb_deferred_io_init() is not freed
on the error path. Fix it by calling fb_deferred_io_cleanup().

Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/metronomefb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index 6f0942c6e5f1..83c614963a0a 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -645,12 +645,14 @@ static int metronomefb_probe(struct platform_device *dev)
 	info->flags = FBINFO_VIRTFB;
 
 	info->fbdefio = &metronomefb_defio;
-	fb_deferred_io_init(info);
+	retval = fb_deferred_io_init(info);
+	if (retval)
+		goto err_free_irq;
 
 	retval = fb_alloc_cmap(&info->cmap, 8, 0);
 	if (retval < 0) {
 		dev_err(&dev->dev, "Failed to allocate colormap\n");
-		goto err_free_irq;
+		goto err_fbdefio;
 	}
 
 	/* set cmap */
@@ -673,6 +675,8 @@ static int metronomefb_probe(struct platform_device *dev)
 
 err_cmap:
 	fb_dealloc_cmap(&info->cmap);
+err_fbdefio:
+	fb_deferred_io_cleanup(info);
 err_free_irq:
 	board->cleanup(par);
 err_csum_table:

-- 
2.43.0

[PATCH 04/14] fbdev: radeon: fix potential memory leak in radeonfb_pci_register()

[Abdun Nihaal]

The function radeonfb_pci_register() allocates memory for modelist
(by calling radeon_check_modes() which calls fb_add_videomode()).
The memory is appended to info->modelist, but is not freed in subsequent
error paths. Fix this by calling fb_destroy_modelist().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/aty/radeon_base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c
index adb03489bedf..b6b058cee751 100644
--- a/drivers/video/fbdev/aty/radeon_base.c
+++ b/drivers/video/fbdev/aty/radeon_base.c
@@ -2476,6 +2476,7 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
 	return 0;
 err_unmap_fb:
 	iounmap(rinfo->fb_base);
+	fb_destroy_modelist(&info->modelist);
 err_unmap_rom:
 	kfree(rinfo->mon1_EDID);
 	kfree(rinfo->mon2_EDID);

-- 
2.43.0

[PATCH 05/14] fbdev: carminefb: fix potential memory leak in alloc_carmine_fb()

[Abdun Nihaal]

The memory allocated for modelist in fb_videomode_to_modelist() is not
freed in the subsequent error path.
Fix that by calling fb_destroy_modelist()

Fixes: 2ece5f43b041 ("fbdev: add the carmine FB driver")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/carminefb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/carminefb.c b/drivers/video/fbdev/carminefb.c
index 5f13f1cc79d3..fca50b7961eb 100644
--- a/drivers/video/fbdev/carminefb.c
+++ b/drivers/video/fbdev/carminefb.c
@@ -589,6 +589,7 @@ static int alloc_carmine_fb(void __iomem *regs, void __iomem *smem_base,
 	return 0;
 
 err_dealloc_cmap:
+	fb_destroy_modelist(&info->modelist);
 	fb_dealloc_cmap(&info->cmap);
 err_free_fb:
 	framebuffer_release(info);

-- 
2.43.0

[PATCH 06/14] fbdev: i740fb: fix potential memory leak in i740fb_probe()

[Abdun Nihaal]

In i740fb_probe(), the memory allocated in fb_videomode_to_modelist()
for modelist is not freed in the error paths. Fix that by calling
fb_destroy_modelist().

Fixes: 5350c65f4f15 ("Resurrect Intel740 driver: i740fb")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/i740fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c
index 9b74dae71472..c14a19382769 100644
--- a/drivers/video/fbdev/i740fb.c
+++ b/drivers/video/fbdev/i740fb.c
@@ -1152,6 +1152,7 @@ static int i740fb_probe(struct pci_dev *dev, const struct pci_device_id *ent)
 	fb_dealloc_cmap(&info->cmap);
 err_alloc_cmap:
 err_find_mode:
+	fb_destroy_modelist(&info->modelist);
 	if (par->ddc_registered)
 		i2c_del_adapter(&par->ddc_adapter);
 	pci_iounmap(dev, par->regs);

-- 
2.43.0

[PATCH 07/14] fbdev: nvidia: fix potential memory leak in nvidiafb_probe()

[Abdun Nihaal]

In nvidiafb_probe(), the memory allocated for modelist in
nvidia_set_fbinfo() is not freed in the subsequent error paths.
Fix that by calling fb_destroy_modelist().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/nvidia/nvidia.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/nvidia/nvidia.c b/drivers/video/fbdev/nvidia/nvidia.c
index da2d486022e8..7d20c4087aeb 100644
--- a/drivers/video/fbdev/nvidia/nvidia.c
+++ b/drivers/video/fbdev/nvidia/nvidia.c
@@ -1424,6 +1424,7 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
 
 err_out_iounmap_fb:
 	iounmap(info->screen_base);
+	fb_destroy_modelist(&info->modelist);
 err_out_free_base1:
 	fb_destroy_modedb(info->monspecs.modedb);
 	nvidia_delete_i2c_busses(par);

-- 
2.43.0

[PATCH 08/14] fbdev: s3fb: fix potential memory leak in s3_pci_probe()

[Abdun Nihaal]

In s3_pci_probe(), the memory allocated for modelist using
fb_videomode_to_modelist() is not freed in subsequent error paths.
Fix that by calling fb_destroy_modelist()

Fixes: 86c0f043a737 ("s3fb: add DDC support")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/s3fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/s3fb.c b/drivers/video/fbdev/s3fb.c
index 831e9e6861b1..dc1f9b627185 100644
--- a/drivers/video/fbdev/s3fb.c
+++ b/drivers/video/fbdev/s3fb.c
@@ -1446,6 +1446,7 @@ static int s3_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 err_alloc_cmap:
 err_find_mode:
 #ifdef CONFIG_FB_S3_DDC
+	fb_destroy_modelist(&info->modelist);
 	if (par->ddc_registered)
 		i2c_del_adapter(&par->ddc_adapter);
 	if (par->mmio)

-- 
2.43.0

[PATCH 09/14] fbdev: tdfxfb: fix potential memory leak in tdfxfb_probe()

[Abdun Nihaal]

In tdfxfb_probe(), the memory allocated for modelist using
fb_videomode_to_modelist() when CONFIG_FB_3DFX_I2C is defined, is not
freed in the subsequent error paths.
Fix that by calling fb_destroy_modelist().

Fixes: 215059d2421f ("tdfxfb: make use of DDC information about connected monitor")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/tdfxfb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index a6b63c09b48f..cc6a074f3165 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -1552,6 +1552,7 @@ static int tdfxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 out_err_iobase:
 #ifdef CONFIG_FB_3DFX_I2C
+	fb_destroy_modelist(&info->modelist);
 	tdfxfb_delete_i2c_busses(default_par);
 #endif
 	arch_phys_wc_del(default_par->wc_cookie);

-- 
2.43.0

[PATCH 10/14] fbdev: tridentfb: fix potential memory leak in trident_pci_probe()

[Abdun Nihaal]

In trident_pci_probe(), the memory allocated for modelist using
fb_videomode_to_modelist() is not freed in subsequent error paths.
Fix that by calling fb_destroy_modelist().

Fixes: 6a5e3bd0c8bc ("tridentfb: Add DDC support")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/tridentfb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/tridentfb.c b/drivers/video/fbdev/tridentfb.c
index a8fdbae83a80..9f055ba776c8 100644
--- a/drivers/video/fbdev/tridentfb.c
+++ b/drivers/video/fbdev/tridentfb.c
@@ -1706,6 +1706,7 @@ static int trident_pci_probe(struct pci_dev *dev,
 	return 0;
 
 out_unmap2:
+	fb_destroy_modelist(&info->modelist);
 	if (default_par->ddc_registered)
 		i2c_del_adapter(&default_par->ddc_adapter);
 	kfree(info->pixmap.addr);

-- 
2.43.0

[PATCH 11/14] fbdev: uvesafb: fix potential memory leak in uvesafb_probe()

[Abdun Nihaal]

Due to an incorrect goto label, memory allocated for modedb and modelist
in uvesafb_vbe_init() is not freed in some error paths. Fix this by
updating the goto label.

Fixes: 8bdb3a2d7df4 ("uvesafb: the driver core")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/uvesafb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/uvesafb.c b/drivers/video/fbdev/uvesafb.c
index 88667fccc27b..9d82326c744f 100644
--- a/drivers/video/fbdev/uvesafb.c
+++ b/drivers/video/fbdev/uvesafb.c
@@ -1694,14 +1694,14 @@ static int uvesafb_probe(struct platform_device *dev)
 	i = uvesafb_vbe_init_mode(info);
 	if (i < 0) {
 		err = -EINVAL;
-		goto out;
+		goto out_mode;
 	} else {
 		mode = &par->vbe_modes[i];
 	}
 
 	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
 		err = -ENXIO;
-		goto out;
+		goto out_mode;
 	}
 
 	uvesafb_init_info(info, mode);

-- 
2.43.0

[PATCH 12/14] fbdev: efifb: fix memory leak in efifb_probe()

[Abdun Nihaal]

Since commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to
caller; clarify ownership") the string returned from fb_get_options()
is expected to be freed by the caller, but the string is not freed in
efifb_probe(). Fix that by freeing the option string after setup.

Fixes: 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; clarify ownership")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/efifb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 47ebc0107209..ad8dec7807c3 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -377,6 +377,7 @@ static int efifb_probe(struct platform_device *dev)
 	if (fb_get_options("efifb", &option))
 		return -ENODEV;
 	efifb_setup(si, option);
+	kfree(option);
 
 	/* We don't get linelength from UGA Draw Protocol, only from
 	 * EFI Graphics Protocol.  So if it's not in DMI, and it's not

-- 
2.43.0

[PATCH 13/14] fbdev: vesafb: fix memory leak in vesafb_probe()

[Abdun Nihaal]

Since commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to
caller; clarify ownership") the string returned from fb_get_options()
is expected to be freed by the caller. But the string is not freed in
vesafb_probe(). Fix that by freeing the option string after setup.

Fixes: 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; clarify ownership")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/vesafb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/vesafb.c b/drivers/video/fbdev/vesafb.c
index f84f4db244bf..f1902056bd73 100644
--- a/drivers/video/fbdev/vesafb.c
+++ b/drivers/video/fbdev/vesafb.c
@@ -269,6 +269,7 @@ static int vesafb_probe(struct platform_device *dev)
 	/* ignore error return of fb_get_options */
 	fb_get_options("vesafb", &option);
 	vesafb_setup(option);
+	kfree(option);
 
 	if (si->orig_video_isVGA != VIDEO_TYPE_VLFB)
 		return -ENODEV;

-- 
2.43.0

[PATCH 14/14] fbdev: sm501fb: fix potential memory leak in sm501fb_probe()

[Abdun Nihaal]

The memory allocated for info->edid_data in sm501fb_probe() when
CONFIG_OF is defined is not freed in the subsequent error paths.
Fix that by freeing it in the error path if CONFIG_OF is defined.

Fixes: 4295f9bf74a8 ("video, sm501: add OF binding to support SM501")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/video/fbdev/sm501fb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
index fee4b9f84592..1ee7842517b8 100644
--- a/drivers/video/fbdev/sm501fb.c
+++ b/drivers/video/fbdev/sm501fb.c
@@ -2048,6 +2048,9 @@ static int sm501fb_probe(struct platform_device *pdev)
 	framebuffer_release(info->fb[HEAD_CRT]);
 
 err_alloc:
+#if defined(CONFIG_OF)
+	kfree(info->edid_data);
+#endif
 	kfree(info);
 
 	return ret;

-- 
2.43.0

Re: [PATCH 12/14] fbdev: efifb: fix memory leak in efifb_probe()

[Thomas Zimmermann]

Hi

Am 14.05.26 um 10:24 schrieb Abdun Nihaal:
> Since commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to
> caller; clarify ownership") the string returned from fb_get_options()
> is expected to be freed by the caller, but the string is not freed in
> efifb_probe(). Fix that by freeing the option string after setup.
>
> Fixes: 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; clarify ownership")

Right, the commit message even mentions the (pre-existing) leak. There 
might be other drivers with a similar issue.

> Cc: stable@vger.kernel.org
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/efifb.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 47ebc0107209..ad8dec7807c3 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -377,6 +377,7 @@ static int efifb_probe(struct platform_device *dev)
>   	if (fb_get_options("efifb", &option))
>   		return -ENODEV;
>   	efifb_setup(si, option);
> +	kfree(option);
>   
>   	/* We don't get linelength from UGA Draw Protocol, only from
>   	 * EFI Graphics Protocol.  So if it's not in DMI, and it's not
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 13/14] fbdev: vesafb: fix memory leak in vesafb_probe()

[Thomas Zimmermann]

Am 14.05.26 um 10:24 schrieb Abdun Nihaal:
> Since commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to
> caller; clarify ownership") the string returned from fb_get_options()
> is expected to be freed by the caller. But the string is not freed in
> vesafb_probe(). Fix that by freeing the option string after setup.
>
> Fixes: 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller; clarify ownership")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/vesafb.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/fbdev/vesafb.c b/drivers/video/fbdev/vesafb.c
> index f84f4db244bf..f1902056bd73 100644
> --- a/drivers/video/fbdev/vesafb.c
> +++ b/drivers/video/fbdev/vesafb.c
> @@ -269,6 +269,7 @@ static int vesafb_probe(struct platform_device *dev)
>   	/* ignore error return of fb_get_options */
>   	fb_get_options("vesafb", &option);
>   	vesafb_setup(option);
> +	kfree(option);
>   
>   	if (si->orig_video_isVGA != VIDEO_TYPE_VLFB)
>   		return -ENODEV;
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 03/14] fbdev: metronomefb: fix potential memory leak in metronomefb_probe()

[Thomas Zimmermann]

Am 14.05.26 um 10:24 schrieb Abdun Nihaal:
> The memory allocated for pagerefs in fb_deferred_io_init() is not freed
> on the error path. Fix it by calling fb_deferred_io_cleanup().
>
> Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/metronomefb.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
> index 6f0942c6e5f1..83c614963a0a 100644
> --- a/drivers/video/fbdev/metronomefb.c
> +++ b/drivers/video/fbdev/metronomefb.c
> @@ -645,12 +645,14 @@ static int metronomefb_probe(struct platform_device *dev)
>   	info->flags = FBINFO_VIRTFB;
>   
>   	info->fbdefio = &metronomefb_defio;
> -	fb_deferred_io_init(info);
> +	retval = fb_deferred_io_init(info);
> +	if (retval)
> +		goto err_free_irq;
>   
>   	retval = fb_alloc_cmap(&info->cmap, 8, 0);
>   	if (retval < 0) {
>   		dev_err(&dev->dev, "Failed to allocate colormap\n");
> -		goto err_free_irq;
> +		goto err_fbdefio;
>   	}
>   
>   	/* set cmap */
> @@ -673,6 +675,8 @@ static int metronomefb_probe(struct platform_device *dev)
>   
>   err_cmap:
>   	fb_dealloc_cmap(&info->cmap);
> +err_fbdefio:
> +	fb_deferred_io_cleanup(info);
>   err_free_irq:
>   	board->cleanup(par);
>   err_csum_table:
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 02/14] fbdev: broadsheetfb: fix potential memory leak in broadsheetfb_probe()

[Thomas Zimmermann]

Am 14.05.26 um 10:24 schrieb Abdun Nihaal:
> The memory allocated for pagerefs in fb_deferred_io_init() is not freed
> on the error path. Fix it by calling fb_deferred_io_cleanup().
>
> Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/broadsheetfb.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
> index c8ba098a8c42..582f1ee4c9b6 100644
> --- a/drivers/video/fbdev/broadsheetfb.c
> +++ b/drivers/video/fbdev/broadsheetfb.c
> @@ -1072,12 +1072,14 @@ static int broadsheetfb_probe(struct platform_device *dev)
>   	info->flags = FBINFO_VIRTFB;
>   
>   	info->fbdefio = &broadsheetfb_defio;
> -	fb_deferred_io_init(info);
> +	retval = fb_deferred_io_init(info);
> +	if (retval)
> +		goto err_vfree;
>   
>   	retval = fb_alloc_cmap(&info->cmap, 16, 0);
>   	if (retval < 0) {
>   		dev_err(&dev->dev, "Failed to allocate colormap\n");
> -		goto err_vfree;
> +		goto err_fbdefio;
>   	}
>   
>   	/* set cmap */
> @@ -1121,6 +1123,8 @@ static int broadsheetfb_probe(struct platform_device *dev)
>   	board->cleanup(par);
>   err_cmap:
>   	fb_dealloc_cmap(&info->cmap);
> +err_fbdefio:
> +	fb_deferred_io_cleanup(info);
>   err_vfree:
>   	vfree(videomemory);
>   err_fb_rel:
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 01/14] fbdev: hecubafb: fix potential memory leak in hecubafb_probe()

[Thomas Zimmermann]

Am 14.05.26 um 10:24 schrieb Abdun Nihaal:
> The memory allocated for pagerefs in fb_deferred_io_init() is not freed
> on the error path. Fix it by calling fb_deferred_io_cleanup().
>
> Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/video/fbdev/hecubafb.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/hecubafb.c b/drivers/video/fbdev/hecubafb.c
> index 3547d58a29cf..dd2af980f3d8 100644
> --- a/drivers/video/fbdev/hecubafb.c
> +++ b/drivers/video/fbdev/hecubafb.c
> @@ -192,7 +192,9 @@ static int hecubafb_probe(struct platform_device *dev)
>   	info->flags = FBINFO_VIRTFB;
>   
>   	info->fbdefio = &hecubafb_defio;
> -	fb_deferred_io_init(info);
> +	retval = fb_deferred_io_init(info);
> +	if (retval)
> +		goto err_fbdefio;
>   
>   	retval = register_framebuffer(info);
>   	if (retval < 0)
> @@ -209,6 +211,8 @@ static int hecubafb_probe(struct platform_device *dev)
>   
>   	return 0;
>   err_fbreg:
> +	fb_deferred_io_cleanup(info);
> +err_fbdefio:
>   	framebuffer_release(info);
>   err_fballoc:
>   	vfree(videomemory);
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Claude review: fbdev: fix various memory leaks

[Claude Code Review Bot]

Overall Series Review

Subject: fbdev: fix various memory leaks
Author: Abdun Nihaal <nihaal@cse.iitm.ac.in>
Patches: 20
Reviewed: 2026-05-16T11:10:39.648581

---

This is a 14-patch series from Abdun Nihaal fixing various memory leaks in fbdev drivers, found via static analysis. The series falls into four categories: (1) missing cleanup of `fb_deferred_io_init()` pageref allocations, (2) missing `fb_destroy_modelist()` on error paths, (3) missing `kfree()` of option strings from `fb_get_options()`, and (4) a missing `kfree()` of EDID data. All patches were compile-tested only.

Overall the series is well-structured, clearly described, and the cover letter does a good job grouping the fixes by root cause. Most patches are straightforward and correct. However, there are issues in patches 4 (radeon), 7 (nvidia), and 14 (sm501fb) that need attention, and patches 12 and 13 (efifb/vesafb) need verification of the ownership claim about `fb_get_options()`.

**Verdict: Mostly good, a few patches need revision or closer scrutiny.**

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: hecubafb: fix potential memory leak in hecubafb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

The patch captures the return value of `fb_deferred_io_init()` and adds a new error label `err_fbdefio` that skips `fb_deferred_io_cleanup()` (since init failed), and adds `fb_deferred_io_cleanup()` to the existing `err_fbreg` fallthrough path:

```c
-	fb_deferred_io_init(info);
+	retval = fb_deferred_io_init(info);
+	if (retval)
+		goto err_fbdefio;
```

```c
 err_fbreg:
+	fb_deferred_io_cleanup(info);
+err_fbdefio:
 	framebuffer_release(info);
```

The error ordering is correct: if `fb_deferred_io_init()` fails, we skip cleanup and go directly to `framebuffer_release()`. If `register_framebuffer()` fails, we call `fb_deferred_io_cleanup()` first, then release the framebuffer. The Fixes tag and stable Cc are appropriate.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: broadsheetfb: fix potential memory leak in broadsheetfb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

Same pattern as patch 1 — captures return value of `fb_deferred_io_init()`, adds `err_fbdefio` label between `err_cmap` and `err_vfree`:

```c
 err_cmap:
 	fb_dealloc_cmap(&info->cmap);
+err_fbdefio:
+	fb_deferred_io_cleanup(info);
 err_vfree:
```

The label ordering is correct: `fb_alloc_cmap()` failure now goes to `err_fbdefio` (cleaning up deferred IO before vfree), and `err_cmap` falls through to `err_fbdefio`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: metronomefb: fix potential memory leak in metronomefb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

Same pattern as patches 1 and 2. Adds `err_fbdefio` between `err_cmap` and `err_free_irq`:

```c
 err_cmap:
 	fb_dealloc_cmap(&info->cmap);
+err_fbdefio:
+	fb_deferred_io_cleanup(info);
 err_free_irq:
 	board->cleanup(par);
```

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: radeon: fix potential memory leak in radeonfb_pci_register()

[Claude Code Review Bot]

Patch Review

**Status: Needs revision — placement is correct but incomplete**

The patch adds `fb_destroy_modelist()` between `err_unmap_fb` and `err_unmap_rom`:

```c
 err_unmap_fb:
 	iounmap(rinfo->fb_base);
+	fb_destroy_modelist(&info->modelist);
 err_unmap_rom:
```

I verified in the source tree that `radeon_check_modes()` (which populates the modelist via `fb_add_videomode()`) is called at line 2425, and the only `goto err_unmap_rom` is at line 2373 (before the modelist is populated), while the only `goto err_unmap_fb` is at line 2460 (after). So the placement between the two labels is correct — jumps to `err_unmap_rom` bypass the cleanup (modelist not yet populated), while jumps to `err_unmap_fb` fall through it.

However, the **normal remove path** (`radeonfb_pci_unregister()`) should also be checked. If it doesn't call `fb_destroy_modelist()`, that's a separate leak on module unload.

The Fixes tag citing `1da177e4c3f4 ("Linux-2.6.12-rc2")` is technically correct but a bit odd — this is the initial Linux git import. A more precise Fixes tag would reference the commit that added the `fb_add_videomode()` call in `radeon_check_modes()`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: carminefb: fix potential memory leak in alloc_carmine_fb()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

```c
 err_dealloc_cmap:
+	fb_destroy_modelist(&info->modelist);
 	fb_dealloc_cmap(&info->cmap);
```

Simple and correct. The `fb_videomode_to_modelist()` call populates the modelist before the error path, and this adds the missing cleanup.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: i740fb: fix potential memory leak in i740fb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

```c
 err_find_mode:
+	fb_destroy_modelist(&info->modelist);
 	if (par->ddc_registered)
 		i2c_del_adapter(&par->ddc_adapter);
```

The modelist is populated before any of the error paths that reach `err_find_mode`, so this is correctly placed.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: nvidia: fix potential memory leak in nvidiafb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Needs revision — placement concern**

```c
 err_out_iounmap_fb:
 	iounmap(info->screen_base);
+	fb_destroy_modelist(&info->modelist);
 err_out_free_base1:
 	fb_destroy_modedb(info->monspecs.modedb);
```

The modelist is populated inside `nvidia_set_fbinfo()`. From the source tree, I confirmed `nvidia_set_fbinfo()` is called at line 1397, and error paths reaching `err_out_iounmap_fb` are only after that point. So the placement between `err_out_iounmap_fb` and `err_out_free_base1` is correct.

However, there's a subtle issue: `nvidia_set_fbinfo()` itself can fail (return < 0) and the code does `goto err_out_iounmap_fb`. If `nvidia_set_fbinfo()` partially populates the modelist before failing, the cleanup is still correct (iterating an incomplete list is fine). But if `fb_videomode_to_modelist()` is called early in `nvidia_set_fbinfo()` and a later step in that function fails, the modelist destruction here is needed and correct.

Same concern as radeon about the Fixes tag citing `1da177e4c3f4`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: s3fb: fix potential memory leak in s3_pci_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

```c
 #ifdef CONFIG_FB_S3_DDC
+	fb_destroy_modelist(&info->modelist);
 	if (par->ddc_registered)
 		i2c_del_adapter(&par->ddc_adapter);
```

The `fb_videomode_to_modelist()` call is under `CONFIG_FB_S3_DDC`, and the cleanup is also under the same `#ifdef`. This is correct.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: tdfxfb: fix potential memory leak in tdfxfb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

```c
 #ifdef CONFIG_FB_3DFX_I2C
+	fb_destroy_modelist(&info->modelist);
 	tdfxfb_delete_i2c_busses(default_par);
 #endif
```

Same pattern as s3fb — the modelist population and cleanup are both under the same config guard.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: tridentfb: fix potential memory leak in trident_pci_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

```c
 out_unmap2:
+	fb_destroy_modelist(&info->modelist);
 	if (default_par->ddc_registered)
 		i2c_del_adapter(&default_par->ddc_adapter);
```

Straightforward and correctly placed.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: uvesafb: fix potential memory leak in uvesafb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

This is the most interesting fix in the series. Instead of adding a new cleanup call, it fixes incorrect `goto` labels:

```c
-		goto out;
+		goto out_mode;
```

I verified in the source tree that `out_mode` (at line 1759) performs modelist destruction, modedb destruction, and cmap deallocation, while `out` (at line 1764) only frees `par->vbe_modes` and releases the framebuffer. Since `uvesafb_vbe_init()` populates the modelist and modedb before these error points, the `goto out` was indeed skipping necessary cleanup. The fix to `goto out_mode` is correct.

Note: calling `fb_dealloc_cmap()` at `out_mode` when `fb_alloc_cmap()` hasn't been called yet (in the `uvesafb_vbe_init_mode()` failure case) is safe — `fb_dealloc_cmap()` handles uninitialized cmaps gracefully.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: efifb: fix memory leak in efifb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct, but needs verification of the Fixes claim**

```c
 efifb_setup(si, option);
+kfree(option);
```

I verified that `fb_get_options()` calls `kstrdup()` to duplicate the option string, giving ownership to the caller. I also verified that `efifb_setup()` passes `options` by value to `strsep()`, which modifies only the local copy of the pointer — the caller's `option` variable still points to the start of the allocated buffer. So `kfree(option)` is safe.

The key question is whether the referenced commit `73ce73c30ba9` actually changed `fb_get_options()` to use `kstrdup()`. The current code in `fb_cmdline.c` does indeed use `kstrdup()` with a comment saying "The caller owns the string returned in @option and is responsible for releasing the memory." This confirms the fix is correct.

Note this does NOT leak on the `fb_get_options()` failure path since that returns `-ENODEV` before reaching the `kfree`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: vesafb: fix memory leak in vesafb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Correct**

```c
 vesafb_setup(option);
+kfree(option);
```

Same analysis as patch 12. `vesafb_setup()` also uses `strsep()` on a local copy, so `option` still points to the original buffer. Safe to free.

Note the comment `/* ignore error return of fb_get_options */` — even if `fb_get_options()` "fails" (returns 1), it still sets `*option` to `NULL` (or a kstrdup'd string). `kfree(NULL)` is a no-op, so this is safe in all paths.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: sm501fb: fix potential memory leak in sm501fb_probe()

[Claude Code Review Bot]

Patch Review

**Status: Needs revision — potential double-free**

```c
 err_alloc:
+#if defined(CONFIG_OF)
+	kfree(info->edid_data);
+#endif
 	kfree(info);
```

I verified in the source tree that `info->edid_data` is allocated via `kmemdup()` under `#if defined(CONFIG_OF)` in `sm501fb_probe()`. However, I found a critical issue:

**`sm501fb_init_fb()` (called from `sm501fb_start()`) already frees `info->edid_data` in certain code paths without setting it to NULL:**

```c
if (info->edid_data) {
    ret = fb_find_mode(...);
    kfree(info->edid_data);
    /* edid_data is NOT set to NULL here! */
}
```

If `sm501fb_start()` succeeds (calling `sm501fb_init_fb()` which frees `edid_data`), but a later step like `register_framebuffer()` fails, the error path goes through `err_started` → `err_probed_panel` → `err_probed_crt` → `err_alloc`, which would call `kfree(info->edid_data)` on an already-freed pointer — a **double-free bug**.

**Recommended fix:** Either:
1. Add `info->edid_data = NULL` after the `kfree()` in `sm501fb_init_fb()` as a separate preparatory patch, or
2. Use `kfree(info->edid_data); info->edid_data = NULL;` pattern here and in `sm501fb_init_fb()`.

Also, the `#if defined(CONFIG_OF)` guard is unnecessary since `edid_data` is unconditionally defined in `struct sm501fb_info` and `kfree(NULL)` is a no-op. Removing the guard would be cleaner.

---
Generated by Claude Code Patch Reviewer