Re: [PATCH v2 3/4] drm/qxl: reject relocations whose writes cross a page boundary

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] An integer overflow in `reloc.dst_offset + write_size` bypasses bounds checks, leading to out-of-bounds writes within the shared command buffer.
- [Critical] The fallback path in `qxl_bo_kmap_atomic_page` improperly multiplies an already byte-scaled offset by `PAGE_SIZE`, leading to massive out-of-bounds kernel memory corruption.
- [High] Type truncation of `dst_offset` and passing an unsigned value to a signed `int` causes negative memory mapping before the VRAM buffer.
- [High] `qxl_bo_kmap_atomic_page` fails to increment `map_count` in the fallback path when `bo->kptr` is already set, causing a reference count underflow when unmapped.
--

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

drm/qxl: reject relocations whose writes cross a page boundary

This commit adds a check to reject relocations in the qxl driver where the
write would cross a page boundary, preventing out-of-bounds writes into the
kernel's fixmap area.

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index e727a35c94136..9fba6e26d048b 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
[ ... ]
> @@ -247,6 +247,12 @@ static int qxl_process_single_command(struct qxl_device *qdev,
[ ... ]
>  		if (reloc.dst_handle) {
>  			ret = qxlhw_handle_to_bo(file_priv, reloc.dst_handle, release,
>  						 &reloc_info[i].dst_bo);
>  			if (ret)
>  				goto out_free_bos;
>  			reloc_info[i].dst_offset = reloc.dst_offset;
>  		} else {
>  			if (reloc.dst_offset < sizeof(union qxl_release_info) ||
>  			    reloc.dst_offset + write_size >
>  			    sizeof(union qxl_release_info) + cmd->command_size) {
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Critical]
This isn't a bug introduced by this patch, but does the addition
reloc.dst_offset + write_size overflow?

If a user sets reloc.dst_offset to a value like 0xFFFFFFFFFFFFFFF8 (-8),
the addition of write_size (8) would wrap around to 0. Since the first check
(0xFFFFFFFFFFFFFFF8 < 40) is false, and the second check (0 > 40 +
cmd->command_size) is also false, the bounds checks are bypassed.

This could allow out-of-bounds writes into the shared cmd_bo ring buffer.

>  				ret = -EINVAL;
>  				goto out_free_bos;
>  			}
>  			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) {
>  			ret = -EINVAL;
>  			goto out_free_bos;
>  		}
> 
> +		if ((reloc_info[i].dst_offset & ~PAGE_MASK) + write_size >
> +		    PAGE_SIZE) {
> +			ret = -EINVAL;
> +			goto out_free_bos;
> +		}
> +
>  		/* reserve and validate the reloc dst bo */


While reviewing the relocation bounds checks, I noticed a few pre-existing
issues in the page mapping code called during relocations:

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);
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to pass page_offset
as a signed int?

The callers pass dst_offset & PAGE_MASK to qxl_bo_kmap_atomic_page(). If a
user provides a large offset like 0x80000000 for a large buffer, it will be
interpreted as a negative integer (-2147483648) and sign-extended in
io_mapping_map_atomic_wc(), mapping memory before the intended physical VRAM
buffer.


drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kmap_atomic_page() {
    ...
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;

	rptr += page_offset * PAGE_SIZE;
	return rptr;
}

[Severity: Critical]
This isn't a bug introduced by this patch, but is page_offset incorrectly
multiplied by PAGE_SIZE here?

The callers already pass a byte-scaled offset (e.g., info->dst_offset &
PAGE_MASK) as the page_offset argument. If the fallback path is taken
(e.g. for a buffer in TTM_PL_SYSTEM), multiplying this byte offset by
PAGE_SIZE again causes a massive out-of-bounds kernel pointer calculation.


[Severity: High]
This isn't a bug introduced by this patch, but does the fallback path fail
to update the map count when bo->kptr is already set?

drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kmap_atomic_page() {
    ...
fallback:
	if (bo->kptr) {
		rptr = bo->kptr + (page_offset * PAGE_SIZE);
		return rptr;
	}
    ...
}

If a buffer is already mapped via the fallback path (for instance, exported
via PRIME and vmapped by another driver), returning bo->kptr without
incrementing map_count creates a mismatch.

When it is later unmapped:

drivers/gpu/drm/qxl/qxl_object.c:qxl_bo_kunmap_atomic_page() {
    ...
 fallback:
	qxl_bo_vunmap_locked(bo);
}

The qxl_bo_vunmap_locked() function unconditionally decrements map_count to 0,
prematurely unmapping the buffer while the original map user is still active.

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