[PATCH] video/fbdev/via: check ioremap return value in viafb_lcd_get_mobile_state

[PATCH] video/fbdev/via: check ioremap return value in viafb_lcd_get_mobile_state

[Wang Jun]

The function viafb_lcd_get_mobile_state() calls ioremap() without
checking the return value. If ioremap() fails (returns NULL), the
subsequent readw() will cause a NULL pointer dereference.

This patch adds a proper NULL check after ioremap() and returns
-ENOMEM in case of failure.

Signed-off-by: Wang Jun <1742789905@qq.com>
---
 drivers/video/fbdev/via/lcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/video/fbdev/via/lcd.c b/drivers/video/fbdev/via/lcd.c
index 8673fced8749..91359d2b64fb 100644
--- a/drivers/video/fbdev/via/lcd.c
+++ b/drivers/video/fbdev/via/lcd.c
@@ -954,6 +954,10 @@ bool viafb_lcd_get_mobile_state(bool *mobile)
 	u16 start_pattern;
 
 	biosptr = ioremap(romaddr, 0x10000);
+	if (!biosptr) {
+		DEBUG_MSG(KERN_ERR " Failed to remap BIOS memory\n");
+		return false;
+	}
 	start_pattern = readw(biosptr);
 
 	/* Compare pattern */
-- 
2.43.0

Re: [PATCH] video/fbdev/via: check ioremap return value in viafb_lcd_get_mobile_state

[Helge Deller]

On 3/10/26 02:14, Wang Jun wrote:
> The function viafb_lcd_get_mobile_state() calls ioremap() without
> checking the return value. If ioremap() fails (returns NULL), the
> subsequent readw() will cause a NULL pointer dereference.

correct.

> This patch adds a proper NULL check after ioremap() 

yes.

> and returns
> -ENOMEM in case of failure.

You return "false", not -ENOMEM.

Anyway, I corrected this, dropped the DEBUG_MSG() call and applied it
to the fbdev git tree.

Thanks!
Helge

  
> Signed-off-by: Wang Jun <1742789905@qq.com>
> ---
>   drivers/video/fbdev/via/lcd.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/video/fbdev/via/lcd.c b/drivers/video/fbdev/via/lcd.c
> index 8673fced8749..91359d2b64fb 100644
> --- a/drivers/video/fbdev/via/lcd.c
> +++ b/drivers/video/fbdev/via/lcd.c
> @@ -954,6 +954,10 @@ bool viafb_lcd_get_mobile_state(bool *mobile)
>   	u16 start_pattern;
>   
>   	biosptr = ioremap(romaddr, 0x10000);
> +	if (!biosptr) {
> +		DEBUG_MSG(KERN_ERR " Failed to remap BIOS memory\n");
> +		return false;
> +	}
>   	start_pattern = readw(biosptr);
>   
>   	/* Compare pattern */

Claude review: video/fbdev/via: check ioremap return value in viafb_lcd_get_mobile_state

[Claude Code Review Bot]

Overall Series Review

Subject: video/fbdev/via: check ioremap return value in viafb_lcd_get_mobile_state
Author: Wang Jun <1742789905@qq.com>
Patches: 2
Reviewed: 2026-03-11T13:49:46.599498

---

This is a single patch adding a NULL check for `ioremap()` in the VIA fbdev LCD driver. The fix addresses a real bug — a missing NULL check that could lead to a NULL pointer dereference. The fix is straightforward and correct in principle, but has issues with the commit message and debug logging style.

---
Generated by Claude Code Patch Reviewer

Claude review: video/fbdev/via: check ioremap return value in viafb_lcd_get_mobile_state

[Claude Code Review Bot]

Patch Review

**The bug is real.** At `lcd.c:956`, `ioremap()` is called and the result is used immediately at line 957 via `readw(biosptr)` without any NULL check. If `ioremap()` fails, this would cause a NULL pointer dereference.

**Issues:**

1. **Commit message says `-ENOMEM` but the code returns `false`.** The commit message states:
   > This patch adds a proper NULL check after ioremap() and returns -ENOMEM in case of failure.
   
   But the actual code returns `false`, which is consistent with the function's `bool` return type. The commit message is wrong and should be corrected to say it returns `false`.

2. **DEBUG_MSG format string has a leading space:**
   ```c
   DEBUG_MSG(KERN_ERR " Failed to remap BIOS memory\n");
   ```
   The space after `KERN_ERR` before `"Failed"` looks like a typo. It should be:
   ```c
   DEBUG_MSG(KERN_ERR "Failed to remap BIOS memory\n");
   ```

3. **Use of `DEBUG_MSG` with `KERN_ERR`:** Looking at existing error paths in this function, neither of the existing `return false` paths (line 988) logs anything. Using `DEBUG_MSG` (which is typically compiled out) with `KERN_ERR` is a bit contradictory — if this is truly an error worth reporting, `pr_err()` would be more appropriate and wouldn't be compiled away. That said, using the existing codebase conventions is also reasonable; the author should clarify intent.

4. **Minor: the `*mobile` output parameter is left uninitialized on the error path.** This matches the existing behavior when the BIOS signature doesn't match (the `else` branch at line 986-988 also returns `false` without setting `*mobile`), so this is a pre-existing issue, not something this patch introduces.

**The fix itself is correct in substance** — the NULL check is placed in the right location, the return value is appropriate for the function signature, and the existing `iounmap()` calls in later paths don't need adjustment since we never successfully mapped in this error case.

---
Generated by Claude Code Patch Reviewer