Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Missing backend implementation for exposed `INTEL_PLANE_CB_CSC_FF` colorop. - [Medium] Unreachable SDR plane color pipeline logic. - [Medium] Validation bypass for 64-bit bitmasks on 32-bit architectures. Pre-existing issues: - [High] NULL pointer dereference in error cleanup path of `intel_color_pipeline_plane_add_colorop`. - [High] Leaked DRM mode object ID on colorop cleanup leads to a Use-After-Free. -- --- Patch [2]: [v5 02/14] drm/i915/color: Add CSC on SDR plane color pipeline --- commit 406763c8b5fb35d6a8f9c41091f76f4b8453fc5a 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] Can this bypass validation for invalid upper bits on 32-bit architectures? When calling drm_plane_colorop_fixed_matrix_init() and passing the 64-bit mask intel_plane_supported_csc_ff, the validation check uses the BIT() macro: drivers/gpu/drm/drm_colorop.c:drm_plane_colorop_fixed_matrix_init() { ... if ((supported_fm & -BIT(DRM_COLOROP_FM_COUNT)) != 0) { ... } Since BIT() returns an unsigned long, which is 32 bits on 32-bit architectures, negating it produces a 32-bit mask (0xFFFFFFFx) that is zero-extended when ANDed with the 64-bit supported_fm mask. Does this silently ignore bits 32-63 during validation? [Severity: High] Does the i915 driver have the backend implementation to process this newly exposed fixed-matrix colorop? The INTEL_PLANE_CB_CSC_FF colorop exposes the fixed_matrix_type property to userspace. However, looking at the parsing logic: drivers/gpu/drm/i915/display/intel_plane.c: intel_plane_color_copy_uapi_to_hw_state() { ... blob = new_colorop_state->bypass ? NULL : new_colorop_state->data; intel_colorop = to_intel_colorop(colorop); changed |= intel_plane_colorop_replace_blob(plane_state, intel_colorop, blob); ... } This appears to only handle blobs (LUTs, CTMs) and completely ignores the fixed_matrix_type state. Could this cause the userspace configuration to be silently discarded and never programmed to the hardware? [Severity: High] This isn't a bug introduced by this patch, but if drm_plane_colorop_fixed_matrix_init() fails, will the cleanup path trigger a NULL pointer dereference? If initialization fails, it jumps to the cleanup label: drivers/gpu/drm/i915/display/intel_color_pipeline.c: intel_color_pipeline_plane_add_colorop() { ... cleanup: intel_colorop_destroy(&colorop->base); return ERR_PTR(ret); } Which then calls drm_colorop_cleanup(): drivers/gpu/drm/drm_colorop.c:drm_colorop_cleanup() { struct drm_device *dev = colorop->dev; struct drm_mode_config *config = &dev->mode_config; list_del(&colorop->head); ... } If the failure happens early in initialization before colorop->dev or colorop->head are set, dev will be NULL and this will crash. [Severity: High] This is a pre-existing issue, but does drm_colorop_cleanup() leak the DRM mode object ID? When drm_plane_colorop_init() executes successfully, it allocates an ID in the DRM device's IDR by calling drm_mode_object_add(). However, drm_colorop_cleanup() appears to be missing a call to drm_mode_object_unregister(dev, &colorop->base). Does this mean the ID remains allocated forever, potentially leading to a use-after-free if userspace later queries the leaked ID after the memory is freed? > 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 else branch ever be executed for SDR planes? Looking at the caller, intel_color_pipeline_plane_init(), there is an explicit early return for non-HDR planes: drivers/gpu/drm/i915/display/intel_color_pipeline.c: intel_color_pipeline_plane_init() { ... /* Currently expose pipeline only for HDR planes */ if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id)) return 0; ... } It seems this prevents the newly added SDR logic from ever being reached. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
