Re: [CI v4 02/14] drm/i915/color: Add CSC on SDR plane color pipeline

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The newly added SDR plane color pipeline logic is completely unreachable due to an early return for non-HDR planes in the caller function.
- [Medium] The driver exposes the `CSC_FF` colorop but fails to extract and apply its atomic state, violating the DRM API contract.

Pre-existing issues:
- [Critical] `drm_colorop_cleanup()` fails to unregister the DRM mode object, leading to a Use-After-Free via stale object IDs.
- [High] `drm_colorop_cleanup` assumes `colorop->dev` and `colorop->head` are fully initialized, causing a NULL pointer dereference and crash on early initialization failures.
- [High] Unchecked return value from `kasprintf()` leads to a NULL pointer dereference during DRM property creation.
--

--- Patch [2]: [CI v4 02/14] drm/i915/color: Add CSC on SDR plane color pipeline ---
commit ed94f2ef5daa2b6a1fe43a799710d164ac1323d4
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

drm/i915/color: Add CSC on SDR plane color pipeline

Add the fixed-function CSC block to color pipeline in SDR planes
as a DRM_COLOROP_FIXED_MATRIX colorop.

> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> index 6cf8080ee8000..2ef42a133a987 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c

[ ... ]

> @@ -92,6 +102,12 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
>  							  DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
>  							  DRM_COLOROP_FLAG_ALLOW_BYPASS);
>  		break;
> +	case INTEL_PLANE_CB_CSC_FF:
> +		ret = drm_plane_colorop_fixed_matrix_init(dev, &colorop->base, plane,
> +							  &intel_colorop_funcs,
> +							  intel_plane_supported_csc_ff,
> +							  DRM_COLOROP_FLAG_ALLOW_BYPASS);
> +		break;

[Severity: Medium]
Will intel_plane_color_copy_uapi_to_hw_state() successfully extract the
state for INTEL_PLANE_CB_CSC_FF?

Looking at intel_plane_colorop_replace_blob() in
drivers/gpu/drm/i915/display/intel_plane.c, it handles blob-based colorops
like INTEL_PLANE_CB_CSC, but lacks a case for INTEL_PLANE_CB_CSC_FF.

Could this cause userspace settings for fixed_matrix_type and bypass on this
colorop to be silently ignored when applying the pipeline?

[Severity: High]
This is a pre-existing issue, but if drm_plane_colorop_fixed_matrix_init()
or other initialization functions fail early (e.g. due to OOM), the code
jumps to the cleanup label in this function and calls intel_colorop_destroy().

This calls drm_colorop_cleanup(), which unconditionally dereferences
colorop->dev and colorop->head via list_del().

Since these fields might not be initialized on early failure, is it possible
for this to cause a NULL pointer dereference?

[Severity: Critical]
This is another pre-existing issue on the cleanup path. When
drm_colorop_cleanup() runs, it never calls drm_mode_object_unregister().

Because the object was registered in drm_mode_object_add() during init,
does omitting drm_mode_object_unregister() leave a dangling pointer in the
device's IDR, allowing userspace to trigger a use-after-free by querying
the freed object ID?

>  	default:
>  		drm_err(plane->dev, "Invalid colorop id [%d]", id);
>  		ret = -EINVAL;

[ ... ]

> @@ -126,9 +142,12 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
>  	if (plane_has_3dlut(display, pipe, plane)) {
>  		pipeline = xe3plpd_primary_plane_pipeline;
>  		pipeline_len = ARRAY_SIZE(xe3plpd_primary_plane_pipeline);
> -	} else {
> +	} else if (icl_is_hdr_plane(display, to_intel_plane(plane)->id)) {
>  		pipeline = hdr_plane_pipeline;
>  		pipeline_len = ARRAY_SIZE(hdr_plane_pipeline);
> +	} else {
> +		pipeline = sdr_plane_pipeline;
> +		pipeline_len = ARRAY_SIZE(sdr_plane_pipeline);
>  	}

[Severity: Medium]
Will this newly added sdr_plane_pipeline ever execute?

The caller of this function, intel_color_pipeline_plane_init(), has an early
return:

intel_color_pipeline_plane_init()
    if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id))
        return 0;
        
    ret = _intel_color_pipeline_plane_init(plane, &pipelines[len], pipe);

Because it skips all non-HDR planes before calling this function, doesn't
that make this else branch for SDR planes completely unreachable?

[Severity: High]
This isn't a bug introduced by this patch, but immediately below this block
in _intel_color_pipeline_plane_init(), kasprintf() is called:

    list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ...);

The return value is not checked for NULL.

If kasprintf() fails, it passes the NULL pointer to
drm_plane_create_color_pipeline_property(), which passes it to
drm_property_create_enum().

Will this cause a panic when strlen(name) is called on the NULL pointer
inside drm_property_add_enum()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713051516.4087420-1-chaitanya.kumar.borah@intel.com?part=2