Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/qxl: Silent failure in apply_reloc allows bypassing hardware relocations - [Medium] drm/qxl: Meaningful code and commit message mismatch on BO reservation --
commit 19602cc9bae945e0036cb9d92dfb3b6be072f8de Author: Hui Peng <[email protected]> drm/qxl: fix vmalloc OOB write, surface size overflow, and BO reloc leaks This commit addresses multiple memory safety and resource management issues in the QXL driver, including out-of-bounds kernel vmalloc writes during relocation processing, integer overflows in surface size calculations, and resource leaks. > 3. In qxl_process_single_command(), prevent overwriting the union > qxl_release_info header at offset 0 of cmd_bo, and reserve/unreserve > non-command dst_bo buffers around apply_reloc()/apply_surf_reloc(). [Severity: Medium] The commit message mentions adding logic to reserve/unreserve non-command dst_bo buffers around apply_reloc() and apply_surf_reloc(). Is this description accurate? Looking at the relocation loop in qxl_process_single_command(), there are no local reserve or unreserve calls added by this patch. It seems qxlhw_handle_to_bo() adds dst_bo to the release object, which is then batch-reserved by qxl_release_reserve_list() before the loop. The buffers appear to be safely reserved already, but does the commit message need an update to reflect the actual batch reservation mechanism used? > diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c > index 591b026ceff9c..6bb609bc6a7ea 100644 > --- a/drivers/gpu/drm/qxl/qxl_ioctl.c > +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c > @@ -89,6 +89,8 @@ apply_reloc(struct qxl_device *qdev, struct qxl_reloc_info > *info) > void *reloc_page; > > reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, > info->dst_offset & PAGE_MASK); > + if (!reloc_page) > + return; [Severity: High] Does returning early without an error code here allow unpatched commands to reach the hardware? If qxl_bo_kmap_atomic_page() fails (for example, due to vmalloc exhaustion when a user specifies a dumb buffer for dst_bo), this silently aborts the relocation. Since the caller loop in qxl_process_single_command() assumes success: for (i = 0; i < cmd->relocs_num; ++i) { if (reloc_info[i].type == QXL_RELOC_TYPE_BO) apply_reloc(qdev, &reloc_info[i]); ... } could this allow a user to bypass validation and submit unpatched, user-controlled physical addresses to the virtual GPU? Should these apply functions return an error code to properly abort the ioctl? > *(uint64_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = > qxl_bo_physical_address(qdev, > > info->src_bo, > > info->src_offset); > @@ -105,6 +107,8 @@ apply_surf_reloc(struct qxl_device *qdev, struct > qxl_reloc_info *info) > id = info->src_bo->surface_id; > > reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, > info->dst_offset & PAGE_MASK); > + if (!reloc_page) > + return; [Severity: High] Similar to apply_reloc() above, this early return silently ignores the map failure for surface relocations, leading to the same risk of submitting a corrupted command to the hardware. > *(uint32_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = id; > qxl_bo_kunmap_atomic_page(qdev, info->dst_bo, reloc_page); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
