[PATCH v2] fbdev: platinumfb: add error checking for ioremap calls

[PATCH v2] fbdev: platinumfb: add error checking for ioremap calls

[yuebingkun]

The ioremap() and ioremap_wt() calls in platinumfb_probe() were not
checked for failure. If any of these mappings fail, the driver would
dereference NULL pointers, leading to a kernel panic.

Add proper error checking and use goto-based cleanup to avoid code
duplication across the error paths.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: yuebingkun <yuebingkun@kylinos.cn>
---
 drivers/video/fbdev/platinumfb.c | 36 ++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/platinumfb.c b/drivers/video/fbdev/platinumfb.c
index fa27a3a4f05b..fdffadcc3e73 100644
--- a/drivers/video/fbdev/platinumfb.c
+++ b/drivers/video/fbdev/platinumfb.c
@@ -567,15 +567,30 @@ static int platinumfb_probe(struct platform_device* odev)
 	/* frame buffer - map only 4MB */
 	pinfo->frame_buffer_phys = pinfo->rsrc_fb.start;
 	pinfo->frame_buffer = ioremap_wt(pinfo->rsrc_fb.start, 0x400000);
+	if (!pinfo->frame_buffer) {
+		dev_err(&odev->dev, "failed to ioremap frame buffer\n");
+		rc = -ENOMEM;
+		goto err_release_fb;
+	}
 	pinfo->base_frame_buffer = pinfo->frame_buffer;
 
 	/* registers */
 	pinfo->platinum_regs_phys = pinfo->rsrc_reg.start;
 	pinfo->platinum_regs = ioremap(pinfo->rsrc_reg.start, 0x1000);
+	if (!pinfo->platinum_regs) {
+		dev_err(&odev->dev, "failed to ioremap registers\n");
+		rc = -ENOMEM;
+		goto err_unmap_fb;
+	}
 
 	pinfo->cmap_regs_phys = 0xf301b000;	/* XXX not in prom? */
 	request_mem_region(pinfo->cmap_regs_phys, 0x1000, "platinumfb cmap");
 	pinfo->cmap_regs = ioremap(pinfo->cmap_regs_phys, 0x1000);
+	if (!pinfo->cmap_regs) {
+		dev_err(&odev->dev, "failed to ioremap cmap registers\n");
+		rc = -ENOMEM;
+		goto err_release_cmap;
+	}
 
 	/* Grok total video ram */
 	out_be32(&pinfo->platinum_regs->reg[16].r, (unsigned)pinfo->frame_buffer_phys);
@@ -623,13 +638,22 @@ static int platinumfb_probe(struct platform_device* odev)
 	dev_set_drvdata(&odev->dev, info);
 
 	rc = platinum_init_fb(info);
-	if (rc != 0) {
-		iounmap(pinfo->frame_buffer);
-		iounmap(pinfo->platinum_regs);
-		iounmap(pinfo->cmap_regs);
-		framebuffer_release(info);
-	}
+	if (rc != 0)
+		goto err_unmap_cmap;
+
+	return 0;
 
+err_unmap_cmap:
+	iounmap(pinfo->cmap_regs);
+err_release_cmap:
+	release_mem_region(pinfo->cmap_regs_phys, 0x1000);
+err_unmap_regs:
+	iounmap(pinfo->platinum_regs);
+err_unmap_fb:
+	iounmap(pinfo->frame_buffer);
+err_release_fb:
+	release_mem_region(pinfo->rsrc_fb.start, resource_size(&pinfo->rsrc_fb));
+	framebuffer_release(info);
 	return rc;
 }
 
-- 
2.43.0

Re: [PATCH v2] fbdev: platinumfb: add error checking for ioremap calls

[sashiko-bot]

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

Pre-existing issues:
- [High] Unchecked request_mem_region combined with unconditional release_mem_region causes resource corruption and Use-After-Free.
--

--- Patch [1]: [PATCH v2] fbdev: platinumfb: add error checking for ioremap calls ---
commit a9dd2a2a25aeae119ea17d0f40c6cbdddde67dfd
Author: yuebingkun <yuebingkun@kylinos.cn>

fbdev: platinumfb: add error checking for ioremap calls
    
This commit adds error checking for ioremap() and ioremap_wt() calls in
platinumfb_probe(), avoiding NULL pointer dereferences by introducing
goto-based cleanup paths on failure.

> diff --git a/drivers/video/fbdev/platinumfb.c b/drivers/video/fbdev/platinumfb.c
> index a08d955d9b439..f74c937c839f8 100644
> --- a/drivers/video/fbdev/platinumfb.c
> +++ b/drivers/video/fbdev/platinumfb.c
[ ... ]
> @@ -567,15 +567,30 @@ static int platinumfb_probe(struct platform_device* odev)
[ ... ]
>  	pinfo->cmap_regs_phys = 0xf301b000;	/* XXX not in prom? */
>  	request_mem_region(pinfo->cmap_regs_phys, 0x1000, "platinumfb cmap");
>  	pinfo->cmap_regs = ioremap(pinfo->cmap_regs_phys, 0x1000);
> +	if (!pinfo->cmap_regs) {
> +		dev_err(&odev->dev, "failed to ioremap cmap registers\n");
> +		rc = -ENOMEM;
> +		goto err_release_cmap;
> +	}
[ ... ]
> +err_unmap_cmap:
> +	iounmap(pinfo->cmap_regs);
> +err_release_cmap:
> +	release_mem_region(pinfo->cmap_regs_phys, 0x1000);

[Severity: High]
This is a pre-existing issue, but I noticed the return value of
request_mem_region() is ignored just before the ioremap() call.

If request_mem_region() fails because the physical resource is already
claimed by platform code or another driver, probe continues. If a subsequent
step then fails, such as ioremap() returning NULL or platinum_init_fb()
failing, this newly added error path branches to err_release_cmap.

Since err_release_cmap unconditionally calls release_mem_region(), does this
risk freeing a resource that wasn't actually acquired? If the region size
and start address exactly match an existing claim, this could unlink and
free a resource owned by another driver, potentially causing a
use-after-free later.

Could we add a check for the return value of request_mem_region() so we
only release it if the driver successfully claimed it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817063942.308926-1-yuebingkun@kylinos.cn?part=1

Re: [PATCH v2] fbdev: platinumfb: add error checking for ioremap calls

[Markus Elfring]

…
> Add proper error checking and use goto-based cleanup to avoid code
> duplication across the error paths.

* Please avoid duplicate source code also in the implementation of the function “platinumfb_probe”
  a bit more.
  https://elixir.bootlin.com/linux/v7.2-rc7/source/drivers/video/fbdev/platinumfb.c#L527-L634

* Under which circumstances will development interests grow for the application of scope-based resource management?


…
> ---
>  drivers/video/fbdev/platinumfb.c | 36 ++++++++++++++++++++++++++------
…

Some contributors would appreciate patch version descriptions.
https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n310

Regards,
Markus