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: omap2: fix use-after-free in omapfb_mmap
Date: Thu, 04 Jun 2026 13:04:18 +1000
Message-ID: <review-patch1-20260602085421.194325-1-zenghongling@kylinos.cn>
In-Reply-To: <20260602085421.194325-1-zenghongling@kylinos.cn>
References: <20260602085421.194325-1-zenghongling@kylinos.cn> <20260602085421.194325-1-zenghongling@kylinos.cn>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Race fix on `start` (correct):**

The replacement of `omapfb_get_region_paddr(ofbi)` with inline reads from `=
rg` is correct. The original helper reads from `ofbi->region->...` which ca=
n be concurrently reassigned, while the patched code reads from `rg`, which=
 was captured under the region lock:

```c
+	if (ofbi->rotation_type =3D=3D OMAP_DSS_ROT_VRFB)
+		start =3D rg->vrfb.paddr[0];
+	else
+		start =3D rg->paddr;
```

This correctly ensures the physical address comes from the same region whos=
e `map_count` will be incremented.

**Critical bug: Double `map_count` increment on success path.**

The patched code (lines 1116 and 1123 of the result file) increments `map_c=
ount` **twice** on the success path:

```c
	atomic_inc(&rg->map_count);          // line 1116: NEW increment

	r =3D vm_iomap_memory(vma, start, len);
	if (r)
		goto error;

	/* vm_ops.open won't be called for mmap itself. */
	atomic_inc(&rg->map_count);          // line 1123: ORIGINAL increment (kep=
t!)
```

But `mmap_user_close()` only decrements once:

```c
static void mmap_user_close(struct vm_area_struct *vma)
{
	struct omapfb2_mem_region *rg =3D vma->vm_private_data;
	omapfb_get_mem_region(rg);
	atomic_dec(&rg->map_count);
	omapfb_put_mem_region(rg);
}
```

This means every successful mmap leaks +1 on `map_count`. The region will n=
ever reach map_count =3D=3D 0, so it will **never be freed**. This is a mem=
ory leak that replaces the use-after-free with a resource leak.

The fix should **move** the increment (removing the original), not **add** =
a second one. The original `atomic_inc` after `vm_iomap_memory` should be d=
eleted.

**Race on `len` not addressed:**

The commit message states: *"The fix is to read all required values (start,=
 len) from the same region reference (rg)"* =E2=80=94 but the code still re=
ads:

```c
	len =3D fix->smem_len;
```

`fix->smem_len` is updated by `omapfb_setup_overlay()` at line 620 (`fix->s=
mem_len =3D rg->size`), which runs under `lock` but not `mm_lock`. The v2 c=
hangelog explains this is intentional for VRFB sparse mappings, where `fix-=
>smem_len` encompasses the VRFB address space and differs from `rg->size`. =
This is an acceptable compromise, but the commit message should be updated =
to not claim `len` is read from `rg`, since it isn't.

**Error path (correct):**

The new error path correctly undoes the pre-mapping increment:

```c
error:
+	atomic_dec(&rg->map_count);
	omapfb_put_mem_region(rg);
```

This is balanced =E2=80=94 if `vm_iomap_memory` fails, the increment is rev=
erted before returning. (This is only correct once the double-increment on =
the success path is fixed.)

**Summary of required changes:**

1. Remove the **original** `atomic_inc(&rg->map_count)` after `vm_iomap_mem=
ory()` (currently at line 1123), keeping only the new one before it. The co=
mment `/* vm_ops.open won't be called for mmap itself. */` should be moved =
to above the pre-mapping increment to preserve the documentation of why it'=
s needed.
2. Update the commit message to accurately state that `len` is still read f=
rom `fix->smem_len` (intentionally, for VRFB compatibility).

---
Generated by Claude Code Patch Reviewer