Re: [RFC PATCH 3/8] drm/i915/colorop: Update to new pipeline API

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing `drm_mode_object_unregister()` in colorop cleanup leaves dangling pointers in the DRM device IDR.
- [High] If colorop initialization fails early, the cleanup path will unconditionally dereference uninitialized pointers, causing a kernel panic.
- [High] Missing error handling for `kasprintf` leads to a NULL pointer dereference when creating the pipeline property.
--

commit 430890cda6ad288f9a83188f2b716cf9737d8c83
Author: John Harrison <John.Harrison@Igalia.com>

drm/i915/colorop: Update to new pipeline API

Switch to using the new method for building colorop pipelines.

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

[ ... ]

> @@ -100,8 +100,10 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
>  	if (ret)
>  		goto cleanup;
>  
> -	if (prev)
> -		drm_colorop_set_next_property(&prev->base, &colorop->base);
> +	if (!*pipeline)
> +		*pipeline = &colorop->base;
> +
> +	drm_colorop_add_to_pipeline(*pipeline, &colorop->base);
>  
>  	return colorop;

[Severity: High]
This isn't a bug introduced by this patch, but if the colorop initialization
fails before this point, the code jumps to the cleanup label:

	cleanup:
		intel_colorop_destroy(&colorop->base);
		return ERR_PTR(ret);

Could this unconditionally dereference an uninitialized pointer?

The colorop was just allocated, so colorop->dev is NULL.
Calling intel_colorop_destroy() invokes drm_colorop_cleanup(), which
attempts to dereference colorop->dev and call list_del(&colorop->head),
resulting in a NULL pointer dereference.

[Severity: Critical]
This is another pre-existing issue in the teardown path.

If a colorop is later destroyed (either here in the error path, or during
general teardown), drm_colorop_cleanup() is called:

	void drm_colorop_cleanup(struct drm_colorop *colorop)
	{
		...
		list_del(&colorop->head);
		config->num_colorop--;

		if (colorop->state)
			drm_colorop_atomic_destroy_state(colorop,
							 colorop->state);
	}

Is this missing a call to drm_mode_object_unregister()?

The object is registered with the DRM device IDR via drm_mode_object_add()
during initialization. Omitting drm_mode_object_unregister() leaves a
dangling pointer in dev->mode_config.object_idr, which could lead to a
use-after-free or a system crash during device teardown.

[ ... ]

> @@ -132,14 +134,12 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
>  	}
>  
>  	for (i = 0; i < pipeline_len; i++) {
> -		colorop[i] = intel_color_pipeline_plane_add_colorop(plane, prev,
> +		colorop[i] = intel_color_pipeline_plane_add_colorop(plane, &pipeline_root,
>  								    pipeline[i]);
>  		if (IS_ERR(colorop[i])) {
>  			ret = PTR_ERR(colorop[i]);
>  			goto cleanup;
>  		}
> -
> -		prev = colorop[i];
>  	}
>  
>  	list->type = colorop[0]->base.base.id;

[Severity: High]
This is a pre-existing issue, but right after this line in
_intel_color_pipeline_plane_init(), there is an unchecked kasprintf():

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

	return 0;

If this allocation fails under memory pressure, list->name is set to NULL
and the function unconditionally returns 0 (success).

Could this cause a NULL pointer dereference later when
drm_plane_create_color_pipeline_property() passes this name to
drm_property_create_enum() and drm_property_add_enum(), which performs a
strlen(name)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609171941.169104-1-John.Harrison@Igalia.com?part=3