Re: [PATCH v3 5/7] drm/amd/display: make shaper bypass mode cleaner

[sashiko-bot@kernel.org]

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

New issues:
- [High] Removing the `enabled` fallback leaves the shaper transfer function in a corrupted state if an invalid 1D LUT size is provided.

Pre-existing issues:
- [High] Atomic check silently ignores invalid colorop LUT sizes and allocation failures in the blend transfer function.
--

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

drm/amd/display: make shaper bypass mode cleaner

Start shaper tranfer function setup in bypass mode, i.e. tf->type ==
TF_TYPE_BYPASS and let the helper checks set it to a different mode
according to userspace request.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 13ad7ac1e35e4..4651241433296 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -1648,10 +1648,11 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>  	struct dc_transfer_func *tf = &dc_plane_state->in_shaper_func;
>  	const struct drm_color_lut32 *shaper_lut;
>  	struct drm_device *dev = colorop->dev;
> -	bool enabled = false;
>  	u32 shaper_size;
>  	int i = 0, ret = 0;
>  
> +	tf->type = TF_TYPE_BYPASS;
> +
>  	/* 1D Curve - SHAPER TF */
>  	old_colorop = colorop;
>  	for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {

[ ... ]

> @@ -1702,13 +1702,9 @@ __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
>  			ret = __set_output_tf_32(tf, shaper_lut, shaper_size, false);
>  			if (ret)
>  				return ret;
> -			enabled = true;
>  		}
>  	}
>  
> -	if (!enabled)
> -		tf->type = TF_TYPE_BYPASS;
> -
>  	return 0;
>  }

[Severity: High]
Does removing the enabled fallback leave the shaper transfer function in a
corrupted state if an invalid 1D LUT size is provided?

If userspace sets bypass = false but provides an invalid LUT size or unsets
the data blob (size = 0), the check for shaper_size == colorop->size fails
and __set_output_tf_32() is skipped in __set_dm_plane_colorop_shaper().

Since tf->type was already set to TF_TYPE_DISTRIBUTED_POINTS earlier in the
function, and we no longer fall back to TF_TYPE_BYPASS when enabled is false,
the function returns 0. Could this cause the atomic commit to succeed while
programming the hardware with an uninitialized transfer function?

[Severity: High]
This is a pre-existing issue, but a similar problem exists in
__set_dm_plane_colorop_blend().

Does the atomic check silently ignore invalid colorop LUT sizes and allocation
failures in the blend transfer function?

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c:__set_dm_plane_colorop_blend() {
	...
		/* Custom LUT size must be the same as supported size */
		if (blend_size == colorop->size)
			__set_input_tf_32(NULL, tf, blend_lut, blend_size);
	}

	return 0;
}

If the size check fails, __set_input_tf_32() is skipped entirely, but the
function returns 0. Also, the return value of __set_input_tf_32() is ignored,
meaning allocation failures are not propagated. Could this allow the hardware
to be programmed with an uninitialized or partially initialized transfer
function?

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