Re: [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop

[sashiko-bot@kernel.org]

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

New issues:
- [High] Page flips that do not modify the color pipeline incorrectly bypass the YUV-to-RGB conversion matrix.
- [Medium] vkms_plane_atomic_check fails to validate the bypass state for the fixed matrix colorop.
- [Medium] FIXED_MATRIX colorop is silently ignored for RGB formats, and state is unconditionally interpreted as fixed_matrix.

Pre-existing issues:
- [High] The background composer accesses live DRM object state (`colorop->state`), risking Use-After-Free.
--

--- Patch [5]: [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop ---
commit 7c8489284a67e8c66fd1ead26966ba84d8380366
Author: Harry Wentland <harry.wentland@amd.com>

drm/vkms: Add atomic check and matrix handling for fixed matrix colorop

This commit validates that YUV framebuffers use a non-bypassed fixed matrix
colorop when a color pipeline is active. It also maps the fixed matrix type
to color encoding and range for YUV-to-RGB conversion, while treating the
fixed matrix colorop as a no-op during composition.

> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..899120cd07ac5 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -187,6 +187,12 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
>  		if (colorop_state->data)
>  			apply_3x4_matrix(pixel,
>  					 (struct drm_color_ctm_3x4 *)colorop_state->data->data);
> +	} else if (colorop->type == DRM_COLOROP_FIXED_MATRIX) {
> +		/*
> +		 * Fixed Matrix is a no-op here. YUV-to-RGB conversion
> +		 * is handled at format read time via the
> +		 * conversion_matrix set in vkms_plane_atomic_update().
> +		 */
>  	}

[Severity: Medium]
If userspace applies a DRM_COLOROP_FIXED_MATRIX to an RGB plane, will this
silently ignore the operation instead of properly rejecting it in the atomic
check?

[Severity: High]
This isn't a bug introduced by this patch, but doesn't reading the live
colorop->state pointer asynchronously in the worker risk a use-after-free
if a concurrent atomic commit frees the old state? 

Looking at the beginning of apply_colorop() in vkms_composer.c:

static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colorop)
{
	struct drm_colorop_state *colorop_state = colorop->state;

Should VKMS snapshot the required properties into vkms_plane_state during
atomic_update instead of dereferencing the live DRM object state asynchronously?

[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 68cb2a3335e64..6ee5c3f3207c5 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
[ ... ]
> @@ -148,7 +152,49 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  	frame_info->rotation = new_state->rotation;
>  
>  	vkms_plane_state->pixel_read_line = get_pixel_read_line_function(fmt);
> -	get_conversion_matrix_to_argb_u16(fmt, new_state->color_encoding, new_state->color_range,
> +
> +	if (new_state->color_pipeline) {
> +		struct drm_colorop *colorop = new_state->color_pipeline;
> +		struct drm_colorop_state *colorop_state;
> +
> +		colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
> +		bypass = !colorop_state || colorop_state->bypass;

[Severity: High]
Could this logic inadvertently break YUV decoding on standard page flips?

If a page flip does not include the colorop in the atomic commit,
drm_atomic_get_new_colorop_state() returns NULL, which forces bypass to true.
Will this reset the conversion matrix to a no-op on the second frame?

> +
> +		if (!bypass) {
> +			switch (colorop_state->fixed_matrix_type) {

[Severity: Medium]
When the active colorop on an RGB plane is a 1D_CURVE instead of a
FIXED_MATRIX, will this unconditionally misinterpret the state struct and read
an invalid fixed_matrix_type?

> +			case DRM_COLOROP_FM_YCBCR601_FULL_RGB:
> +				encoding = DRM_COLOR_YCBCR_BT601;
> +				range = DRM_COLOR_YCBCR_FULL_RANGE;
> +				break;
[ ... ]
> @@ -175,6 +221,13 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
>  	if (ret != 0)
>  		return ret;
>  
> +	if (new_plane_state->color_pipeline && new_plane_state->fb->format->is_yuv) {
> +		struct drm_colorop *colorop = new_plane_state->color_pipeline;
> +
> +		if (colorop->type != DRM_COLOROP_FIXED_MATRIX)
> +			return -EINVAL;

[Severity: Medium]
The commit message states that "The fixed matrix colorop must not be bypassed"
when the framebuffer format is YUV.

Does this atomic check fail to enforce that rule by not retrieving the
colorop state to check its bypass property?

> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=5