[PATCH] fbdev: omap2: fix use-after-free in omapfb_mmap

[PATCH] fbdev: omap2: fix use-after-free in omapfb_mmap

[Hongling Zeng]

omapfb_mmap() has a race condition with OMAPFB_SETUP_PLANE ioctl that
can lead to use-after-free:

The fb_mmap() entry point holds mm_lock but not lock (fb_info->lock),
while ioctl handlers like OMAPFB_SETUP_PLANE hold lock but not mm_lock.
This allows concurrent execution.

In omapfb_mmap():
1. rg = omapfb_get_mem_region(ofbi->region);      // Get old region ref
2. start = omapfb_get_region_paddr(ofbi);          // Read from NEW region
3. len = fix->smem_len;                             // Read from NEW region
4. vm_iomap_memory(vma, start, len);               // Map NEW region memory
5. atomic_inc(&rg->map_count);                      // Increment OLD region!

Concurrently, OMAPFB_SETUP_PLANE can:
- Reassign ofbi->region = new_rg
- Update fix->smem_len
- OMAPFB_SETUP_MEM then checks NEW region's map_count (0!) and frees it

This leaves userspace with a mapping to freed physical memory.

The fix is to read all required values (start, len) from the same
region reference (rg) that will have its map_count incremented,
preventing the region from being freed while still mapped.

Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index d70deb6a9150..853bd55621ec 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
@@ -1099,8 +1099,11 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 
 	rg = omapfb_get_mem_region(ofbi->region);
 
-	start = omapfb_get_region_paddr(ofbi);
-	len = fix->smem_len;
+	if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
+		start = rg->vrfb.paddr[0];
+	else
+		start = rg->paddr;
+	len = rg->size;
 
 	DBG("user mmap region start %lx, len %d, off %lx\n", start, len,
 			vma->vm_pgoff << PAGE_SHIFT);
-- 
2.25.1

Claude review: fbdev: omap2: fix use-after-free in omapfb_mmap

[Claude Code Review Bot]

Overall Series Review

Subject: fbdev: omap2: fix use-after-free in omapfb_mmap
Author: Hongling Zeng <zenghongling@kylinos.cn>
Patches: 1
Reviewed: 2026-06-04T13:04:18.710868

---

This is a single-patch series fixing a race condition between `omapfb_mmap()` and `OMAPFB_SETUP_PLANE` ioctl in the omap2 fbdev driver. The race analysis in the commit message is sound — `ofbi->region` can be reassigned concurrently, causing `omapfb_get_region_paddr()` to read from the new region while `map_count` is incremented on the old one, enabling a use-after-free.

However, the patch has a **critical bug**: it adds a new `atomic_inc(&rg->map_count)` before `vm_iomap_memory()` without removing the original increment after it. This double-increments `map_count` on the success path, leaking a reference and preventing the region from ever being freed.

Additionally, the commit message claims to fix the race on both `start` and `len`, but `len` is still read from the raceable `fix->smem_len`.

**Recommendation: Needs revision.** The double-increment must be fixed before this can be applied.

---

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: omap2: fix use-after-free in omapfb_mmap

[Claude Code Review Bot]

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 can be concurrently reassigned, while the patched code reads from `rg`, which was captured under the region lock:

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

This correctly ensures the physical address comes from the same region whose `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_count` **twice** on the success path:

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

	r = 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 (kept!)
```

But `mmap_user_close()` only decrements once:

```c
static void mmap_user_close(struct vm_area_struct *vma)
{
	struct omapfb2_mem_region *rg = 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 never reach map_count == 0, so it will **never be freed**. This is a memory 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 deleted.

**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)"* — but the code still reads:

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

`fix->smem_len` is updated by `omapfb_setup_overlay()` at line 620 (`fix->smem_len = rg->size`), which runs under `lock` but not `mm_lock`. The v2 changelog 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 — if `vm_iomap_memory` fails, the increment is reverted 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_memory()` (currently at line 1123), keeping only the new one before it. The comment `/* 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 from `fix->smem_len` (intentionally, for VRFB compatibility).

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: omap2: fix use-after-free in omapfb_mmap

[Claude Code Review Bot]

Overall Series Review

Subject: fbdev: omap2: fix use-after-free in omapfb_mmap
Author: Hongling Zeng <zenghongling@kylinos.cn>
Patches: 1
Reviewed: 2026-06-04T13:05:53.596691

---

This is a single patch fixing a real use-after-free race condition in the omapfb driver's `mmap` path. The analysis in the commit message is accurate and well-explained: `omapfb_mmap()` takes a reference to `rg` via `omapfb_get_mem_region(ofbi->region)`, but then calls `omapfb_get_region_paddr(ofbi)` which re-dereferences `ofbi->region` — a pointer that can be changed concurrently by `OMAPFB_SETUP_PLANE`. This means `start` and `len` can come from a different region than the one whose `map_count` gets incremented, allowing the actual mapped region to be freed.

The fix is correct in principle: it reads `start` and `len` directly from `rg` (the locked region) rather than going through helpers that re-read `ofbi->region`. This ensures all values are consistent with the region being reference-counted.

**Verdict: The patch is correct and should be accepted**, with one minor observation below.

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: omap2: fix use-after-free in omapfb_mmap

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** The patch correctly inlines the logic of `omapfb_get_region_paddr()` using the already-locked `rg` pointer instead of going through `ofbi`:

```c
-	start = omapfb_get_region_paddr(ofbi);
-	len = fix->smem_len;
+	if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
+		start = rg->vrfb.paddr[0];
+	else
+		start = rg->paddr;
+	len = rg->size;
```

The original `omapfb_get_region_paddr()` does:
```c
static u32 omapfb_get_region_paddr(const struct omapfb_info *ofbi)
{
	if (ofbi->rotation_type == OMAP_DSS_ROT_VRFB)
		return ofbi->region->vrfb.paddr[0];
	else
		return ofbi->region->paddr;
}
```

The patch correctly replaces `ofbi->region->` with `rg->` for the paddr access, and replaces `fix->smem_len` (which is also updated by `set_fb_fix()` during the ioctl) with `rg->size` — both now derived from the locked region.

**Note on `ofbi->rotation_type`:** The condition still reads `ofbi->rotation_type` without holding a lock that protects it. However, `rotation_type` is set once during probe and is not changed by `OMAPFB_SETUP_PLANE`, so this is safe. It's a property of the fb_info, not of the region.

**Note on `fix->smem_len` vs `rg->size`:** The original code read `fix->smem_len` which gets updated by `set_fb_fix()` inside `omapfb_setup_plane()` to reflect the new region. Switching to `rg->size` is correct — it reads the size of the region we actually locked and will map-count.

**Commit message quality:** Excellent. The step-by-step race scenario is clearly laid out and accurate. The Cc: stable tag is appropriate for a use-after-free fix.

**Minor style observation:** The inlined code duplicates the body of `omapfb_get_region_paddr()`. An alternative would be to refactor that helper to take a `struct omapfb2_mem_region *` parameter instead of `struct omapfb_info *`. However, this is a legacy fbdev driver unlikely to see further development, so the pragmatic inline approach is fine and arguably clearer for a -stable backport.

**Overall: Reviewed-by worthy.** The fix is minimal, correct, and addresses a real race condition.

---
Generated by Claude Code Patch Reviewer