Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] A missing NULL check for the `kasprintf()` allocation in `_intel_color_pipeline_plane_init()` causes a NULL pointer dereference in `drm_property_add_enum()`. - [High] Early initialization failures during colorop creation cause `drm_colorop_cleanup()` to access uninitialized structure members, leading to a crash. - [High] Missing `drm_mode_object_unregister()` in `drm_colorop_cleanup()` leaves destroyed colorops in the DRM object IDR, leading to a Use-After-Free. -- --- Patch [14]: [v5 14/14] drm/i915/color: Add color pipeline support for SDR planes --- commit 898c62da94ca7a00530f154f1e3d4b18c45aaa47 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 check that previously limited it 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 136b9378ca484..a99b6eb204488 100644 > --- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c > +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c > @@ -188,17 +188,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) > { > - struct drm_device *dev = plane->dev; > - struct intel_display *display = to_intel_display(dev); > struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES] = {}; > int len = 0; > int ret = 0; > int i; > > - /* 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 isn't a bug introduced by this patch, but opening this up to SDR planes makes a pre-existing issue more reachable. In _intel_color_pipeline_plane_init(), kasprintf() can fail and return NULL under memory pressure: list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", colorop[0]->base.base.id); Because there is no check for failure, a NULL name gets passed down to drm_plane_create_color_pipeline_property(), then to drm_property_create_enum(), and finally to drm_property_add_enum(), which dereferences it during strlen(name) and crashes. Should we add a NULL check for kasprintf() in _intel_color_pipeline_plane_init()? [Severity: High] This is also a pre-existing issue, but since we are expanding the usage of this pipeline: If an early initialization failure occurs in intel_color_pipeline_plane_add_colorop() (such as drm_mode_object_add() returning an error), the cleanup path calls intel_colorop_destroy() which invokes drm_colorop_cleanup(). At this point, drm_colorop_cleanup() will dereference colorop->dev: void drm_colorop_cleanup(struct drm_colorop *colorop) { struct drm_device *dev = colorop->dev; struct drm_mode_config *config = &dev->mode_config; ... If drm_mode_object_add() failed, colorop->dev remains NULL, causing an immediate crash. Additionally, list_del(&colorop->head) is called on a zero-initialized list node. Can this cleanup path be made safe for objects that failed early initialization? [Severity: High] This is another pre-existing issue exposed by the expanded plane support: If a colorop initialization fails midway (for instance, if drm_property_create_enum() fails after drm_mode_object_add() succeeds), the error path calls drm_colorop_cleanup() to free the memory. However, drm_colorop_cleanup() does not call drm_mode_object_unregister(). This leaves a freed object pointer in the DRM device's object IDR. Is there a risk that a user could trigger a use-after-free by passing the leaked object ID to an ioctl like DRM_IOCTL_MODE_OBJ_GETPROPERTIES? > if (ret) > goto out; > len++; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
