Re: [v5 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:
- [High] Missing backend implementation for exposed `INTEL_PLANE_CB_CSC_FF` colorop.
- [Medium] Unreachable SDR plane color pipeline logic.
- [Medium] Validation bypass for 64-bit bitmasks on 32-bit architectures.

Pre-existing issues:
- [High] NULL pointer dereference in error cleanup path of `intel_color_pipeline_plane_add_colorop`.
- [High] Leaked DRM mode object ID on colorop cleanup leads to a Use-After-Free.
--

--- Patch [2]: [v5 02/14] drm/i915/color: Add CSC on SDR plane color pipeline ---
commit 406763c8b5fb35d6a8f9c41091f76f4b8453fc5a
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]
Can this bypass validation for invalid upper bits on 32-bit architectures?

When calling drm_plane_colorop_fixed_matrix_init() and passing the 64-bit mask
intel_plane_supported_csc_ff, the validation check uses the BIT() macro:

drivers/gpu/drm/drm_colorop.c:drm_plane_colorop_fixed_matrix_init() {
	...
	if ((supported_fm & -BIT(DRM_COLOROP_FM_COUNT)) != 0) {
	...
}

Since BIT() returns an unsigned long, which is 32 bits on 32-bit
architectures, negating it produces a 32-bit mask (0xFFFFFFFx) that is
zero-extended when ANDed with the 64-bit supported_fm mask. Does this silently
ignore bits 32-63 during validation?

[Severity: High]
Does the i915 driver have the backend implementation to process this newly
exposed fixed-matrix colorop?

The INTEL_PLANE_CB_CSC_FF colorop exposes the fixed_matrix_type property to
userspace. However, looking at the parsing logic:

drivers/gpu/drm/i915/display/intel_plane.c:
intel_plane_color_copy_uapi_to_hw_state() {
	...
	blob = new_colorop_state->bypass ? NULL : new_colorop_state->data;
	intel_colorop = to_intel_colorop(colorop);
	changed |= intel_plane_colorop_replace_blob(plane_state,
						    intel_colorop,
						    blob);
	...
}

This appears to only handle blobs (LUTs, CTMs) and completely ignores the
fixed_matrix_type state. Could this cause the userspace configuration to be
silently discarded and never programmed to the hardware?

[Severity: High]
This isn't a bug introduced by this patch, but if
drm_plane_colorop_fixed_matrix_init() fails, will the cleanup path trigger
a NULL pointer dereference?

If initialization fails, it jumps to the cleanup label:

drivers/gpu/drm/i915/display/intel_color_pipeline.c:
intel_color_pipeline_plane_add_colorop() {
	...
cleanup:
	intel_colorop_destroy(&colorop->base);
	return ERR_PTR(ret);
}

Which then calls drm_colorop_cleanup():

drivers/gpu/drm/drm_colorop.c:drm_colorop_cleanup() {
	struct drm_device *dev = colorop->dev;
	struct drm_mode_config *config = &dev->mode_config;

	list_del(&colorop->head);
	...
}

If the failure happens early in initialization before colorop->dev or
colorop->head are set, dev will be NULL and this will crash.

[Severity: High]
This is a pre-existing issue, but does drm_colorop_cleanup() leak the DRM
mode object ID?

When drm_plane_colorop_init() executes successfully, it allocates an ID in the
DRM device's IDR by calling drm_mode_object_add(). However,
drm_colorop_cleanup() appears to be missing a call to
drm_mode_object_unregister(dev, &colorop->base).

Does this mean the ID remains allocated forever, potentially leading to a
use-after-free if userspace later queries the leaked ID after the memory is
freed?

>  	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 else branch ever be executed for SDR planes?

Looking at the caller, intel_color_pipeline_plane_init(), there is an explicit
early return for non-HDR planes:

drivers/gpu/drm/i915/display/intel_color_pipeline.c:
intel_color_pipeline_plane_init() {
	...
	/* Currently expose pipeline only for HDR planes */
	if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id))
		return 0;
	...
}

It seems this prevents the newly added SDR logic from ever being reached.

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