Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] An integer overflow in `reloc.dst_offset + write_size` bypasses bounds checks, leading to out-of-bounds writes within the shared command buffer. - [Critical] The fallback path in `qxl_bo_kmap_atomic_page` improperly multiplies an already byte-scaled offset by `PAGE_SIZE`, leading to massive out-of-bounds kernel memory corruption. - [High] Type truncation of `dst_offset` and passing an unsigned value to a signed `int` causes negative memory mapping before the VRAM buffer. - [High] `qxl_bo_kmap_atomic_page` fails to increment `map_count` in the fallback path when `bo->kptr` is already set, causing a reference count underflow when unmapped. -- commit 9e6139abca50852ee6ddcc5adb21b30da6e7a5ae Author: Aldo Ariel Panzardo <[email protected]> drm/qxl: reject relocations whose writes cross a page boundary This commit adds a check to reject relocations in the qxl driver where the write would cross a page boundary, preventing out-of-bounds writes into the kernel's fixmap area. > diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c > index e727a35c94136..9fba6e26d048b 100644 > --- a/drivers/gpu/drm/qxl/qxl_ioctl.c > +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c [ ... ] > @@ -247,6 +247,12 @@ static int qxl_process_single_command(struct qxl_device > *qdev, [ ... ] > if (reloc.dst_handle) { > ret = qxlhw_handle_to_bo(file_priv, reloc.dst_handle, > release, > &reloc_info[i].dst_bo); > if (ret) > goto out_free_bos; > reloc_info[i].dst_offset = reloc.dst_offset; > } else { > if (reloc.dst_offset < sizeof(union qxl_release_info) || > reloc.dst_offset + write_size > > sizeof(union qxl_release_info) + cmd->command_size) > { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Critical] This isn't a bug introduced by this patch, but does the addition reloc.dst_offset + write_size overflow? If a user sets reloc.dst_offset to a value like 0xFFFFFFFFFFFFFFF8 (-8), the addition of write_size (8) would wrap around to 0. Since the first check (0xFFFFFFFFFFFFFFF8 < 40) is false, and the second check (0 > 40 + cmd->command_size) is also false, the bounds checks are bypassed. This could allow out-of-bounds writes into the shared cmd_bo ring buffer. > ret = -EINVAL; > goto out_free_bos; > } > reloc_info[i].dst_bo = cmd_bo; > reloc_info[i].dst_offset = reloc.dst_offset + > release->release_offset; > } > > if ((u64)reloc_info[i].dst_offset + write_size > > reloc_info[i].dst_bo->tbo.base.size) { > ret = -EINVAL; > goto out_free_bos; > } > > + if ((reloc_info[i].dst_offset & ~PAGE_MASK) + write_size > > + PAGE_SIZE) { > + ret = -EINVAL; > + goto out_free_bos; > + } > + > /* reserve and validate the reloc dst bo */ While reviewing the relocation bounds checks, I noticed a few pre-existing issues in the page mapping code called during relocations: drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kmap_atomic_page() { ... offset = bo->tbo.resource->start << PAGE_SHIFT; return io_mapping_map_atomic_wc(map, offset + page_offset); } [Severity: High] This isn't a bug introduced by this patch, but is it safe to pass page_offset as a signed int? The callers pass dst_offset & PAGE_MASK to qxl_bo_kmap_atomic_page(). If a user provides a large offset like 0x80000000 for a large buffer, it will be interpreted as a negative integer (-2147483648) and sign-extended in io_mapping_map_atomic_wc(), mapping memory before the intended physical VRAM buffer. drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kmap_atomic_page() { ... fallback: if (bo->kptr) { rptr = bo->kptr + (page_offset * PAGE_SIZE); return rptr; } ret = qxl_bo_vmap_locked(bo, &bo_map); if (ret) return NULL; rptr = bo_map.vaddr; rptr += page_offset * PAGE_SIZE; return rptr; } [Severity: Critical] This isn't a bug introduced by this patch, but is page_offset incorrectly multiplied by PAGE_SIZE here? The callers already pass a byte-scaled offset (e.g., info->dst_offset & PAGE_MASK) as the page_offset argument. If the fallback path is taken (e.g. for a buffer in TTM_PL_SYSTEM), multiplying this byte offset by PAGE_SIZE again causes a massive out-of-bounds kernel pointer calculation. [Severity: High] This isn't a bug introduced by this patch, but does the fallback path fail to update the map count when bo->kptr is already set? drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kmap_atomic_page() { ... fallback: if (bo->kptr) { rptr = bo->kptr + (page_offset * PAGE_SIZE); return rptr; } ... } If a buffer is already mapped via the fallback path (for instance, exported via PRIME and vmapped by another driver), returning bo->kptr without incrementing map_count creates a mismatch. When it is later unmapped: drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kunmap_atomic_page() { ... fallback: qxl_bo_vunmap_locked(bo); } The qxl_bo_vunmap_locked() function unconditionally decrements map_count to 0, prematurely unmapping the buffer while the original map user is still active. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
