[PATCH v2] accel/qaic: Add overflow check to remap_pfn_range during mmap

[PATCH v2] accel/qaic: Add overflow check to remap_pfn_range during mmap

[Zack McKevitt]

The call to remap_pfn_range in qaic_gem_object_mmap is susceptible to
(re)mapping beyond the VMA if the BO is too large. This can cause use
after free issues when munmap() unmaps only the VMA region and not the
additional mappings. To prevent this, check the remaining size of the
VMA before remapping and truncate the remapped length if sg->length is
too large.

Reported-by: Lukas Maar <lukas.maar@tugraz.at>
Fixes: ff13be830333 ("accel/qaic: Add datapath")
Reviewed-by: Karol Wachowski <karol.wachowski@linux.intel.com>
Signed-off-by: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
---
Changes in v2:
- Use check_sub_overflow to check if vma->vm_end - remap_start goes negative.
- Check if remap_end is strictly greater than vma->vm_end (rather than greater
  than or equal to) when deciding to truncate length.
- Link to v1: https://lore.kernel.org/all/20260423204412.2861046-1-zachary.mckevitt@oss.qualcomm.com/

 drivers/accel/qaic/qaic_data.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index 95300c2f7d8a..642b6ae9edfa 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -606,8 +606,11 @@ static const struct vm_operations_struct drm_vm_ops = {
 static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 {
 	struct qaic_bo *bo = to_qaic_bo(obj);
+	unsigned long remap_start;
 	unsigned long offset = 0;
+	unsigned long remap_end;
 	struct scatterlist *sg;
+	unsigned long length;
 	int ret = 0;
 
 	if (drm_gem_is_imported(obj))
@@ -615,11 +618,26 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
 
 	for (sg = bo->sgt->sgl; sg; sg = sg_next(sg)) {
 		if (sg_page(sg)) {
+			/* if sg is too large for the VMA, so truncate it to fit */
+			if (check_add_overflow(vma->vm_start, offset, &remap_start))
+				return -EINVAL;
+			if (check_add_overflow(remap_start, sg->length, &remap_end))
+				return -EINVAL;
+
+			if (remap_end > vma->vm_end) {
+				if (check_sub_overflow(vma->vm_end, remap_start, &length))
+					return -EINVAL;
+			} else
+				length = sg->length;
+
+			if (length == 0)
+				goto out;
+
 			ret = remap_pfn_range(vma, vma->vm_start + offset, page_to_pfn(sg_page(sg)),
-					      sg->length, vma->vm_page_prot);
+					      length, vma->vm_page_prot);
 			if (ret)
 				goto out;
-			offset += sg->length;
+			offset += length;
 		}
 	}
 
-- 
2.34.1

Claude review: accel/qaic: Add overflow check to remap_pfn_range during mmap

[Claude Code Review Bot]

Patch Review

**Positive aspects:**
- Correctly identifies a real security bug (CWE-787 / out-of-bounds mapping).
- Uses `check_add_overflow()` / `check_sub_overflow()` rather than hand-rolled arithmetic — good practice for kernel code.
- The truncation logic and early-exit on `length == 0` are correct.

**Issues:**

1. **Coding style: missing braces on `else` branch (kernel style violation)**

```c
			if (remap_end > vma->vm_end) {
				if (check_sub_overflow(vma->vm_end, remap_start, &length))
					return -EINVAL;
			} else
				length = sg->length;
```

The kernel coding style (Documentation/process/coding-style.rst §3) requires that if one branch of an `if/else` uses braces, the other must too. The `if` block has braces but the `else` does not. Should be:

```c
			} else {
				length = sg->length;
			}
```

This will trip `checkpatch.pl` warnings.

2. **Comment typo: spurious "so"**

```c
			/* if sg is too large for the VMA, so truncate it to fit */
```

Should be either "if sg is too large for the VMA, truncate it to fit" or "sg is too large for the VMA, so truncate it to fit". Currently it reads as a mashup of both.

3. **`return -EINVAL` inside a `for` loop that iterates over DMA mappings — potential cleanup concern**

The original code jumps to `goto out` on error (which just returns `ret`). The new overflow checks use `return -EINVAL` directly:

```c
			if (check_add_overflow(vma->vm_start, offset, &remap_start))
				return -EINVAL;
			if (check_add_overflow(remap_start, sg->length, &remap_end))
				return -EINVAL;
			...
				if (check_sub_overflow(vma->vm_end, remap_start, &length))
					return -EINVAL;
```

In the current code this is fine because there's no cleanup needed in the `out:` label — it just does `return ret`. However, mixing `return` and `goto out` in the same function is a maintenance hazard; if someone later adds cleanup to the `out:` path (e.g., for partial unmap on error), these early returns would bypass it. Consider changing these to `ret = -EINVAL; goto out;` for consistency.

4. **The overflow checks on `vma->vm_start + offset` are arguably redundant**

`remap_start = vma->vm_start + offset` cannot overflow if the loop is functioning correctly, because `offset` only grows by amounts that were already verified to fit within `[vm_start, vm_end)`. After the truncation, `offset += length` where `length ≤ vma->vm_end - remap_start`, so offset stays bounded. The first `check_add_overflow` is therefore defensive — not harmful, but also not strictly necessary since `offset` is already bounded by prior iterations. The second `check_add_overflow` (for `remap_end`) is more valuable since `sg->length` comes from the scatterlist and is the untrusted quantity.

This isn't a request to remove them — belt-and-suspenders is fine for a security fix — just noting that the real protection comes from the `remap_end > vma->vm_end` comparison and the truncation.

5. **Minor: `sg->length` is `unsigned int`, mixed with `unsigned long` arithmetic**

`check_add_overflow(remap_start, sg->length, &remap_end)` mixes `unsigned long` and `unsigned int`. The `check_add_overflow` macro handles type promotion correctly in modern kernels, so this works, but it's worth noting that `sg->length` is implicitly widened.

**Summary:** The fix is correct and addresses a real vulnerability. The style issues (missing braces, comment typo) should be fixed before merge. Consider using `goto out` instead of direct `return` for consistency with the function's existing error-handling pattern.

---
Generated by Claude Code Patch Reviewer

Claude review: accel/qaic: Add overflow check to remap_pfn_range during mmap

[Claude Code Review Bot]

Overall Series Review

Subject: accel/qaic: Add overflow check to remap_pfn_range during mmap
Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
Patches: 1
Reviewed: 2026-05-05T09:56:03.607793

---

This is a single-patch security fix for the QAIC accelerator driver's `mmap` handler. The vulnerability is real: without bounds checking, `remap_pfn_range()` can map memory beyond the VMA boundary when the BO's scatterlist is larger than the mmap'd region. This leads to use-after-free on `munmap()` since the kernel only tears down the VMA-sized region.

The fix is correct in concept and uses the right overflow-checked arithmetic helpers. There are a few style and logic nits worth addressing.

**Verdict: The fix is sound and important. Minor issues below should be addressed before merge.**

---
Generated by Claude Code Patch Reviewer