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/qxl: validate dst_offset in apply_reloc against BO size Date: Sat, 16 May 2026 10:59:09 +1000 Message-ID: In-Reply-To: <1778753260.e30adb21c660@berkoc.com> References: <1778753260.e30adb21c660@berkoc.com> <1778753260.e30adb21c660@berkoc.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 **What the patch does:** Adds bounds checking in `qxl_process_single_comman= d()` to ensure that user-supplied `dst_offset` values don't exceed the dest= ination BO's size before `apply_reloc()` (8-byte write) or `apply_surf_relo= c()` (4-byte write) use them. **Correctness =E2=80=94 the core check is right:** The two checks correctly use the actual write size for each reloc type: ```c if (reloc_info[i].type =3D=3D QXL_RELOC_TYPE_BO && (reloc_info[i].dst_offset > U32_MAX - sizeof(uint64_t) || reloc_info[i].dst_offset + sizeof(uint64_t) > reloc_info[i].dst_bo->tbo.base.size)) { ``` and ```c if (reloc_info[i].type =3D=3D QXL_RELOC_TYPE_SURF && (reloc_info[i].dst_offset > U32_MAX - sizeof(uint32_t) || reloc_info[i].dst_offset + sizeof(uint32_t) > reloc_info[i].dst_bo->tbo.base.size)) { ``` The overflow guard (`dst_offset > U32_MAX - sizeof(...)`) is defense-in-dep= th: on 64-bit systems the `+ sizeof()` promotes `dst_offset` to `size_t` so= the arithmetic can't actually wrap, but on 32-bit systems it could. Keepin= g it is good practice for security-critical code. The placement after BO handle resolution (line 223=E2=80=93232) but before = the write path is correct. The error path `goto out_free_bos` is safe since= resolved BOs are tracked via `qxl_release_list_add` and will be cleaned up= by `qxl_release_free`. **Issues:** 1. **Missing `Fixes:` tag.** This is a security fix for a bug present since= the driver was introduced. It should include: ``` Fixes: f64122c1f6 ("drm: add new QXL driver. (v1.4)") ``` (or whatever the actual SHA is for the original QXL merge). This tag is = needed for stable backport automation and for tooling that tracks vulnerabi= lity lifecycles. 2. **Missing `Cc: stable@vger.kernel.org`.** A security fix of this nature = should be backported to stable kernels. Add: ``` Cc: stable@vger.kernel.org ``` 3. **Pre-existing page-boundary crossing bug not addressed.** `apply_reloc(= )` maps a single page and writes 8 bytes at `reloc_page + (info->dst_offset= & ~PAGE_MASK)`. If `dst_offset & ~PAGE_MASK` is in the range `[PAGE_SIZE -= 7, PAGE_SIZE - 1]`, the 8-byte write spans two pages but only one is mappe= d. The new bounds check ensures the write is within the *BO*, but not withi= n a *single mapped page*. For example, with a 8192-byte BO and `dst_offset = =3D 0xFF9` (4089), the write at bytes 4089=E2=80=934096 crosses the page bo= undary. This is a pre-existing issue, not introduced by this patch, but sin= ce this patch is explicitly adding safety validation for these writes, it w= ould be natural to also add: ```c if ((reloc_info[i].dst_offset & ~PAGE_MASK) + write_size > PAGE_SIZE) { ret =3D -EINVAL; goto out_free_bos; } ``` Or at minimum note it as a known limitation / follow-up. 4. **`__u64` to `uint32_t` truncation of `reloc.dst_offset` is not guarded.= ** The UAPI struct `drm_qxl_reloc.dst_offset` is `__u64` (line 75 of `qxl_d= rm.h`), but `qxl_reloc_info.dst_offset` is `uint32_t` (line 76 of `qxl_ioct= l.c`). The assignment at line 228: ```c reloc_info[i].dst_offset =3D reloc.dst_offset; ``` silently truncates the upper 32 bits. The commit message acknowledges th= is truncation but doesn't fix it. The bounds check validates the *truncated= * value, which is correct for the actual write that happens, so this isn't = a security issue. However, best practice for a security hardening patch wou= ld be to explicitly reject values above `U32_MAX`: ```c if (reloc.dst_offset > U32_MAX) { ret =3D -EINVAL; goto out_free_bos; } ``` This makes the truncation intentional rather than implicit, and prevents= userspace from being silently confused about where its write actually land= s. 5. **The two `if` blocks could be consolidated.** Minor style point =E2=80= =94 the logic is duplicated except for `sizeof(uint64_t)` vs `sizeof(uint32= _t)`. A single check with a conditional size would be slightly cleaner: ```c size_t write_size =3D (reloc_info[i].type =3D=3D QXL_RELOC_TYPE_BO) ? sizeof(uint64_t) : sizeof(uint32_t); if (reloc_info[i].dst_offset > U32_MAX - write_size || reloc_info[i].dst_offset + write_size > reloc_info[i].dst_bo->tbo.base.size) { ret =3D -EINVAL; goto out_free_bos; } ``` This is not a correctness issue, just a suggestion to reduce duplication. 6. **Suspicious `X-Mailer` header.** The email has `X-Mailer: git-send-emai= l/skynet 1.0` =E2=80=94 this is not a standard git-send-email version strin= g. Not a code issue, but unusual for a security patch submission. **Summary:** The fix is correct and addresses a real, longstanding vulnerab= ility. It needs `Fixes:` and `Cc: stable` tags before merging, and would be= nefit from guarding the `__u64` =E2=86=92 `uint32_t` truncation explicitly = and addressing the page-boundary crossing case. The core bounds-check logic= is sound. --- Generated by Claude Code Patch Reviewer