Re: [v6 2/9] drm/i915/color: Add CSC on SDR plane color pipeline

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

New issues:
- [Medium] The newly added SDR color pipeline is entirely unreachable because its initialization is blocked by an early return guard for non-HDR planes.
- [Low] Validating a 64-bit mask (`supported_fm`) using `-BIT()` causes integer truncation on 32-bit platforms, failing to check the upper 32 bits of the bitmask.

Pre-existing issues:
- [High] Error handling in `intel_color_pipeline_plane_add_colorop()` triggers multiple NULL pointer dereferences when tearing down partially initialized colorops.
- [High] Missing `drm_mode_object_unregister()` in `drm_colorop_cleanup()` causes IDR leaks and potential Use-After-Free of drm_colorop objects.
--

--- Patch [2]: [v6 2/9] drm/i915/color: Add CSC on SDR plane color pipeline ---
commit 8f7cf1094859b74b7d23e60a46f2012f047db34f
Author: Chaitanya Kumar Borah <[email protected]>
Subject: drm/i915/color: Add CSC on SDR plane color pipeline

This commit adds the fixed-function CSC block to the 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
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c

[ ... ]

> @@ -92,6 +104,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: Low]
Does this call expose a truncation bug on 32-bit platforms?

Looking at drm_plane_colorop_fixed_matrix_init() in drm_colorop.c:

drm_colorop.c:drm_plane_colorop_fixed_matrix_init() {
    ...
    if ((supported_fm & -BIT(DRM_COLOROP_FM_COUNT)) != 0) {
        drm_err(dev, "Unknown Fixed Matrix provided...\n");
        return -EINVAL;
    }
    ...
}

Since BIT() uses UL, on 32-bit systems -BIT() evaluates as an unsigned
32-bit value. When promoted to 64-bit to evaluate against supported_fm,
it zero-extends. This means the upper 32 bits are not checked properly,
which could silently allow invalid caller configurations where bits >= 32
are set.

[Severity: High]
This is a pre-existing issue, but does this error path cause a NULL pointer
dereference?

If drm_plane_colorop_fixed_matrix_init() fails and returns an error before
setting colorop->dev, we jump to the cleanup label in
intel_color_pipeline_plane_add_colorop():

intel_color_pipeline.c:intel_color_pipeline_plane_add_colorop() {
    ...
cleanup:
    intel_colorop_destroy(&colorop->base);
    return ERR_PTR(ret);
}

This calls drm_colorop_cleanup(), which unconditionally dereferences
colorop->dev:

drm_colorop.c:drm_colorop_cleanup() {
    struct drm_device *dev = colorop->dev;
    struct drm_mode_config *config = &dev->mode_config;
    
    list_del(&colorop->head);
    config->num_colorop--;
    ...
}

Since dev is not set yet, dev->mode_config will crash, and list_del() will
operate on a zero-initialized list head.

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

If a colorop init function successfully adds the mode object to the IDR via
drm_mode_object_add() but fails later, it calls drm_colorop_cleanup().

Since drm_colorop_cleanup() does not call drm_mode_object_unregister(), the
object ID remains in the DRM device's IDR. When the colorop memory is freed,
this leaves a dangling pointer in the IDR, which could lead to a Use-After-Free
if userspace queries it via DRM_IOCTL_MODE_OBJ_GETPROPERTIES.

[ ... ]

> @@ -126,9 +144,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_yuv_pipeline;
> +		pipeline_len = ARRAY_SIZE(sdr_plane_yuv_pipeline);
>  	}

[Severity: Medium]
Is this newly added SDR pipeline logic reachable?

Any initialization of an SDR plane calls intel_color_pipeline_plane_init(),
which appears to have an early return guard for non-HDR planes before it
calls _intel_color_pipeline_plane_init():

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;

    return _intel_color_pipeline_plane_init(plane, list, pipe);
}

Because of this check, _intel_color_pipeline_plane_init() will never be
reached for an SDR plane, making the new sdr_plane_yuv_pipeline dead code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2