Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] The patch fails to prevent out-of-bounds writes for system RAM BOs (dumb buffers) due to a preexisting flaw in qxl_bo_kmap_atomic_page's fallback branch. -- commit 816a48396f060545d332f11cc2d661437076fb2f Author: Aldo Ariel Panzardo <[email protected]> drm/qxl: validate relocation dst_offset against destination BO This patch attempts to prevent out-of-bounds writes during relocation processing by validating that the user-provided dst_offset falls within the destination buffer object. It adds bounds checks for both the command/release BO and general relocation destination BOs. > diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c > index 591b026ceff9c..e727a35c94136 100644 > --- a/drivers/gpu/drm/qxl/qxl_ioctl.c > +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c [ ... ] > @@ -227,10 +231,22 @@ static int qxl_process_single_command(struct qxl_device > *qdev, > goto out_free_bos; > reloc_info[i].dst_offset = reloc.dst_offset; > } else { [ ... ] > 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) { [Severity: Critical] This is a pre-existing issue, but does this newly added bounds check fully prevent out-of-bounds writes for system RAM BOs (dumb buffers)? The new bounds check in qxl_process_single_command verifies that dst_offset is within the BO size. However, it relies on the un-multiplied byte offset. Later, apply_reloc calls qxl_bo_kmap_atomic_page, passing dst_offset & PAGE_MASK as the page_offset argument. If a user creates a dumb buffer (placed in TTM_PL_SYSTEM) and passes its handle with a large dst_offset, qxl_bo_kmap_atomic_page in drivers/gpu/drm/qxl/qxl_object.c takes a fallback branch. That fallback branch contains the following pointer calculation: rptr += page_offset * PAGE_SIZE; Because page_offset is derived from a byte offset, multiplying it by PAGE_SIZE creates a massive out-of-bounds pointer calculation (for example, a 100MB offset becomes 400GB). Since the QXL_EXECBUFFER ioctl is accessible to any authenticated DRM client, an unprivileged user could use this path to write a 64-bit physical address or 32-bit surface ID into kernel memory, which can lead to system compromise or a kernel panic. Should the underlying multiplier bug in qxl_bo_kmap_atomic_page be addressed alongside this patch to ensure the vulnerability is fully closed? > + ret = -EINVAL; > + goto out_free_bos; > + } > + > /* reserve and validate the reloc dst bo */ > if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) { > ret = qxlhw_handle_to_bo(file_priv, reloc.src_handle, > release, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
