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

[sashiko-bot@kernel.org]

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