Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Incorrect pointer arithmetic when setting `mm_time` in `qxl_process_single_command` corrupts the first release slot in the BO page instead of the currently allocated one. - [High] An integer overflow in the bounds check for `reloc.dst_offset` allows bypass of the command buffer boundary checks, leading to out-of-bounds writes into other release slots. - [Medium] Userspace can spuriously receive `-EFAULT` if the command payload is paged out, because the driver lacks a page-fault fallback. -- commit 32830020edc55718de9f74f8287e937fa568d5e5 Author: Aldo Ariel Panzardo <[email protected]> drm/qxl: reject command sizes that exceed the release slot This commit adds a check in qxl_alloc_release_reserved() to reject allocations where the requested size exceeds the slot size for the given release type. This prevents a heap buffer overflow that could occur when command payloads are copied from userspace. Cc: [email protected] > diff --git a/drivers/gpu/drm/qxl/qxl_release.c > b/drivers/gpu/drm/qxl/qxl_release.c > index 06979d0e8a9f0..049ad167fe128 100644 > --- a/drivers/gpu/drm/qxl/qxl_release.c > +++ b/drivers/gpu/drm/qxl/qxl_release.c > @@ -312,6 +312,9 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, > unsigned long size, > return -EINVAL; > } > > + if (size > release_size_per_bo[cur_idx]) > + return -EINVAL; > + [Severity: High] This is a pre-existing issue, but while this size check prevents one form of buffer overflow, there appear to be other out-of-bounds writes in qxl_process_single_command() in drivers/gpu/drm/qxl/qxl_ioctl.c. For example, when setting mm_time, the code seems to ignore the in-page offset: drivers/gpu/drm/qxl/qxl_ioctl.c:qxl_process_single_command() { ... struct qxl_drawable *draw = fb_cmd; draw->mm_time = qdev->rom->mm_clock; ... } Because fb_cmd points to the base of the mapped page, doesn't this write to the mm_time field of the first drawable (slot 0) in the mapped page instead of the currently allocated one at (release->release_offset & ~PAGE_MASK)? This could corrupt previously submitted commands in the ring and leave the active slot's mm_time uninitialized or controlled by userspace. Note that subsequent patches in this series do not appear to fix this issue. [Severity: High] This is also a pre-existing issue, but there seems to be an integer overflow in the command buffer boundary checks in the same function: drivers/gpu/drm/qxl/qxl_ioctl.c:qxl_process_single_command() { ... if (reloc.dst_offset < sizeof(union qxl_release_info) || reloc.dst_offset + write_size > sizeof(union qxl_release_info) + cmd->command_size) { ... reloc_info[i].dst_offset = reloc.dst_offset + release->release_offset; ... } Since reloc.dst_offset + write_size uses an unprotected 64-bit addition, could a maliciously crafted large reloc.dst_offset (e.g., near U64_MAX) wrap around and bypass this bounds check? If it bypasses the check, the large offset is added to release->release_offset and implicitly truncated to 32 bits, which could yield an offset pointing before the current command slot. Would this allow arbitrary out-of-bounds writes to previous release slots in the cmd_bo ring buffer? Note that follow-up patches in this series do not seem to fix this issue. Patch 3 only adds a page-boundary check which does not prevent writing to adjacent slots within the same page, and Patch 4 only validates src_offset. [Severity: Medium] This is another pre-existing issue, but how are page faults handled when copying the command payload? drivers/gpu/drm/qxl/qxl_ioctl.c:qxl_process_single_command() { ... fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_MASK)); unwritten = copy_from_user_inatomic_nontemporal(...); ... if (unwritten) { ret = -EFAULT; } ... } Since qxl_bo_kmap_atomic_page() creates an atomic mapping which disables page faults, if the user buffer is paged out, copy_from_user_inatomic_nontemporal() will fail. Instead of falling back to a slow path to fault the pages in, this returns -EFAULT directly. Will this cause valid ioctl calls to randomly fail under normal memory pressure when a user page is swapped out? > idr_ret = qxl_release_alloc(qdev, type, release); > if (idr_ret < 0) { > if (rbo) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
