Claude review: drm/qxl: validate dst_offset in apply_reloc against BO size

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**What the patch does:** Adds bounds checking in `qxl_process_single_command()` to ensure that user-supplied `dst_offset` values don't exceed the destination BO's size before `apply_reloc()` (8-byte write) or `apply_surf_reloc()` (4-byte write) use them.

**Correctness — the core check is right:**

The two checks correctly use the actual write size for each reloc type:
```c
if (reloc_info[i].type == 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 == 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-depth: 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. Keeping it is good practice for security-critical code.

The placement after BO handle resolution (line 223–232) 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 vulnerability 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 mapped. The new bounds check ensures the write is within the *BO*, but not within a *single mapped page*. For example, with a 8192-byte BO and `dst_offset = 0xFF9` (4089), the write at bytes 4089–4096 crosses the page boundary. This is a pre-existing issue, not introduced by this patch, but since this patch is explicitly adding safety validation for these writes, it would be natural to also add:
   ```c
   if ((reloc_info[i].dst_offset & ~PAGE_MASK) + write_size > PAGE_SIZE) {
       ret = -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_drm.h`), but `qxl_reloc_info.dst_offset` is `uint32_t` (line 76 of `qxl_ioctl.c`). The assignment at line 228:
   ```c
   reloc_info[i].dst_offset = reloc.dst_offset;
   ```
   silently truncates the upper 32 bits. The commit message acknowledges this 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 would be to explicitly reject values above `U32_MAX`:
   ```c
   if (reloc.dst_offset > U32_MAX) {
       ret = -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 lands.

5. **The two `if` blocks could be consolidated.** Minor style point — 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 = (reloc_info[i].type == 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 = -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-email/skynet 1.0` — this is not a standard git-send-email version string. Not a code issue, but unusual for a security patch submission.

**Summary:** The fix is correct and addresses a real, longstanding vulnerability. It needs `Fixes:` and `Cc: stable` tags before merging, and would benefit from guarding the `__u64` → `uint32_t` truncation explicitly and addressing the page-boundary crossing case. The core bounds-check logic is sound.

---
Generated by Claude Code Patch Reviewer