Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Arbitrary kernel memory corruption due to incorrect byte-to-page arithmetic in CPU-domain fallback path. - [High] Incomplete mitigation for VRAM out-of-bounds access due to lack of command extent validation. - [High] Unaligned memory accesses to Write-Combining (WC) I/O memory via user-controlled `dst_offset`. -- commit dafc93fc8afc46baa3769cdc18eb27a22986da61 Author: Aldo Ariel Panzardo <[email protected]> drm/qxl: validate relocation src_offset and fix type truncation The UAPI struct drm_qxl_reloc declares src_offset as __u64, but the internal struct qxl_reloc_info stores it as a signed int. The assignment silently truncates the 64-bit value without any bounds check. A negative or very large truncated offset is then passed to qxl_bo_physical_address(), generating a physical address that points outside the source BO. > diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c > index 9fba6e26d048b..6559ba7d86f90 100644 > --- a/drivers/gpu/drm/qxl/qxl_ioctl.c > +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c [ ... ] > @@ -259,6 +259,10 @@ static int qxl_process_single_command(struct qxl_device > *qdev, > &reloc_info[i].src_bo); > if (ret) > goto out_free_bos; > + if (reloc.src_offset >= > reloc_info[i].src_bo->tbo.base.size) { [Severity: High] This is a pre-existing issue, but does this check fully mitigate the out-of-bounds access? If a user submits a DRAW command and sets reloc.src_offset to a valid offset near the end of the BO (e.g. bo->size - 1), this check passes. However, doesn't the hypervisor then read the full extent dictated by the DRAW command payload, crossing the BO boundary into adjacent VRAM? Would it be safer to also validate that the size of the access requested by the QXL command fits within the source BO? > + ret = -EINVAL; > + goto out_free_bos; > + } > reloc_info[i].src_offset = reloc.src_offset; > } else { > reloc_info[i].src_bo = NULL; [Severity: Critical] This isn't a bug introduced by this patch, but can this code cause an arbitrary out-of-bounds kernel memory write? In the CPU-domain fallback path for mapping a BO: 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); 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; /* TODO: Use mapping abstraction properly */ rptr += page_offset * PAGE_SIZE; return rptr; } The page_offset parameter is passed as a byte offset (e.g., from dst_offset & PAGE_MASK in apply_reloc()). If a user creates a CPU-domain dumb buffer and provides a large dst_offset, doesn't multiplying page_offset by PAGE_SIZE here generate a massively out-of-bounds pointer? [Severity: High] This is a pre-existing issue, but can these unaligned assignments cause a kernel panic (alignment fault) on strict-alignment architectures like ARM64? When a user submits an execbuffer command with a dst_offset that is not aligned to an 8-byte boundary: drivers/gpu/drm/qxl/qxl_ioctl.c:apply_reloc() { ... reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK); *(uint64_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = qxl_bo_physical_address(qdev, info->src_bo, info->src_offset); qxl_bo_kunmap_atomic_page(qdev, info->dst_bo, reloc_page); } and drivers/gpu/drm/qxl/qxl_ioctl.c:apply_surf_reloc() { ... reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK); *(uint32_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = id; qxl_bo_kunmap_atomic_page(qdev, info->dst_bo, reloc_page); } Since reloc_page is mapped as Write-Combining (WC) I/O memory via io_mapping_map_atomic_wc(), doesn't performing an unaligned 64-bit or 32-bit direct memory write trigger a fatal Data Abort? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
