Re: [PATCH v2 4/4] drm/qxl: validate relocation src_offset and fix type truncation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Arbitrary kernel memory corruption due to incorrect byte-to-page arithmetic in CPU-domain fallback path.
- [High] Incomplete mitigation for VRAM out-of-bounds access due to lack of command extent validation.
- [High] Unaligned memory accesses to Write-Combining (WC) I/O memory via user-controlled `dst_offset`.
--

commit dafc93fc8afc46baa3769cdc18eb27a22986da61
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

drm/qxl: validate relocation src_offset and fix type truncation

The UAPI struct drm_qxl_reloc declares src_offset as __u64, but the
internal struct qxl_reloc_info stores it as a signed int.  The
assignment silently truncates the 64-bit value without any bounds check.
A negative or very large truncated offset is then passed to
qxl_bo_physical_address(), generating a physical address that points outside
the source BO.

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 9fba6e26d048b..6559ba7d86f90 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c

[ ... ]

> @@ -259,6 +259,10 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>  						 &reloc_info[i].src_bo);
>  			if (ret)
>  				goto out_free_bos;
> +			if (reloc.src_offset >= reloc_info[i].src_bo->tbo.base.size) {

[Severity: High]
This is a pre-existing issue, but does this check fully mitigate the
out-of-bounds access?

If a user submits a DRAW command and sets reloc.src_offset to a valid offset
near the end of the BO (e.g. bo->size - 1), this check passes.  However,
doesn't the hypervisor then read the full extent dictated by the DRAW
command payload, crossing the BO boundary into adjacent VRAM?

Would it be safer to also validate that the size of the access requested by
the QXL command fits within the source BO?

> +				ret = -EINVAL;
> +				goto out_free_bos;
> +			}
>  			reloc_info[i].src_offset = reloc.src_offset;
>  		} else {
>  			reloc_info[i].src_bo = NULL;

[Severity: Critical]
This isn't a bug introduced by this patch, but can this code cause an
arbitrary out-of-bounds kernel memory write?

In the CPU-domain fallback path for mapping a BO:

drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kmap_atomic_page() {
...
	offset = bo->tbo.resource->start << PAGE_SHIFT;
	return io_mapping_map_atomic_wc(map, offset + page_offset);
fallback:
	if (bo->kptr) {
		rptr = bo->kptr + (page_offset * PAGE_SIZE);
		return rptr;
	}

	ret = qxl_bo_vmap_locked(bo, &bo_map);
	if (ret)
		return NULL;
	rptr = bo_map.vaddr; /* TODO: Use mapping abstraction properly */

	rptr += page_offset * PAGE_SIZE;
	return rptr;
}

The page_offset parameter is passed as a byte offset (e.g., from
dst_offset & PAGE_MASK in apply_reloc()).  If a user creates a CPU-domain
dumb buffer and provides a large dst_offset, doesn't multiplying page_offset
by PAGE_SIZE here generate a massively out-of-bounds pointer?

[Severity: High]
This is a pre-existing issue, but can these unaligned assignments cause a
kernel panic (alignment fault) on strict-alignment architectures like ARM64?

When a user submits an execbuffer command with a dst_offset that is not
aligned to an 8-byte boundary:

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);
	qxl_bo_kunmap_atomic_page(qdev, info->dst_bo, reloc_page);
}

and

drivers/gpu/drm/qxl/qxl_ioctl.c:apply_surf_reloc() {
...
	reloc_page = qxl_bo_kmap_atomic_page(qdev, info->dst_bo, info->dst_offset & PAGE_MASK);
	*(uint32_t *)(reloc_page + (info->dst_offset & ~PAGE_MASK)) = id;
	qxl_bo_kunmap_atomic_page(qdev, info->dst_bo, reloc_page);
}

Since reloc_page is mapped as Write-Combining (WC) I/O memory via
io_mapping_map_atomic_wc(), doesn't performing an unaligned 64-bit or 32-bit
direct memory write trigger a fatal Data Abort?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913223000.695299-1-qwe.aldo@gmail.com?part=4