Re: [PATCH] drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] VKMS composer accesses global colorop->state directly without shadowing it during atomic commit, leading to a data race and potential Use-After-Free.
--

commit 7feb24a5b3e1c9cb2aa23694b44e46aece887a34
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Skip pre_blend_color_transform when pipeline is all bypassed

This patch adds an optimization to pre_blend_color_transform() to skip
the entire pixel processing loop when all colorops in the pipeline are
bypassed or the pipeline is empty.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..18f58569f0169 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -190,13 +190,29 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
>  	}
>  }
>  
> +static bool pipeline_all_bypassed(struct drm_colorop *colorop)
> +{
> +	while (colorop) {
> +		struct drm_colorop_state *colorop_state = colorop->state;

[Severity: High]
This is a pre-existing issue, but does this direct access to colorop->state
introduce a potential data race and use-after-free?

The VKMS composer processes frames asynchronously in a workqueue
(vkms_composer_worker). While plane states are properly duplicated and
shadowed, reading the global colorop->state pointer directly here in
pipeline_all_bypassed() within drivers/gpu/drm/vkms/vkms_composer.c seems
unsafe.

If a non-blocking atomic commit swaps the state via
drm_atomic_helper_swap_state() and frees the old state via
drm_atomic_state_put() while the composer worker is preempted, will the
worker dereference a freed state pointer when it resumes? Local userspace
with DRM master privileges could potentially trigger this by submitting rapid
non-blocking atomic commits that modify the color pipeline while the virtual
hardware is compositing frames.

This same unsafe access pattern already exists in the inner loop of
pre_blend_color_transform(), but this patch expands the pattern into the new
pipeline_all_bypassed() function. Should the colorop state be properly
shadowed for the atomic commit instead of being read globally?

> +
> +		if (!colorop_state || !colorop_state->bypass)
> +			return false;
> +		colorop = colorop->next;
> +	}
> +	return true;
> +}

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