From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: fbdev: omap2: fix use-after-free in omapfb_mmap Date: Thu, 04 Jun 2026 13:05:53 +1000 Message-ID: In-Reply-To: <20260602080415.171378-1-zenghongling@kylinos.cn> References: <20260602080415.171378-1-zenghongling@kylinos.cn> <20260602080415.171378-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 **Correctness: Good.** The patch correctly inlines the logic of `omapfb_get= _region_paddr()` using the already-locked `rg` pointer instead of going thr= ough `ofbi`: ```c - start =3D omapfb_get_region_paddr(ofbi); - len =3D fix->smem_len; + if (ofbi->rotation_type =3D=3D OMAP_DSS_ROT_VRFB) + start =3D rg->vrfb.paddr[0]; + else + start =3D rg->paddr; + len =3D 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 =3D=3D 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 acc= ess, and replaces `fix->smem_len` (which is also updated by `set_fb_fix()` = during the ioctl) with `rg->size` =E2=80=94 both now derived from the locke= d region. **Note on `ofbi->rotation_type`:** The condition still reads `ofbi->rotatio= n_type` without holding a lock that protects it. However, `rotation_type` i= s 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->sme= m_len` which gets updated by `set_fb_fix()` inside `omapfb_setup_plane()` t= o reflect the new region. Switching to `rg->size` is correct =E2=80=94 it r= eads the size of the region we actually locked and will map-count. **Commit message quality:** Excellent. The step-by-step race scenario is cl= early laid out and accurate. The Cc: stable tag is appropriate for a use-af= ter-free fix. **Minor style observation:** The inlined code duplicates the body of `omapf= b_get_region_paddr()`. An alternative would be to refactor that helper to t= ake a `struct omapfb2_mem_region *` parameter instead of `struct omapfb_inf= o *`. However, this is a legacy fbdev driver unlikely to see further develo= pment, 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