Re: [v5 14/14] drm/i915/color: Add color pipeline support for SDR planes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A missing NULL check for the `kasprintf()` allocation in `_intel_color_pipeline_plane_init()` causes a NULL pointer dereference in `drm_property_add_enum()`.
- [High] Early initialization failures during colorop creation cause `drm_colorop_cleanup()` to access uninitialized structure members, leading to a crash.
- [High] Missing `drm_mode_object_unregister()` in `drm_colorop_cleanup()` leaves destroyed colorops in the DRM object IDR, leading to a Use-After-Free.
--

--- Patch [14]: [v5 14/14] drm/i915/color: Add color pipeline support for SDR planes ---
commit 898c62da94ca7a00530f154f1e3d4b18c45aaa47
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

drm/i915/color: Add color pipeline support for SDR planes

This commit exposes the SDR plane color pipeline to user-space by removing
the check that previously limited it to HDR planes.

> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> index 136b9378ca484..a99b6eb204488 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> @@ -188,17 +188,11 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
>  
>  int intel_color_pipeline_plane_init(struct drm_plane *plane, enum pipe pipe)
>  {
> -	struct drm_device *dev = plane->dev;
> -	struct intel_display *display = to_intel_display(dev);
>  	struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES] = {};
>  	int len = 0;
>  	int ret = 0;
>  	int i;
>  
> -	/* Currently expose pipeline only for HDR planes */
> -	if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id))
> -		return 0;
> -
>  	/* Add pipeline consisting of transfer functions */
>  	ret = _intel_color_pipeline_plane_init(plane, &pipelines[len], pipe);

[Severity: High]
This isn't a bug introduced by this patch, but opening this up to SDR planes
makes a pre-existing issue more reachable.

In _intel_color_pipeline_plane_init(), kasprintf() can fail and return NULL
under memory pressure:

	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", 
			       colorop[0]->base.base.id);

Because there is no check for failure, a NULL name gets passed down to
drm_plane_create_color_pipeline_property(), then to drm_property_create_enum(),
and finally to drm_property_add_enum(), which dereferences it during 
strlen(name) and crashes. Should we add a NULL check for kasprintf() in 
_intel_color_pipeline_plane_init()?

[Severity: High]
This is also a pre-existing issue, but since we are expanding the usage of
this pipeline:

If an early initialization failure occurs in
intel_color_pipeline_plane_add_colorop() (such as drm_mode_object_add()
returning an error), the cleanup path calls intel_colorop_destroy() which
invokes drm_colorop_cleanup().

At this point, drm_colorop_cleanup() will dereference colorop->dev:

void drm_colorop_cleanup(struct drm_colorop *colorop)
{
	struct drm_device *dev = colorop->dev;
	struct drm_mode_config *config = &dev->mode_config;
        ...

If drm_mode_object_add() failed, colorop->dev remains NULL, causing an
immediate crash. Additionally, list_del(&colorop->head) is called on a
zero-initialized list node. Can this cleanup path be made safe for objects
that failed early initialization?

[Severity: High]
This is another pre-existing issue exposed by the expanded plane support:

If a colorop initialization fails midway (for instance, if
drm_property_create_enum() fails after drm_mode_object_add() succeeds),
the error path calls drm_colorop_cleanup() to free the memory.

However, drm_colorop_cleanup() does not call drm_mode_object_unregister().
This leaves a freed object pointer in the DRM device's object IDR. Is there
a risk that a user could trigger a use-after-free by passing the leaked
object ID to an ioctl like DRM_IOCTL_MODE_OBJ_GETPROPERTIES?

>  	if (ret)
>  		goto out;
>  	len++;

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