On 02/09/2026 11:45, Jiri Slaby wrote:
On 02. 09. 26, 11:53, Tvrtko Ursulin wrote:
On 28/08/2026 09:51, Jiri Slaby (SUSE) wrote:
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) <[email protected]>
Assisted-by: Gemini <[email protected]> # 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 <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: Dave Airlie <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Simona Vetter <[email protected]>
Cc: [email protected]
---
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
[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);
Looks plausible on a superficial look, albeit fragile. I am not sure
why qxl_release_alloc wasn't calling dma_fence_init in the first place?
If you did, you could not test the ops (previously) or
dma_fence_was_initialized() now, right?
Right, but on a superficial look what would be lost if that wasn't done, ie:
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 06979d0e8a9f..ea1e0b4f6e5b 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -147,16 +147,11 @@ qxl_release_free(struct qxl_device *qdev,
idr_remove(&qdev->release_idr, release->id);
spin_unlock(&qdev->release_idr_lock);
- if (dma_fence_was_initialized(&release->base)) {
- WARN_ON(list_empty(&release->bos));
- qxl_release_free_list(release);
+ qxl_release_free_list(release);
+
+ dma_fence_signal(&release->base);
+ dma_fence_put(&release->base);
- dma_fence_signal(&release->base);
- dma_fence_put(&release->base);
- } else {
- qxl_release_free_list(release);
- kfree(release);
- }
atomic_dec(&qdev->release_count);
}
WARN_ON is lost but on balance how much does that matter? Or could it be
moved somewhere else?
I don't know this driver to be clear but was just curious to understand
if there is an alternative. As said, the patch as is looks okay to me
looking from the outside.
Regards,
Tvrtko
