Re: [PATCH] drm/qxl: fix vmalloc OOB write, surface size overflow, and BO reloc leaks

[sashiko-bot@kernel.org]

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 <benquike@gmail.com>

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/20260919215228.3469508-1-benquike@gmail.com?part=1