[PATCH v2 0/4] drm/qxl: fix multiple missing bounds checks in execbuffer relocations

[PATCH v2 0/4] drm/qxl: fix multiple missing bounds checks in execbuffer relocations

[Aldo Ariel Panzardo]

The qxl execbuffer ioctl processes user-supplied relocation entries
without adequate validation, enabling several memory-safety violations
reachable from any DRM_AUTH render client.

Patch 1 is a v2 of a previously submitted single patch.  The v1 had two
gaps identified by sashiko-bot review: the upper bound for dst_handle==0
relocations did not account for the write width, and the general BO
bounds check could overflow on ILP32.  Both are fixed in this revision.

Patches 2-4 fix pre-existing issues in the same code path that were
surfaced during the v1 review:

  - Patch 2: The release sub-allocator ignores the requested size and
    always uses 256-byte slots, but the ioctl allows commands up to
    ~4088 bytes, overflowing into adjacent slots.

  - Patch 3: Relocations with page-unaligned offsets can write past a
    single-page kmap mapping.

  - Patch 4: src_offset is silently truncated from __u64 to int and
    never validated, generating out-of-bounds physical addresses.

Aldo Ariel Panzardo (4):
  drm/qxl: validate relocation dst_offset against destination BO
  drm/qxl: reject command sizes that exceed the release slot
  drm/qxl: reject relocations whose writes cross a page boundary
  drm/qxl: validate relocation src_offset and fix type truncation

 drivers/gpu/drm/qxl/qxl_ioctl.c   | 28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/qxl/qxl_release.c |  3 +++
 2 files changed, 30 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/4] drm/qxl: validate relocation dst_offset against 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 that the
    final byte (offset + write width) 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.  Cast to u64
    before the addition so the check cannot be bypassed by a 32-bit
    integer overflow on ILP32 platforms.

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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 591b026ce..e727a35c9 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -204,6 +204,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 	for (i = 0; i < cmd->relocs_num; ++i) {
 		struct drm_qxl_reloc reloc;
 		struct drm_qxl_reloc __user *u = u64_to_user_ptr(cmd->relocs);
+		size_t write_size;
 
 		if (copy_from_user(&reloc, u + i, sizeof(reloc))) {
 			ret = -EFAULT;
@@ -220,6 +221,9 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		}
 		reloc_info[i].type = reloc.reloc_type;
 
+		write_size = reloc.reloc_type == QXL_RELOC_TYPE_BO ?
+			     sizeof(uint64_t) : sizeof(uint32_t);
+
 		if (reloc.dst_handle) {
 			ret = qxlhw_handle_to_bo(file_priv, reloc.dst_handle, release,
 						 &reloc_info[i].dst_bo);
@@ -227,10 +231,22 @@ 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 + write_size >
+			    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;
 		}
 
+		if ((u64)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 v2 1/4] drm/qxl: validate relocation dst_offset against destination BO

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The patch fails to prevent out-of-bounds writes for system RAM BOs (dumb buffers) due to a preexisting flaw in qxl_bo_kmap_atomic_page's fallback branch.
--

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

drm/qxl: validate relocation dst_offset against destination BO

This patch attempts to prevent out-of-bounds writes during relocation
processing by validating that the user-provided dst_offset falls within
the destination buffer object. It adds bounds checks for both the
command/release BO and general relocation destination BOs.

> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 591b026ceff9c..e727a35c94136 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
[ ... ]
> @@ -227,10 +231,22 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>  				goto out_free_bos;
>  			reloc_info[i].dst_offset = reloc.dst_offset;
>  		} else {
[ ... ]
>  			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) {

[Severity: Critical]
This is a pre-existing issue, but does this newly added bounds check fully
prevent out-of-bounds writes for system RAM BOs (dumb buffers)?

The new bounds check in qxl_process_single_command verifies that dst_offset
is within the BO size. However, it relies on the un-multiplied byte offset.
Later, apply_reloc calls qxl_bo_kmap_atomic_page, passing
dst_offset & PAGE_MASK as the page_offset argument.

If a user creates a dumb buffer (placed in TTM_PL_SYSTEM) and passes its
handle with a large dst_offset, qxl_bo_kmap_atomic_page in
drivers/gpu/drm/qxl/qxl_object.c takes a fallback branch. That fallback
branch contains the following pointer calculation:

rptr += page_offset * PAGE_SIZE;

Because page_offset is derived from a byte offset, multiplying it by
PAGE_SIZE creates a massive out-of-bounds pointer calculation (for example,
a 100MB offset becomes 400GB). Since the QXL_EXECBUFFER ioctl is accessible
to any authenticated DRM client, an unprivileged user could use this path to
write a 64-bit physical address or 32-bit surface ID into kernel memory,
which can lead to system compromise or a kernel panic.

Should the underlying multiplier bug in qxl_bo_kmap_atomic_page be addressed
alongside this patch to ensure the vulnerability is fully closed?

> +			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,

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

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

[Aldo Ariel Panzardo]

qxl_alloc_release_reserved() receives a requested size from its callers
but never validates it against the actual sub-allocation slot size.
Drawable releases use 256-byte slots (RELEASE_SIZE), yet
qxl_process_single_command() allows command_size up to
PAGE_SIZE - sizeof(union qxl_release_info), approximately 4088 bytes.

The command payload is then copied from userspace via
copy_from_user_inatomic_nontemporal() into the 256-byte slot, causing a
heap buffer overflow that corrupts adjacent release slots in the same
page and can overwrite the neighbouring release_info headers.

Add a check in qxl_alloc_release_reserved() to reject allocations where
the requested size exceeds the slot size for the given release type.

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_release.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 06979d0e8..049ad167f 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;
+
 	idr_ret = qxl_release_alloc(qdev, type, release);
 	if (idr_ret < 0) {
 		if (rbo)
-- 
2.43.0

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

[sashiko-bot]

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

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

[Aldo Ariel Panzardo]

apply_reloc() and apply_surf_reloc() map a single page via
qxl_bo_kmap_atomic_page() and then write 8 or 4 bytes at the
page-relative offset (dst_offset & ~PAGE_MASK).  When the offset is
near the end of the page the write extends past the mapped region into
adjacent kernel virtual address space.

For example, a BO relocation at page offset 4092 writes bytes 4092-4099,
crossing the 4096-byte page boundary.  The fixmap slot only covers one
page, so bytes 4096-4099 corrupt whatever virtual page follows in the
kernel's fixmap area.

Reject any relocation whose page-relative offset plus write width
exceeds PAGE_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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index e727a35c9..9fba6e26d 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,
 			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 */
 		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 v2 3/4] drm/qxl: reject relocations whose writes cross a page boundary

[sashiko-bot]

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

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

[Aldo Ariel Panzardo]

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(), which computes:

  slot->high_bits | ((bo->start << PAGE_SHIFT) + offset)

generating a physical address that points outside the source BO.  The
SPICE virtual GPU then interprets this forged address, potentially
reading from arbitrary device memory.

Fix by:
  - Changing src_offset in struct qxl_reloc_info from int to uint32_t
    to prevent sign extension.
  - Validating reloc.src_offset against the source BO size before
    the assignment.

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 9fba6e26d..6559ba7d8 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -75,7 +75,7 @@ struct qxl_reloc_info {
 	struct qxl_bo *dst_bo;
 	uint32_t dst_offset;
 	struct qxl_bo *src_bo;
-	int src_offset;
+	uint32_t src_offset;
 };
 
 /*
@@ -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) {
+				ret = -EINVAL;
+				goto out_free_bos;
+			}
 			reloc_info[i].src_offset = reloc.src_offset;
 		} else {
 			reloc_info[i].src_bo = NULL;
-- 
2.43.0

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

[sashiko-bot]

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