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

[PATCH] 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 cleanup for all three ioremap calls in
the probe function, ensuring that any resources already allocated are
properly released on failure.

Signed-off-by: yuebingkun <yuebingkun@kylinos.cn>
---
 drivers/video/fbdev/platinumfb.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/video/fbdev/platinumfb.c b/drivers/video/fbdev/platinumfb.c
index fa27a3a4f05b..e607d03a8adb 100644
--- a/drivers/video/fbdev/platinumfb.c
+++ b/drivers/video/fbdev/platinumfb.c
@@ -567,15 +567,40 @@ 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");
+		release_mem_region(pinfo->rsrc_fb.start,
+				   resource_size(&pinfo->rsrc_fb));
+		framebuffer_release(info);
+		return -ENOMEM;
+	}
 	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");
+		iounmap(pinfo->frame_buffer);
+		release_mem_region(pinfo->rsrc_fb.start,
+				   resource_size(&pinfo->rsrc_fb));
+		framebuffer_release(info);
+		return -ENOMEM;
+	}
 
 	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");
+		iounmap(pinfo->platinum_regs);
+		iounmap(pinfo->frame_buffer);
+		release_mem_region(pinfo->cmap_regs_phys, 0x1000);
+		release_mem_region(pinfo->rsrc_fb.start,
+				   resource_size(&pinfo->rsrc_fb));
+		framebuffer_release(info);
+		return -ENOMEM;
+	}
 
 	/* Grok total video ram */
 	out_be32(&pinfo->platinum_regs->reg[16].r, (unsigned)pinfo->frame_buffer_phys);
-- 
2.43.0

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

[Markus Elfring]

…
> Add proper error checking and cleanup for all three ioremap calls in
> the probe function, ensuring that any resources already allocated are
> properly released on failure.

* Please avoid duplicate source code also in the implementation of the function “platinumfb_probe”.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.2-rc7#n526

* How do you think about to increase the application of scope-based resource management?

* Would you like to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n145
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2-rc7#n34


Regards,
Markus

[PATCH] 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] 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] The return value of `request_mem_region` for `cmap_regs_phys` is ignored, and the error path unconditionally calls `release_mem_region` on it.
--

--- Patch [1]: [PATCH] fbdev: platinumfb: add error checking for ioremap calls ---
commit 71a977c44688a9c49d5d28cb513b56fa5a810a1d
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() to prevent NULL pointer dereferences. It also transitions
the function to use goto-based cleanup for handling initialization failures.

> 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

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but by adding this new error path, the code
propagates an unsafe assumption.

Earlier in platinumfb_probe(), the return value of request_mem_region()
is ignored:

	request_mem_region(pinfo->cmap_regs_phys, 0x1000, "platinumfb cmap");
	pinfo->cmap_regs = ioremap(pinfo->cmap_regs_phys, 0x1000);

If request_mem_region() fails (for instance, if the region is already
claimed), execution continues. If a subsequent step like ioremap() or
platinum_init_fb() then fails, the code will jump to this newly added
err_release_cmap label and unconditionally release the region.

Because __release_region() searches by address and doesn't verify the caller,
could this forcefully free an I/O memory region that actually belongs to
another driver, potentially leading to resource tree corruption or a
use-after-free?

> +err_unmap_regs:
> +	iounmap(pinfo->platinum_regs);
> +err_unmap_fb:
> +	iounmap(pinfo->frame_buffer);

[ ... ]

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