Re: [PATCH v2 2/4] drm/qxl: reject command sizes that exceed the release slot

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect pointer arithmetic when setting `mm_time` in `qxl_process_single_command` corrupts the first release slot in the BO page instead of the currently allocated one.
- [High] An integer overflow in the bounds check for `reloc.dst_offset` allows bypass of the command buffer boundary checks, leading to out-of-bounds writes into other release slots.
- [Medium] Userspace can spuriously receive `-EFAULT` if the command payload is paged out, because the driver lacks a page-fault fallback.
--

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

drm/qxl: reject command sizes that exceed the release slot

This commit adds a check in qxl_alloc_release_reserved() to reject
allocations where the requested size exceeds the slot size for the given
release type. This prevents a heap buffer overflow that could occur when
command payloads are copied from userspace.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
> index 06979d0e8a9f0..049ad167fe128 100644
> --- a/drivers/gpu/drm/qxl/qxl_release.c
> +++ b/drivers/gpu/drm/qxl/qxl_release.c
> @@ -312,6 +312,9 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
>  		return -EINVAL;
>  	}
>  
> +	if (size > release_size_per_bo[cur_idx])
> +		return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but while this size check prevents one form of
buffer overflow, there appear to be other out-of-bounds writes in
qxl_process_single_command() in drivers/gpu/drm/qxl/qxl_ioctl.c.

For example, when setting mm_time, the code seems to ignore the in-page offset:

drivers/gpu/drm/qxl/qxl_ioctl.c:qxl_process_single_command() {
    ...
    struct qxl_drawable *draw = fb_cmd;

    draw->mm_time = qdev->rom->mm_clock;
    ...
}

Because fb_cmd points to the base of the mapped page, doesn't this write to
the mm_time field of the first drawable (slot 0) in the mapped page instead of
the currently allocated one at (release->release_offset & ~PAGE_MASK)?

This could corrupt previously submitted commands in the ring and leave the
active slot's mm_time uninitialized or controlled by userspace. Note that
subsequent patches in this series do not appear to fix this issue.

[Severity: High]
This is also a pre-existing issue, but there seems to be an integer overflow
in the command buffer boundary checks in the same function:

drivers/gpu/drm/qxl/qxl_ioctl.c:qxl_process_single_command() {
    ...
    if (reloc.dst_offset < sizeof(union qxl_release_info) ||
        reloc.dst_offset + write_size >
        sizeof(union qxl_release_info) + cmd->command_size) {
    ...
    reloc_info[i].dst_offset = reloc.dst_offset + release->release_offset;
    ...
}

Since reloc.dst_offset + write_size uses an unprotected 64-bit addition, could
a maliciously crafted large reloc.dst_offset (e.g., near U64_MAX) wrap around
and bypass this bounds check?

If it bypasses the check, the large offset is added to release->release_offset
and implicitly truncated to 32 bits, which could yield an offset pointing
before the current command slot. Would this allow arbitrary out-of-bounds
writes to previous release slots in the cmd_bo ring buffer?

Note that follow-up patches in this series do not seem to fix this issue.
Patch 3 only adds a page-boundary check which does not prevent writing to
adjacent slots within the same page, and Patch 4 only validates src_offset.

[Severity: Medium]
This is another pre-existing issue, but how are page faults handled when
copying the command payload?

drivers/gpu/drm/qxl/qxl_ioctl.c:qxl_process_single_command() {
    ...
    fb_cmd = qxl_bo_kmap_atomic_page(qdev, cmd_bo, (release->release_offset & PAGE_MASK));
    unwritten = copy_from_user_inatomic_nontemporal(...);
    ...
    if (unwritten) {
        ret = -EFAULT;
    }
    ...
}

Since qxl_bo_kmap_atomic_page() creates an atomic mapping which disables
page faults, if the user buffer is paged out,
copy_from_user_inatomic_nontemporal() will fail.

Instead of falling back to a slow path to fault the pages in, this returns
-EFAULT directly. Will this cause valid ioctl calls to randomly fail under
normal memory pressure when a user page is swapped out?

>  	idr_ret = qxl_release_alloc(qdev, type, release);
>  	if (idr_ret < 0) {
>  		if (rbo)

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