Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] The command relocation processing lacks bounds checking on `dst_offset`, allowing userspace to perform arbitrary writes to kernel or VRAM memory. - [High] The size computation in `qxl_alloc_surf_ioctl` prevents integer overflows but fails to validate that the userspace-provided `stride` is mathematically sufficient for the `width` of the surface, leaving it vulnerable to undersized VRAM allocations. -- --- Patch [1]: [PATCH v2] drm/qxl: fix 32-bit overflow in the ALLOC_SURF size computation --- commit 0b850095b7d5d290c2b02556121eaeca53cced5b Author: Aldo Ariel Panzardo <[email protected]> drm/qxl: fix 32-bit overflow in the ALLOC_SURF size computation This commit fixes a 32-bit integer overflow in the QXL surface allocation ioctl by computing the backing size as u64 and checking it against INT_MAX. It also rejects zero dimensions and handles an INT_MIN stride properly. > diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c > index 591b026ceff9c..5f84a4cf9631d 100644 > --- a/drivers/gpu/drm/qxl/qxl_ioctl.c > +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c [Severity: Critical] This is a pre-existing issue, but does qxl_process_single_command() lack bounds checking on dst_offset? A userspace client could send a QXL_EXECBUFFER ioctl with a maliciously crafted dst_offset. qxl_process_single_command() copies this without validating it against dst_bo->tbo.base.size: drivers/gpu/drm/qxl/qxl_ioctl.c:qxl_process_single_command() { ... reloc_info[i].dst_offset = reloc.dst_offset; ... } Later, apply_reloc() calls qxl_bo_kmap_atomic_page() with this out-of-bounds offset and writes the source BO's physical address to it: 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); ... } Could this allow arbitrary local privilege escalation or system crashes from any DRM_AUTH client via out-of-bounds writes? > @@ -386,12 +386,24 @@ int qxl_alloc_surf_ioctl(struct drm_device *dev, void > *data, struct drm_file *fi > struct drm_qxl_alloc_surf *param = data; > int handle; > int ret; > - int size, actual_stride; > + int actual_stride; > + u64 size; > struct qxl_surface surf; > > /* work out size allocate bo with handle */ > + if (param->stride == INT_MIN) > + return -EINVAL; > actual_stride = param->stride < 0 ? -param->stride : param->stride; > - size = actual_stride * param->height + actual_stride; > + if (!actual_stride || !param->width || !param->height) > + return -EINVAL; > + /* > + * size = actual_stride * (height + 1), computed in u64 so it cannot > + * wrap on any architecture, then bounded so it still fits the int > + * parameter of qxl_gem_object_create(). > + */ > + size = (u64)actual_stride * ((u64)param->height + 1); > + if (size > INT_MAX) > + return -EINVAL; > > surf.format = param->format; > surf.width = param->width; [Severity: High] This isn't a bug introduced by this patch, but does this allocation validate that the stride is mathematically sufficient for the width? If DRM_IOCTL_QXL_ALLOC_SURF is called with a huge param->width and a tiny actual_stride (e.g., 1), the size computation will pass the INT_MAX check, but fail to enforce stride >= width * bytes_per_pixel. Since the width is passed directly to the QXL host device, could the host subsequently overrun the guest VRAM outside the undersized allocated buffer when processing the surface? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
