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) <jirislaby(a)kernel.org>
>> Assisted-by: Gemini <gemini(a)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(a)amd.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com>
>> Cc: Dave Airlie <airlied(a)redhat.com>
>> Cc: Gerd Hoffmann <kraxel(a)redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com>
>> Cc: Maxime Ripard <mripard(a)kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann(a)suse.de>
>> Cc: David Airlie <airlied(a)gmail.com>
>> Cc: Simona Vetter <simona(a)ffwll.ch>
>> Cc: stable(a)vger.kernel.org
>> ---
>> Cc: virtualization(a)lists.linux.dev
>> Cc: spice-devel(a)lists.freedesktop.org
>> Cc: dri-devel(a)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);
>
> 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