Re: [PATCH 1/2] drm/vkms: Cache composer line buffers in vkms_output

[sashiko-bot@kernel.org]

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

New issues:
- [High] The device teardown path frees the composer line buffers before the composer workqueue is flushed, leading to a Use-After-Free (UAF).
- [High] A double-free vulnerability exists if the allocation for the stage buffer fails during reallocation, leaving a dangling pointer.

Pre-existing issues:
- [High] Preexisting Use-After-Free of `vkms_crtc_state` when destroying the CRTC state while a composer worker is pending.
--

--- Patch [1]: [PATCH 1/2] drm/vkms: Cache composer line buffers in vkms_output ---
commit 169a243140521e33c116ce0ae779021e7e4e4b76
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Cache composer line buffers in vkms_output

compose_active_planes() kvmalloc/kvfree two line buffers every vblank
frame, but their size depends only on hdisplay which changes only on
modeset.  Cache them in vkms_output and reallocate only when the width
changes, avoiding repeated alloc/free overhead.

Register a drmm action to free the buffers when the device is released.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..f3fb203946e83 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
[ ... ]
> @@ -565,27 +565,35 @@ static int compose_active_planes(struct vkms_writeback_job *active_wb,
>  	stage_buffer.n_pixels = line_width;
>  	output_buffer.n_pixels = line_width;
>  
> -	stage_buffer.pixels = kvmalloc(line_width * pixel_size, GFP_KERNEL);
> -	if (!stage_buffer.pixels) {
> -		DRM_ERROR("Cannot allocate memory for the output line buffer");
> -		return -ENOMEM;
> -	}
> +	if (out->composer_buffer_width != line_width) {
> +		kvfree(out->composer_stage_buffer);
> +		kvfree(out->composer_output_buffer);
> +		out->composer_buffer_width = 0;
> +
> +		out->composer_stage_buffer = kvmalloc(line_width * pixel_size, GFP_KERNEL);
> +		if (!out->composer_stage_buffer) {
> +			DRM_ERROR("Cannot allocate memory for the output line buffer");
> +			return -ENOMEM;
> +		}

[Severity: High]
If this kvmalloc() fails, does it leave out->composer_output_buffer pointing
to freed memory?

When the line width changes, both buffers are freed. If the allocation for the
stage buffer fails, the function returns -ENOMEM without clearing
out->composer_output_buffer.

Later, during device teardown, vkms_composer_buffers_release() will call
kvfree() on out->composer_output_buffer again. Could this lead to a
double-free?

> +
> +		out->composer_output_buffer = kvmalloc(line_width * pixel_size, GFP_KERNEL);
> +		if (!out->composer_output_buffer) {
> +			DRM_ERROR("Cannot allocate memory for intermediate line buffer");
> +			kvfree(out->composer_stage_buffer);
> +			out->composer_stage_buffer = NULL;
> +			return -ENOMEM;
> +		}
[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 079abfba427d0..6e03c9a1fbd82 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
[ ... ]
> @@ -237,5 +245,9 @@ struct vkms_output *vkms_crtc_init(struct drm_device *dev, struct drm_plane *pri
>  	if (IS_ERR(vkms_out->composer_workq))
>  		return ERR_CAST(vkms_out->composer_workq);
>  
> +	ret = drmm_add_action_or_reset(dev, vkms_composer_buffers_release, vkms_out);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	return vkms_out;
>  }

[Severity: High]
Does this setup create a use-after-free during device teardown?

The DRM managed resource API executes release actions in LIFO order. Here, the
workqueue is allocated first (which registers its destruction/flush action),
and then the buffer release action is registered.

During teardown, the buffers will be freed first. Then the workqueue is
destroyed, which flushes any pending vkms_composer_worker items. If a worker
is pending and executes compose_active_planes(), won't it access the already
freed buffer memory?


[Severity: High]
This is a pre-existing issue, but I noticed another potential use-after-free
in vkms_atomic_crtc_destroy_state().

When the CRTC state is destroyed, it issues a warning if the composer work is
pending:

    WARN_ON(work_pending(&vkms_state->composer_work));
    kfree(vkms_state->active_planes);
    kfree(vkms_state);

Since it doesn't synchronize or cancel the work (e.g., using
cancel_work_sync()), if the worker executes after this point, won't it
access the freed vkms_state and its planes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820083123.544655-1-oushixiong1025@163.com?part=1