[PATCH] drm/qxl: validate dst_offset in apply_reloc against BO size

[PATCH] drm/qxl: validate dst_offset in apply_reloc against BO size

[Berkant Koc]

The QXL DRM relocation handlers apply_reloc() and apply_surf_reloc() in
drivers/gpu/drm/qxl/qxl_ioctl.c perform user-controlled writes at
reloc_page + (info->dst_offset & ~PAGE_MASK), where dst_offset comes
verbatim from the drm_qxl_reloc ioctl argument. An aspirational comment
above apply_reloc explicitly states "dst must be validated, i.e. whole
bo on vram/surfacesram", but the validation has never been implemented
since the driver was merged in v3.10 (2013).

qxl_process_single_command() copies the user drm_qxl_reloc array into
qxl_reloc_info, narrowing dst_offset from __u64 to uint32_t. This still
gives the caller (any DRM_AUTH client) the full 32-bit / 4 GiB range and
no bounds check against dst_bo->tbo.base.size happens before the writes.

The kmap target is io_mapping_map_atomic_wc() over the QXL device's full
VRAM aperture (vram_mapping). The BUG_ON inside io-mapping.h only fires
when the offset exceeds the entire aperture, not the specific BO, so
writes within the aperture but outside the resolved BO succeed and land
on whatever data occupies those pages -- including other guest processes'
QXL BOs, queued QXL release-command BOs, and ring buffers that QEMU
consumes.

The kernel-side primitive is a guest-local OOB write (CWE-787) against
guest VRAM, not a host escape. Corrupting QXL command structures that
QEMU/SPICE then parses moves the attack surface to QEMU's QXL parser
(separate codebase, historically vulnerable), so this is a viable
priming primitive for guest-to-host chains whose terminal step lives in
userspace QEMU. The kernel bug stands as a defense-in-depth concern
independent of any QEMU chain.

Public discussion of this issue is at:
https://lore.kernel.org/virtualization/36acd33982bfdce04090e17294596ff8@berkoc.com/T/#u

Add an explicit bound check in the per-reloc fill loop in
qxl_process_single_command() so that both apply_reloc (8-byte write) and
apply_surf_reloc (4-byte write) refuse out-of-BO offsets and the
arithmetic overflow case (dst_offset close to U32_MAX) with -EINVAL.

Signed-off-by: Berkant Koc <me@berkoc.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 591b026..c65d468 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -231,6 +231,26 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 			reloc_info[i].dst_offset = reloc.dst_offset + release->release_offset;
 		}
 
+		/*
+		 * dst must be validated, i.e. whole bo on vram/surfacesram.
+		 * Refuse offsets that would write past the end of the bo, or
+		 * where dst_offset + write_size would overflow uint32_t.
+		 */
+		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)) {
+			ret = -EINVAL;
+			goto out_free_bos;
+		}
+		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)) {
+			ret = -EINVAL;
+			goto out_free_bos;
+		}
+
 		/* reserve and validate the reloc dst bo */
 		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
 			ret = qxlhw_handle_to_bo(file_priv, reloc.src_handle, release,
-- 
2.43.0

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

[Claude Code Review Bot]

Overall Series Review

Subject: drm/qxl: validate dst_offset in apply_reloc against BO size
Author: Berkant Koc <me@berkoc.com>
Patches: 1
Reviewed: 2026-05-16T10:59:08.741312

---

This is a single-patch security fix for a real out-of-bounds write vulnerability (CWE-787) in the QXL DRM driver that has existed since the driver was merged in v3.10 (2013). The commit message is thorough and well-written, clearly explaining the attack surface and scope. The fix itself is correct — it adds the missing bounds check that an existing aspirational comment explicitly called for but was never implemented.

The fix is the right approach (validate at the reloc processing site before the write), and the logic is sound. There are a few issues that should be addressed before merging, ranging from missing kernel metadata tags to a pre-existing cross-page write bug that should at minimum be noted.

**Verdict: Needs minor revision** — the core fix is correct but it needs a `Fixes:` tag, a `Cc: stable` tag, and ideally should also address the page-boundary crossing issue it partially inherits.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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