Re: [v6 9/9] drm/i915/color: Add color pipeline support for SDR planes

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

Pre-existing issues:
- [Critical] Use-After-Free vulnerability due to missing `drm_mode_object_unregister()` in `drm_colorop_cleanup()`.
- [High] Kernel crash via NULL pointer dereference if `kasprintf()` fails to allocate the pipeline name.
- [High] NULL pointer dereference of `colorop->dev` during error cleanup in `drm_colorop_cleanup()`.
- [High] Kernel crash via NULL pointer dereference when calling `list_del()` on an uninitialized `list_head`.
--

--- Patch [9]: [v6 9/9] drm/i915/color: Add color pipeline support for SDR planes ---
commit 80efc788f141365ff51bb15bb83bcb8d636605f2
Author: Chaitanya Kumar Borah <[email protected]>

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

This commit exposes the SDR plane color pipeline to user-space by
removing the restriction that previously limited it only 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 53e55ce0a5a35..38cfd6ed585d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> @@ -177,17 +177,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)
>  {
[ ... ]
> -	/* 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 is a pre-existing issue, but does this function properly handle
memory allocation failures?

In drivers/gpu/drm/i915/display/intel_color_pipeline.c:
_intel_color_pipeline_plane_init(), the pipeline name is allocated:

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

	return 0;

If kasprintf() fails, list->name is NULL, but the function still returns 0.
This NULL pointer is then passed up the stack and eventually to
drm_property_add_enum(), which calls strlen(name):

	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
		return -EINVAL;

Could this cause a NULL pointer dereference when memory is tight?


[Severity: High]
This is a pre-existing issue, but could error cleanup paths triggered during
initialization cause a NULL pointer dereference?

If intel_color_pipeline_plane_add_colorop() fails early (for example, if
drm_mode_object_add() fails), the cleanup path calls intel_colorop_destroy(),
which invokes drm_colorop_cleanup().

At this point, colorop->dev might still be NULL. In drm_colorop_cleanup():

	struct drm_device *dev = colorop->dev;
	struct drm_mode_config *config = &dev->mode_config;

	list_del(&colorop->head);
	config->num_colorop--;

Will this unconditionally dereference the NULL dev pointer?


[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free risk when freeing
a colorop?

When a colorop is successfully added to the DRM IDR via drm_mode_object_add(),
it needs to be unregistered when destroyed. However, drm_colorop_cleanup()
does not appear to call drm_mode_object_unregister().

If a colorop is freed, but its mode object ID remains valid in the IDR, could
userspace trigger a use-after-free by interacting with the stale ID?


[Severity: High]
This is a pre-existing issue, but could the error handling crash when removing
an uninitialized list node?

In drm_plane_colorop_init(), if drm_mode_object_add() fails, the function
returns early before the colorop is added to the list:

	ret = drm_mode_object_add(dev, &colorop->base,
				  DRM_MODE_OBJECT_COLOROP);
	if (ret)
		return ret;

When the error path later calls drm_colorop_cleanup(), it unconditionally
executes list_del(&colorop->head). Since the head was zero-initialized via
kzalloc_obj(), __list_del() attempts to write to next->prev.

Will this result in an immediate NULL pointer dereference?

>  	if (ret)

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