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

[Claude Code Review Bot <claude-review@example.com>]

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