[PATCH] fbdev:modedb: fix a possible UAF in fb_find_mode()

[PATCH] fbdev:modedb: fix a possible UAF in fb_find_mode()

[Tuo Li]

If mode_option is NULL, it is assigned from mode_option_buf:

  if (!mode_option) {
    fb_get_options(NULL, &mode_option_buf);
    mode_option = mode_option_buf;
  }

Later, name is assigned from mode_option:

  const char *name = mode_option;

However, mode_option_buf is freed before name is no longer used:

  kfree(mode_option_buf);

while name is still accessed by:

  if ((name_matches(db[i], name, namelen) ||

Since name aliases mode_option_buf, this may result in a
use-after-free.

Fix this by moving the kfree(mode_option_buf) call behind the access, and
add corresponding cleanup before early returns.

Signed-off-by: Tuo Li <islituo@gmail.com>
---
 drivers/video/fbdev/core/modedb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
index 703d0b7aec32..91f6c78f9e19 100644
--- a/drivers/video/fbdev/core/modedb.c
+++ b/drivers/video/fbdev/core/modedb.c
@@ -724,7 +724,6 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 			res_specified = 1;
 		}
 done:
-		kfree(mode_option_buf);
 		if (cvt) {
 			struct fb_videomode cvt_mode;
 			int ret;
@@ -749,6 +748,7 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 
 			if (!ret && !fb_try_mode(var, info, &cvt_mode, bpp)) {
 				DPRINTK("modedb CVT: CVT mode ok\n");
+				kfree(mode_option_buf);
 				return 1;
 			}
 
@@ -793,8 +793,10 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 				if (!interlace_specified ||
 				    db_interlace == interlace)
 					if (refresh_specified &&
-					    db[i].refresh == refresh)
+					    db[i].refresh == refresh) {
+						kfree(mode_option_buf);
 						return 1;
+					}
 
 				if (score < diff) {
 					diff = score;
@@ -802,6 +804,8 @@ int fb_find_mode(struct fb_var_screeninfo *var,
 				}
 			}
 		}
+
+		kfree(mode_option_buf);
 		if (best != -1) {
 			fb_try_mode(var, info, &db[best], bpp);
 			return (refresh_specified) ? 2 : 1;
-- 
2.43.0

Claude review: fbdev:modedb: fix a possible UAF in fb_find_mode()

[Claude Code Review Bot]

Overall Series Review

Subject: fbdev:modedb: fix a possible UAF in fb_find_mode()
Author: Tuo Li <islituo@gmail.com>
Patches: 1
Reviewed: 2026-05-27T15:06:24.893069

---

This is a single patch claiming to fix a use-after-free (UAF) in `fb_find_mode()`. The claimed bug is that `mode_option_buf` (a `kstrdup`'d buffer from `fb_get_options`) is freed while the pointer `name`, which aliases it, is still in use.

**The bug claim is valid in the original code.** In the original code, `kfree(mode_option_buf)` was placed at the `done:` label, immediately before the CVT path and the database-search loop. The `name` pointer (which may alias `mode_option_buf`) is then used in the `name_matches()` call at line 783. This is a genuine UAF when `mode_option` was originally NULL and got populated from `fb_get_options`.

**However, the fix is incomplete and introduces new bugs.** It misses several return paths that still leak `mode_option_buf`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev:modedb: fix a possible UAF in fb_find_mode()

[Claude Code Review Bot]

Patch Review

**The bug diagnosis is correct.** The original `kfree(mode_option_buf)` at `done:` (line 726 pre-patch) frees the buffer before `name` is used in the loop at line 783 (`name_matches(db[i], name, namelen)`). Since `name = mode_option` and `mode_option` may point to `mode_option_buf`, this is a real UAF.

**Issue 1: Missing kfree on multiple remaining return paths (memory leak)**

The patch adds `kfree(mode_option_buf)` before three return points:
1. The CVT success return at line 751
2. The exact-refresh-match return at line 797
3. Before the `best != -1` check at line 808

But it **misses** these return paths that still leak:

- **Line 838** (`return 5`): The "best-fit modes" fallback return. If execution reaches the second best-fit loop and finds a match, `mode_option_buf` is leaked.

```c
		if (best != -1) {
			fb_try_mode(var, info, &db[best], bpp);
			return 5;  // <-- mode_option_buf leaked here
		}
```

- **Line 844** (`return 3`): Default mode path — after the `if (mode_option)` block closes at line 840, the function tries the default mode. If mode_option was NULL (so `mode_option_buf` was allocated), but the parsing inside `if (mode_option)` still ran, this path is actually unreachable since `mode_option` would be non-NULL if `mode_option_buf` was allocated. However, if `fb_get_options` sets `*option = NULL` (which it does when `options` is NULL), then `mode_option_buf` stays NULL and `mode_option` stays NULL, so the `if (mode_option)` block is skipped entirely. In that case `mode_option_buf` is NULL and `kfree(NULL)` is a no-op, so this is fine.

- **Line 811** (`return (refresh_specified) ? 2 : 1`): This is the path where `best != -1` after the first database loop. The patch places `kfree` just before this check at line 808, so this path **is** covered. Good.

So the substantive miss is the `return 5` at line 838.

**Issue 2: Fragile and error-prone approach**

The fundamental approach of sprinkling `kfree()` before each `return` statement is fragile and error-prone. The standard kernel pattern for this is to use a single cleanup label (`goto out` / `goto free`) and route all returns through it. This would be cleaner and less bug-prone:

```c
	int ret = 0;
	...
	/* at each early return, instead of return N: */
	ret = 1;
	goto out;
	...
out:
	kfree(mode_option_buf);
	return ret;
```

However, this function has returns both inside and outside the `if (mode_option)` block, and the returns outside that block don't need the free (since `mode_option_buf` can only be non-NULL inside). A simpler alternative: move the `kfree` to just before line 840 (closing brace of `if (mode_option)`) and also keep it before the early returns within that block. But even simpler: just defer the free to the end of the function since `kfree(NULL)` is safe, and route all returns through a cleanup label.

**Issue 3: Commit message inaccuracy**

The commit message says "fix a possible UAF" but the fix introduces a memory leak on the `return 5` path, so it trades one bug for another (albeit a less severe one — leak vs. UAF).

**Summary:** The bug identification is correct, but the fix is incomplete. Recommend NAK and request the author use a `goto`-based cleanup pattern to catch all return paths.

---
Generated by Claude Code Patch Reviewer