From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: fbdev:modedb: fix a possible UAF in fb_find_mode()
Date: Wed, 27 May 2026 15:06:25 +1000
Message-ID: <review-patch1-20260526091507.421730-1-islituo@gmail.com>
In-Reply-To: <20260526091507.421730-1-islituo@gmail.com>
References: <20260526091507.421730-1-islituo@gmail.com> <20260526091507.421730-1-islituo@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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 =3D mo=
de_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 !=3D -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 execu=
tion reaches the second best-fit loop and finds a match, `mode_option_buf` =
is leaked.

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

- **Line 844** (`return 3`): Default mode path =E2=80=94 after the `if (mod=
e_option)` block closes at line 840, the function tries the default mode. I=
f mode_option was NULL (so `mode_option_buf` was allocated), but the parsin=
g inside `if (mode_option)` still ran, this path is actually unreachable si=
nce `mode_option` would be non-NULL if `mode_option_buf` was allocated. How=
ever, if `fb_get_options` sets `*option =3D NULL` (which it does when `opti=
ons` is NULL), then `mode_option_buf` stays NULL and `mode_option` stays NU=
LL, 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 whe=
re `best !=3D -1` after the first database loop. The patch places `kfree` j=
ust 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` state=
ment 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 return=
s through it. This would be cleaner and less bug-prone:

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

However, this function has returns both inside and outside the `if (mode_op=
tion)` 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: mov=
e 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 simp=
ler: 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 memor=
y leak on the `return 5` path, so it trades one bug for another (albeit a l=
ess severe one =E2=80=94 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