[PATCH] drm/qxl: validate relocation dst_offset against the destination BO

[PATCH] drm/qxl: validate relocation dst_offset against the destination BO

[Aldo Ariel Panzardo]

qxl_process_single_command() copies relocation entries from userspace and
passes them to apply_reloc() / apply_surf_reloc() without checking
whether dst_offset falls within the destination buffer object.

apply_reloc() writes 8 bytes and apply_surf_reloc() writes 4 bytes at
the byte offset named by the relocation.  A userspace-chosen offset that
exceeds the BO's allocation leads to an out-of-bounds write into
adjacent slab memory.

When the destination is the command/release BO itself (dst_handle == 0),
the write lands relative to the release's own release_info header.  An
offset of zero overwrites the release_info.id, which the garbage
collector later uses as a release index, leading to a use-after-free of
a release chosen by userspace.  Both paths are reachable from any render
client (DRM_AUTH).

Add two bounds checks:

  - When writing into the release's own BO (dst_handle == 0), require
    that dst_offset points past the release_info header and stays within
    the command data area.

  - For every relocation, require that the final byte offset plus the
    write width does not exceed the destination BO size.

Fixes: f64122c1f6ad ("drm: add qxl driver.")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 591b026ce..b62f32b6d 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -227,10 +227,27 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 				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 >= sizeof(union qxl_release_info) +
+						cmd->command_size) {
+				ret = -EINVAL;
+				goto out_free_bos;
+			}
 			reloc_info[i].dst_bo = cmd_bo;
 			reloc_info[i].dst_offset = reloc.dst_offset + release->release_offset;
 		}
 
+		{
+			size_t write_size = reloc.reloc_type == QXL_RELOC_TYPE_BO ?
+					    sizeof(uint64_t) : sizeof(uint32_t);
+
+			if (reloc_info[i].dst_offset + write_size >
+			    reloc_info[i].dst_bo->tbo.base.size) {
+				ret = -EINVAL;
+				goto out_free_bos;
+			}
+		}
+
 		/* reserve and validate the reloc dst bo */
 		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
 			ret = qxlhw_handle_to_bo(file_priv, reloc.src_handle, release,
-- 
2.43.0

Re: [PATCH] drm/qxl: validate relocation dst_offset against the destination BO

[sashiko-bot]

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

New issues:
- [Critical] The newly added bounds check against `dst_bo->tbo.base.size` is susceptible to a 32-bit integer overflow, allowing out-of-bounds writes.
- [High] The bounds check for `dst_handle == 0` relocations only validates the starting offset and fails to account for the width of the write.

Pre-existing issues:
- [Critical] Missing `RELEASE_SIZE` limit check allows userspace to copy arbitrarily large command payloads into a fixed 256-byte release slot.
- [High] Relocations with unaligned offsets that cross a page boundary will overflow the single-page fixmap mapping in `apply_reloc` and `apply_surf_reloc`.
- [High] The `reloc.src_offset` field is unsafely truncated and never validated against the source buffer object's size, allowing out-of-bounds QXL physical addresses to be generated.
--

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

drm/qxl: validate relocation dst_offset against the destination BO

This patch adds bounds checks to qxl_process_single_command() to prevent
out-of-bounds writes when processing relocations. It verifies that dst_offset
stays within the release's command data area and that the final offset plus
write width does not exceed the destination BO size.

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 591b026ceff9c..b62f32b6dc81a 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -227,10 +227,27 @@ static int qxl_process_single_command(struct qxl_device *qdev,

[Severity: Critical]
This is a pre-existing issue, but does qxl_process_single_command() allow a
large overflow when copying the command payload?

The function permits cmd->command_size up to
PAGE_SIZE - sizeof(union qxl_release_info). However,
qxl_alloc_release_reserved() ignores the requested size and allocates exactly
RELEASE_SIZE (256 bytes).

The copy_from_user_inatomic_nontemporal() call then writes up to ~4032 bytes
directly into the 256-byte slot:

qxl_process_single_command() {
    ...
    unwritten = copy_from_user_inatomic_nontemporal
        (fb_cmd + sizeof(union qxl_release_info) + 
         (release->release_offset & ~PAGE_MASK),
         u64_to_user_ptr(cmd->command), cmd->command_size);
    ...
}

If the release offset is near the end of the mapped page, could this
completely overflow the single-page mapping and overwrite adjacent memory?

>  				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 >= sizeof(union qxl_release_info) +
> +						cmd->command_size) {

[Severity: High]
Is the command bounds check missing an addition for the write width?

If a userspace render client sets reloc.dst_offset to
sizeof(union qxl_release_info) + cmd->command_size - 1, this check passes.
However, apply_reloc() writes 8 bytes, which extends 7 bytes past the allowed
command size.

Because multiple releases are tightly packed in cmd_bo, can this
out-of-bounds write corrupt the qxl_release_info header of the adjacent
release?

> +				ret = -EINVAL;
> +				goto out_free_bos;
> +			}
>  			reloc_info[i].dst_bo = cmd_bo;
>  			reloc_info[i].dst_offset = reloc.dst_offset + release->release_offset;
>  		}
>  
> +		{
> +			size_t write_size = reloc.reloc_type == QXL_RELOC_TYPE_BO ?
> +					    sizeof(uint64_t) : sizeof(uint32_t);
> +
> +			if (reloc_info[i].dst_offset + write_size >
> +			    reloc_info[i].dst_bo->tbo.base.size) {

[Severity: Critical]
Is this bounds check susceptible to a 32-bit integer overflow?

Because size_t is 32-bit on 32-bit platforms, if a render client provides a
dst_offset like 0xFFFFFFFC and write_size is 8, the addition wraps around
to 4, which bypasses the check against tbo.base.size.

The truncated offset 0xFFFFFFFC would mask to a negative page offset (-4096),
which is then passed to the mapping function:

qxl_bo_kmap_atomic_page() {
    ...
    return io_mapping_map_atomic_wc(map, offset + page_offset);
}

Could this map physical memory preceding the VRAM BO into the kernel fixmap
and overwrite it?

[Severity: High]
This is a pre-existing issue, but should this bounds check also verify that
the write doesn't cross a page boundary?

If a user submits a relocation with a valid dst_offset within the BO size
but unaligned and near the page end (e.g., dst_offset & ~PAGE_MASK == 4092),
the 8-byte write will span bytes 4092 to 4099:

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

Since qxl_bo_kmap_atomic_page() only maps a single 4096-byte page, does this
overwrite the adjacent virtual page in the kernel's fixmap area?

> +				ret = -EINVAL;
> +				goto out_free_bos;
> +			}
> +		}
> +
>  		/* reserve and validate the reloc dst bo */
>  		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {

[Severity: High]
This is a pre-existing issue, but reloc.src_offset appears to be unsafely
truncated and used without validation.

In qxl_process_single_command(), reloc.src_offset (which is a __u64 from
userspace) is assigned to reloc_info[i].src_offset, which is a signed 32-bit
int:

qxl_process_single_command() {
    ...
    reloc_info[i].src_offset = reloc.src_offset;
    ...
}

It is then passed to qxl_bo_physical_address() without any bounds check
against src_bo->tbo.base.size:

qxl_bo_physical_address() {
    ...
    return slot->high_bits | ((bo->tbo.resource->start << PAGE_SHIFT) +
                              offset);
}

Can a forged or negative offset generate a physical address pointing
completely outside the source BO, allowing the guest to bypass protections?

>  			ret = qxlhw_handle_to_bo(file_priv, reloc.src_handle, release,

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