Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Critical] The newly added bounds check against `dst_bo->tbo.base.size` is susceptible to a 32-bit integer overflow, allowing out-of-bounds writes. - [High] The bounds check for `dst_handle == 0` relocations only validates the starting offset and fails to account for the width of the write. Pre-existing issues: - [Critical] Missing `RELEASE_SIZE` limit check allows userspace to copy arbitrarily large command payloads into a fixed 256-byte release slot. - [High] Relocations with unaligned offsets that cross a page boundary will overflow the single-page fixmap mapping in `apply_reloc` and `apply_surf_reloc`. - [High] The `reloc.src_offset` field is unsafely truncated and never validated against the source buffer object's size, allowing out-of-bounds QXL physical addresses to be generated. -- commit 6b7944f1931ada1db6b099ba1549d51a63365bb2 Author: Aldo Ariel Panzardo <[email protected]> drm/qxl: validate relocation dst_offset against the destination BO This patch adds bounds checks to qxl_process_single_command() to prevent out-of-bounds writes when processing relocations. It verifies that dst_offset stays within the release's command data area and that the final offset plus write width does not exceed the destination BO size. > diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c > index 591b026ceff9c..b62f32b6dc81a 100644 > --- a/drivers/gpu/drm/qxl/qxl_ioctl.c > +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c > @@ -227,10 +227,27 @@ static int qxl_process_single_command(struct qxl_device > *qdev, [Severity: Critical] This is a pre-existing issue, but does qxl_process_single_command() allow a large overflow when copying the command payload? The function permits cmd->command_size up to PAGE_SIZE - sizeof(union qxl_release_info). However, qxl_alloc_release_reserved() ignores the requested size and allocates exactly RELEASE_SIZE (256 bytes). The copy_from_user_inatomic_nontemporal() call then writes up to ~4032 bytes directly into the 256-byte slot: qxl_process_single_command() { ... unwritten = copy_from_user_inatomic_nontemporal (fb_cmd + sizeof(union qxl_release_info) + (release->release_offset & ~PAGE_MASK), u64_to_user_ptr(cmd->command), cmd->command_size); ... } If the release offset is near the end of the mapped page, could this completely overflow the single-page mapping and overwrite adjacent memory? > 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 >= sizeof(union qxl_release_info) + > + cmd->command_size) { [Severity: High] Is the command bounds check missing an addition for the write width? If a userspace render client sets reloc.dst_offset to sizeof(union qxl_release_info) + cmd->command_size - 1, this check passes. However, apply_reloc() writes 8 bytes, which extends 7 bytes past the allowed command size. Because multiple releases are tightly packed in cmd_bo, can this out-of-bounds write corrupt the qxl_release_info header of the adjacent release? > + ret = -EINVAL; > + goto out_free_bos; > + } > reloc_info[i].dst_bo = cmd_bo; > reloc_info[i].dst_offset = reloc.dst_offset + > release->release_offset; > } > > + { > + size_t write_size = reloc.reloc_type == > QXL_RELOC_TYPE_BO ? > + sizeof(uint64_t) : sizeof(uint32_t); > + > + if (reloc_info[i].dst_offset + write_size > > + reloc_info[i].dst_bo->tbo.base.size) { [Severity: Critical] Is this bounds check susceptible to a 32-bit integer overflow? Because size_t is 32-bit on 32-bit platforms, if a render client provides a dst_offset like 0xFFFFFFFC and write_size is 8, the addition wraps around to 4, which bypasses the check against tbo.base.size. The truncated offset 0xFFFFFFFC would mask to a negative page offset (-4096), which is then passed to the mapping function: qxl_bo_kmap_atomic_page() { ... return io_mapping_map_atomic_wc(map, offset + page_offset); } Could this map physical memory preceding the VRAM BO into the kernel fixmap and overwrite it? [Severity: High] This is a pre-existing issue, but should this bounds check also verify that the write doesn't cross a page boundary? If a user submits a relocation with a valid dst_offset within the BO size but unaligned and near the page end (e.g., dst_offset & ~PAGE_MASK == 4092), the 8-byte write will span bytes 4092 to 4099: 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(...); } Since qxl_bo_kmap_atomic_page() only maps a single 4096-byte page, does this overwrite the adjacent virtual page in the kernel's fixmap area? > + ret = -EINVAL; > + goto out_free_bos; > + } > + } > + > /* reserve and validate the reloc dst bo */ > if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) { [Severity: High] This is a pre-existing issue, but reloc.src_offset appears to be unsafely truncated and used without validation. In qxl_process_single_command(), reloc.src_offset (which is a __u64 from userspace) is assigned to reloc_info[i].src_offset, which is a signed 32-bit int: qxl_process_single_command() { ... reloc_info[i].src_offset = reloc.src_offset; ... } It is then passed to qxl_bo_physical_address() without any bounds check against src_bo->tbo.base.size: qxl_bo_physical_address() { ... return slot->high_bits | ((bo->tbo.resource->start << PAGE_SHIFT) + offset); } Can a forged or negative offset generate a physical address pointing completely outside the source BO, allowing the guest to bypass protections? > ret = qxlhw_handle_to_bo(file_priv, reloc.src_handle, > release, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
