Re: [PATCH v3 2/7] drm/atomic: reject colorop update from inactive color pipeline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm_atomic_colorop_check() allows modifying colorops without adding the parent plane to the atomic commit, bypassing hardware updates.
--

commit c38eca82c086d2e818d8283fed72b37dc3abbab9
Author: Melissa Wen <mwen@igalia.com>

drm/atomic: reject colorop update from inactive color pipeline

This commit rejects updates on colorops that are not part of an active
pipeline. It adds drm_atomic_colorop_check() to verify the color pipeline
of a plane in its current, new or old state.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 0d36a955c4d43..ec3c6e4e5a062 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[ ... ]

> @@ -863,6 +863,55 @@ drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
>  	return 0;
>  }
>  
> +/**
> + * drm_atomic_colorop_check - check new colorop state
> + * @new_colorop_state: new colorop state to check
> + *
> + * Ensure that the colorop in @new_colorop_state belongs to an active color
> + * pipeline, i.e. it's in the chain of colorops set to the color_pipeline
> + * property of current, old or new plane state.
> + *
> + * Returns: 0 on success, -EINVAL otherwise.
> + */
> +static int drm_atomic_colorop_check(const struct drm_colorop_state *new_colorop_state)
> +{
> +	struct drm_atomic_commit *state = new_colorop_state->state;
> +	struct drm_plane *plane = new_colorop_state->colorop->plane;
> +	struct drm_plane_state *new_plane_state, *old_plane_state;
> +	struct drm_colorop *colorop;
> +
> +	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +
> +	/* No changes in the plane state. Check current-committed plane state */
> +	if (!new_plane_state) {
> +		for (colorop = plane->state->color_pipeline; colorop; colorop = colorop->next)
> +			if (colorop == new_colorop_state->colorop)
> +				return 0;

[Severity: High]
If a colorop property is updated but the parent plane isn't explicitly
modified, new_plane_state will be NULL and this returns 0.

Since drm_colorop does not have its own atomic_update callback, hardware
programming relies on the plane's commit paths iterating over planes in
the atomic state.

If the plane is missing from the atomic state, will the hardware ever be
reprogrammed with the new colorop parameters, or does this result in a
silent failure where only the software state is updated?

Should the affected plane be actively pulled into the atomic state
(e.g., via drm_atomic_get_plane_state()) whenever a colorop is modified?

> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(!old_plane_state))
> +		return -EINVAL;
> +
> +	/* Check if the colorop is active in the new plane state */
> +	for (colorop = new_plane_state->color_pipeline; colorop; colorop = colorop->next)
> +		if (colorop == new_colorop_state->colorop)
> +			return 0;
> +
> +	/* Same color pipeline as new; no point walking old. Colorop isn't active */
> +	if (new_plane_state->color_pipeline == old_plane_state->color_pipeline)
> +		return -EINVAL;
> +
> +	/* Check if the colorop was active in the old plane state */
> +	for (colorop = old_plane_state->color_pipeline; colorop; colorop = colorop->next)
> +		if (colorop == new_colorop_state->colorop)
> +			return 0;
> +
> +	/* Colorop is not part of an active color pipeline. */
> +	return -EINVAL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121230.1358786-1-mwen@igalia.com?part=2