[PATCH v2] drm/qxl: fix use-after-free and NULL pointer deref

[PATCH v2] drm/qxl: fix use-after-free and NULL pointer deref

[Jiri Slaby (SUSE)]

When allocating a `qxl_release` structure with `kmalloc()`, the underlying
memory contained uninitialized garbage. Specifically, `release->base.flags`
(part of the embedded `dma_fence`) was not cleared.

This garbage in `base.flags` caused helper functions such as
`dma_fence_was_initialized()` to return true even for releases where the
fence was never actually initialized (e.g. via `dma_fence_init()`).

Consequently, during release cleanup in `qxl_release_free()`, the driver
attempted to put/free an uninitialized `dma_fence`, leading to refcount
underflows (`refcount_t: underflow; use-after-free`) and subsequent NULL
pointer dereferences in `dma_fence_signal_timestamp_locked()`.

Fix this by switching from `kmalloc()` to `kzalloc_obj()` in
`qxl_release_alloc()`, ensuring all fields (including embedded fence
flags) are properly zero-initialized upon allocation, and remove
redundant explicit zero-initializations.

The dumps in question:
 refcount_t: underflow; use-after-free.
 WARNING: lib/refcount.c:28 at refcount_warn_saturate+0x59/0x90, CPU#0: kworker/0:0/1534
 Modules linked in: af_packet nft_fib_inet ...
 CPU: 0 UID: 0 PID: 1534 Comm: kworker/0:0 Not tainted 7.1.3-1-default #1 PREEMPT(full) openSUSE Tumbleweed  b041a6527f6e58424f4cd3de0fade8d408b378fd
 ...
 RIP: 0010:refcount_warn_saturate+0x59/0x90
 ...
 Call Trace:
  <TASK>
  qxl_release_free+0xee/0xf0 [qxl d93e9381353e619799d56790f5f8dda6cce491f6]
  qxl_garbage_collect+0xd1/0x1b0 [qxl d93e9381353e619799d56790f5f8dda6cce491f6]
  process_one_work+0x19e/0x3a0
 ...

And then of course:
 BUG: kernel NULL pointer dereference, address: 0000000000000028
 ...
 RIP: 0010:dma_fence_signal_timestamp_locked+0x32/0x120

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Assisted-by: Gemini <gemini@google.com> # only commit log
Fixes: 2bcbc706dfa0 ("dma-buf: add dma_fence_was_initialized function v2")
Closes: https://bugzilla.suse.com/show_bug.cgi?id=1271081
Cc: Christian König <christian.koenig@amd.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: stable@vger.kernel.org
---
Cc: virtualization@lists.linux.dev
Cc: spice-devel@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org

[v2] use kzalloc_obj() instead of bare kzalloc()
---
 drivers/gpu/drm/qxl/qxl_release.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 06979d0e8a9f..07dc6eafe6f7 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -89,17 +89,13 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
 {
 	struct qxl_release *release;
 	int handle;
-	size_t size = sizeof(*release);
 
-	release = kmalloc(size, GFP_KERNEL);
+	release = kzalloc_obj(*release);
 	if (!release) {
 		DRM_ERROR("Out of memory\n");
 		return -ENOMEM;
 	}
-	release->base.ops = NULL;
 	release->type = type;
-	release->release_offset = 0;
-	release->surface_release_id = 0;
 	INIT_LIST_HEAD(&release->bos);
 
 	idr_preload(GFP_KERNEL);
-- 
2.55.0

Re: [PATCH v2] drm/qxl: fix use-after-free and NULL pointer deref

[sashiko-bot]

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

Pre-existing issues:
- [High] Ignoring the return value of `qxl_release_list_add()` can lead to a use-after-free of a buffer object by the hardware.
- [High] Sleeping in atomic context during plane updates.
--

commit f18d8a877369f4db994d43ef4a35d9b13d174685
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

drm/qxl: fix use-after-free and NULL pointer deref

When allocating a qxl_release structure with kmalloc(), the underlying
memory contained uninitialized garbage. Specifically, release->base.flags
(part of the embedded dma_fence) was not cleared.

Closes: https://bugzilla.suse.com/show_bug.cgi?id=1271081

> diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
> index 06979d0e8a9f0..07dc6eafe6f7f 100644
> --- a/drivers/gpu/drm/qxl/qxl_release.c
> +++ b/drivers/gpu/drm/qxl/qxl_release.c
> @@ -89,17 +89,13 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
>  {
>  	struct qxl_release *release;
>  	int handle;
>  
> -	release = kmalloc(size, GFP_KERNEL);
> +	release = kzalloc_obj(*release);
>  	if (!release) {
>  		DRM_ERROR("Out of memory\n");
>  		return -ENOMEM;
>  	}
> -	release->base.ops = NULL;
>  	release->type = type;
> -	release->release_offset = 0;
> -	release->surface_release_id = 0;
>  	INIT_LIST_HEAD(&release->bos);
>  
>  	idr_preload(GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but does this code sleep in an
atomic context?

Looking at the plane update paths, qxl_primary_atomic_update() is an atomic
commit callback where sleeping is forbidden. It appears to call
qxl_release_alloc() through this chain:

qxl_primary_atomic_update()
  qxl_draw_dirty_fb()
    alloc_drawable()
      qxl_alloc_release_reserved()
        qxl_release_alloc()

Can kzalloc_obj() and idr_preload(GFP_KERNEL) sleep here? Additionally,
qxl_alloc_release_reserved() acquires a mutex before calling this function,
which could also cause deadlocks in an atomic context.


[Severity: High]
This is also a pre-existing issue, but can ignoring the return value of
qxl_release_list_add() lead to a hardware use-after-free?

In drivers/gpu/drm/qxl/qxl_release.c:qxl_alloc_surface_release_reserved():

    qxl_release_list_add(*release, bo);

If qxl_release_list_add() fails (e.g. returning -ENOMEM), the BO is not
added to the list and its reference count is not incremented. However, the
function ignores this failure and continues, returning success. When the
hardware ring processes the command referencing this BO, could the BO memory
be freed prematurely if the original reference is dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828085136.128561-1-jirislaby@kernel.org?part=1