Re: [RFC PATCH 7/8] drm/colorop: Convert pipline 'next' pointer to a list

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional dereference of potentially NULL color pipeline in VKMS pre_blend_color_transform.
- [Medium] Fragile global state tracking in `drm_atomic_colorop_print_state` leads to misaligned pointer dereferences.

Pre-existing issues:
- [Critical] Data race and Use-After-Free due to asynchronous access of global `colorop->state` in VKMS composer.
--

commit bddf98b16d8f020be8f44f0de1ce3cd9f1f44d2d
Author: John Harrison <John.Harrison@Igalia.com>

drm/colorop: Convert pipline 'next' pointer to a list
    
The colorop pipeline system was using explicit next pointers and open
coded lists rather than the official kernel list helpers. Fix that.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 170de30c28ae4..8e985fd0562be 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
[ ... ]
> @@ -849,7 +853,12 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
>  		break;
>  	}
>  
> -	drm_printf_indent(p, 1, "next=%d\n", colorop->next ? colorop->next->base.id : 0);
> +	if (list_is_last(&colorop->pipeline_list, &(*pipeline)->pipeline_head)) {
> +		*pipeline = NULL;
> +		return;
> +	}
> +
> +	drm_printf_indent(p, 1, "next=%d\n", list_next_entry(colorop, pipeline_list)->base.id);
>  }

[Severity: Medium]
What happens if an orphaned colorop (created but not added to any pipeline)
exists in the global config->colorop_list?

It appears the pipeline pointer would be set to this orphan, but list_is_last
will never evaluate to true since its pipeline_head is empty. Won't the
pipeline tracking get permanently stuck, causing a subsequent valid pipeline's
last node to return false here?

This looks like it would evaluate list_next_entry which computes container_of
on pipeline_head at a different offset, leading to a misaligned pointer
dereference.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..fc20b788738a3 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -194,9 +194,12 @@ static void pre_blend_color_transform(const struct vkms_plane_state *plane_state
>  				      struct line_buffer *output_buffer)
>  {
>  	struct pixel_argb_s32 pixel;
> +	struct drm_colorop *pipeline = plane_state->base.base.color_pipeline;
> +
> +	WARN_ON(!list_is_first(&pipeline->pipeline_list, &pipeline->pipeline_head));

[Severity: High]
Could this cause a regression if userspace doesn't configure a color pipeline
and leaves the property unset?

The previous code handled a missing pipeline safely, but this looks like it
will unconditionally dereference the pipeline pointer inside the list_is_first
helper before checking if it actually exists. Does this need an early return
or a valid pipeline check first?

>  
>  	for (size_t x = 0; x < output_buffer->n_pixels; x++) {
> -		struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
> +		struct drm_colorop *colorop;
>  
>  		/*
[ ... ]
> @@ -213,7 +216,7 @@ static void pre_blend_color_transform(const struct vkms_plane_state *plane_state
>  		pixel.g = output_buffer->pixels[x].g;
>  		pixel.b = output_buffer->pixels[x].b;
>  
> -		while (colorop) {
> +		list_for_each_entry(colorop, &pipeline->pipeline_head, pipeline_list) {
>  			struct drm_colorop_state *colorop_state;
>  
>  			colorop_state = colorop->state;

[Severity: Critical]
This is a pre-existing issue, but can this direct access to colorop->state
race with concurrent atomic commits?

Since vkms_composer_worker runs asynchronously, couldn't a modeset dynamically
free this state via drm_atomic_helper_commit_cleanup_done while we're reading
it, resulting in a use-after-free regression?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609171941.169104-1-John.Harrison@Igalia.com?part=7