Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Medium] The newly added SDR plane color pipeline logic is completely unreachable due to an early return for non-HDR planes in the caller function. - [Medium] The driver exposes the `CSC_FF` colorop but fails to extract and apply its atomic state, violating the DRM API contract. Pre-existing issues: - [Critical] `drm_colorop_cleanup()` fails to unregister the DRM mode object, leading to a Use-After-Free via stale object IDs. - [High] `drm_colorop_cleanup` assumes `colorop->dev` and `colorop->head` are fully initialized, causing a NULL pointer dereference and crash on early initialization failures. - [High] Unchecked return value from `kasprintf()` leads to a NULL pointer dereference during DRM property creation. -- --- Patch [2]: [CI v4 02/14] drm/i915/color: Add CSC on SDR plane color pipeline --- commit ed94f2ef5daa2b6a1fe43a799710d164ac1323d4 Author: Chaitanya Kumar Borah <[email protected]> drm/i915/color: Add CSC on SDR plane color pipeline Add the fixed-function CSC block to 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 > index 6cf8080ee8000..2ef42a133a987 100644 > --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c [ ... ] > @@ -92,6 +102,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: Medium] Will intel_plane_color_copy_uapi_to_hw_state() successfully extract the state for INTEL_PLANE_CB_CSC_FF? Looking at intel_plane_colorop_replace_blob() in drivers/gpu/drm/i915/display/intel_plane.c, it handles blob-based colorops like INTEL_PLANE_CB_CSC, but lacks a case for INTEL_PLANE_CB_CSC_FF. Could this cause userspace settings for fixed_matrix_type and bypass on this colorop to be silently ignored when applying the pipeline? [Severity: High] This is a pre-existing issue, but if drm_plane_colorop_fixed_matrix_init() or other initialization functions fail early (e.g. due to OOM), the code jumps to the cleanup label in this function and calls intel_colorop_destroy(). This calls drm_colorop_cleanup(), which unconditionally dereferences colorop->dev and colorop->head via list_del(). Since these fields might not be initialized on early failure, is it possible for this to cause a NULL pointer dereference? [Severity: Critical] This is another pre-existing issue on the cleanup path. When drm_colorop_cleanup() runs, it never calls drm_mode_object_unregister(). Because the object was registered in drm_mode_object_add() during init, does omitting drm_mode_object_unregister() leave a dangling pointer in the device's IDR, allowing userspace to trigger a use-after-free by querying the freed object ID? > default: > drm_err(plane->dev, "Invalid colorop id [%d]", id); > ret = -EINVAL; [ ... ] > @@ -126,9 +142,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_pipeline; > + pipeline_len = ARRAY_SIZE(sdr_plane_pipeline); > } [Severity: Medium] Will this newly added sdr_plane_pipeline ever execute? The caller of this function, intel_color_pipeline_plane_init(), has an early return: intel_color_pipeline_plane_init() if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id)) return 0; ret = _intel_color_pipeline_plane_init(plane, &pipelines[len], pipe); Because it skips all non-HDR planes before calling this function, doesn't that make this else branch for SDR planes completely unreachable? [Severity: High] This isn't a bug introduced by this patch, but immediately below this block in _intel_color_pipeline_plane_init(), kasprintf() is called: list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", ...); The return value is not checked for NULL. If kasprintf() fails, it passes the NULL pointer to drm_plane_create_color_pipeline_property(), which passes it to drm_property_create_enum(). Will this cause a panic when strlen(name) is called on the NULL pointer inside drm_property_add_enum()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
