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

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&gt; >> Assisted-by: Gemini <gemini(a)google.com&gt; # 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&gt; >> Cc: Tvrtko Ursulin <tvrtko.ursulin(a)igalia.com&gt; >> Cc: Dave Airlie <airlied(a)redhat.com&gt; >> Cc: Gerd Hoffmann <kraxel(a)redhat.com&gt; >> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com&gt; >> Cc: Maxime Ripard <mripard(a)kernel.org&gt; >> Cc: Thomas Zimmermann <tzimmermann(a)suse.de&gt; >> Cc: David Airlie <airlied(a)gmail.com&gt; >> Cc: Simona Vetter <simona(a)ffwll.ch&gt; >> 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?