From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/gem: Fix inconsistent plane dimension calculation in drm_gem_fb_init_with_funcs() Date: Sun, 12 Apr 2026 10:54:06 +1000 Message-ID: In-Reply-To: <20260409164156.2235189-1-ashutoshdesai993@gmail.com> References: <20260409164156.2235189-1-ashutoshdesai993@gmail.com> <20260409164156.2235189-1-ashutoshdesai993@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 **Code correctness: Good.** The change from: ```c unsigned int width =3D mode_cmd->width / (i ? info->hsub : 1); unsigned int height =3D mode_cmd->height / (i ? info->vsub : 1); ``` to: ```c unsigned int width =3D drm_format_info_plane_width(info, mode_cmd->width, i= ); unsigned int height =3D drm_format_info_plane_height(info, mode_cmd->height= , i); ``` is correct. Looking at the helper definitions in `include/drm/drm_fourcc.h:= 272-305`, for `plane =3D=3D 0` they return the original value (matching the= old `(i ? ... : 1)` logic), and for `plane > 0` they use `DIV_ROUND_UP()` = instead of truncating division. This matches what `framebuffer_check()` doe= s at `drm_framebuffer.c:172-173`. **Bug analysis is accurate.** The concrete scenario described =E2=80=94 NV1= 2 (vsub=3D2), height=3D1 =E2=80=94 is a valid reproduction case: - `framebuffer_check()` computes chroma height =3D `DIV_ROUND_UP(1, 2) =3D = 1`, passes validation - Old `drm_gem_fb_init_with_funcs()` computes chroma height =3D `1 / 2 =3D = 0` - `min_size =3D (0 - 1) * pitches[i] + ...` wraps `(unsigned int)(0 - 1)` = =3D `0xFFFFFFFF` - The multiply + add overflows `unsigned int`, potentially yielding a small= `min_size` - A too-small GEM object passes the size guard **Issues to address:** 1. **Missing `Fixes:` tag.** This is a bug fix and should include a `Fixes:= ` tag referencing the commit that introduced this code (likely the original= `drm_gem_fb_init_with_funcs()` or a refactor that introduced the open-code= d division). This is important for backporting decisions. 2. **Missing `Cc: stable@vger.kernel.org`.** Given this is a potential out-= of-bounds access triggered from userspace (via the `DRM_IOCTL_MODE_ADDFB2` = path), this should be tagged for stable backports. 3. **Minor type mismatch.** `drm_format_info_plane_width()` and `drm_format= _info_plane_height()` return `int`, but the local variables are `unsigned i= nt`. This is a pre-existing pattern (the same implicit conversion happens a= t `drm_framebuffer.c:172-173`), so it's not introduced by this patch =E2=80= =94 but worth noting. The values will always be positive in practice since = `framebuffer_check()` rejects `width=3D=3D0` and `height=3D=3D0` before thi= s code runs. 4. **Commit message quality.** The commit message is well written and clear= ly explains the root cause, the concrete failure scenario, and the fix. No = issues here. **Summary:** The code change is correct, minimal, and addresses a real bug = that could allow a malicious userspace to bypass the GEM object size check = for sub-sampled formats. It should be resubmitted with `Fixes:` and `Cc: st= able` tags added. --- Generated by Claude Code Patch Reviewer