Re: [v6 6/9] drm/i915/color: Add support for 1D LUT in SDR planes

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

Pre-existing issues:
- [High] Missing drm_mode_object_unregister() in colorop cleanup paths leaves dangling pointers in the DRM object IDR.
- [High] Missing NULL check for kasprintf leads to crash during color pipeline property creation.
- [High] Error cleanup path in intel_color_pipeline_plane_add_colorop crashes when colorop initialization fails early.
--

--- Patch [6]: [v6 6/9] drm/i915/color: Add support for 1D LUT in SDR planes ---
commit b0190470a3e343638c9aa8e156a26be16a13657a
Author: Chaitanya Kumar Borah <[email protected]>

drm/i915/color: Add support for 1D LUT in SDR planes
    
This commit extends the SDR plane color pipeline to include a post-CSC 1D
LUT block. It simplifies the YUV to RGB pipeline by dropping the pre-CSC
LUT.

> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> index efd4375c43318..53e55ce0a5a35 100644
> --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> @@ -45,6 +45,7 @@ static const enum intel_color_block hdr_plane_pipeline[] = {
>  
>  static const enum intel_color_block sdr_plane_yuv_pipeline[] = {
>  	INTEL_PLANE_CB_CSC_FF,
> +	INTEL_PLANE_CB_POST_CSC_LUT,
>  };

[Severity: High]
This is a pre-existing issue, but I have a question about the cleanup path
when initializing these colorops. If colorop initialization fails halfway
(for example, if drm_property_create_enum fails in drm_plane_colorop_init
after drm_mode_object_add), the error path calls intel_colorop_destroy
which delegates to drm_colorop_cleanup:

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

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

Is a call to drm_mode_object_unregister() missing here before the caller
frees the memory? It seems this could leave dangling pointers in the DRM
object IDR, which might be exploitable via DRM_IOCTL_MODE_OBJ_GETPROPERTIES.

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
intel_color_pipeline_plane_add_colorop handle early initialization failures
safely?

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

If an initialization function like drm_plane_colorop_curve_1d_lut_init
fails early inside drm_plane_colorop_init (such as when drm_mode_object_add
fails), colorop->dev is left as NULL and the head list node is uninitialized.
When drm_colorop_cleanup accesses dev->mode_config.num_colorop, could this
cause a NULL pointer dereference and an invalid list_del?

[Severity: High]
This is another pre-existing issue in the color pipeline setup code. I
noticed a potential issue in _intel_color_pipeline_plane_init where
kasprintf is used without checking the return value:

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

	return 0;

Can this lead to a crash if kasprintf fails? If list->name is NULL, the
function still returns success. This NULL name is then passed through to
drm_plane_create_color_pipeline_property and eventually drm_property_add_enum,
where WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN) could oops when attempting
to dereference the NULL pointer.

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