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 <[email protected]> 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/[email protected]?part=3
