Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] Missing `.destroy` callback in `radeon_primary_plane_funcs` causes a NULL pointer dereference in `drm_mode_config_cleanup()` during driver unload or probe failure. - [High] Freeing `radeon_crtc` on `drm_crtc_init_with_planes()` failure introduces a Use-After-Free due to a dangling list pointer in DRM core. Pre-existing issues: - [High] Unhandled `-ENOMEM` from `drm_mode_crtc_set_gamma_size` leads to guaranteed NULL pointer dereference in LUT loading functions. - [High] Silent initialization failures in `radeon_crtc_init` leave unpopulated CRTC slots, leading to NULL pointer dereferences later. -- commit e5375b01119a0846e47327447f621f702f5ebac8 Author: Shixiong Ou <[email protected]> drm/radeon: Create the primary plane in the driver This patch creates a driver-owned primary plane advertising XRGB8888 and ARGB8888, allocated via drmm_universal_plane_alloc(). It sets a blend mode property to prevent validation warnings that occur when falling back to the legacy fixed format list in the DRM core. > diff --git a/drivers/gpu/drm/radeon/radeon_display.c > b/drivers/gpu/drm/radeon/radeon_display.c > index aac6733ddd82c..6427feb5a5c57 100644 > --- a/drivers/gpu/drm/radeon/radeon_display.c > +++ b/drivers/gpu/drm/radeon/radeon_display.c [ ... ] > @@ -664,6 +667,20 @@ radeon_crtc_set_config(struct drm_mode_set *set, > return ret; > } > > +/* > + * The display engine programs an ARGB8888 surface format for both > + * XRGB8888 and ARGB8888 framebuffers. > + */ > +static const uint32_t radeon_primary_formats[] = { > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_ARGB8888, > +}; > + > +static const struct drm_plane_funcs radeon_primary_plane_funcs = { > + .update_plane = drm_plane_helper_update_primary, > + .disable_plane = drm_plane_helper_disable_primary, > +}; > + [Severity: Critical] Does omitting the .destroy callback here lead to a NULL pointer dereference on module unload or initialization failure? While drmm_universal_plane_alloc() requires the destroy callback to be omitted for managed cleanup, the legacy radeon driver explicitly calls drm_mode_config_cleanup() in radeon_modeset_fini(). Because drm_mode_config_cleanup() iterates over dev->mode_config.plane_list and unconditionally executes plane->funcs->destroy(plane), won't it attempt to dereference a NULL function pointer when it encounters this newly added primary plane? > static const struct drm_crtc_funcs radeon_crtc_funcs = { > .cursor_set2 = radeon_crtc_cursor_set2, > .cursor_move = radeon_crtc_cursor_move, [ ... ] > @@ -693,7 +711,20 @@ static void radeon_crtc_init(struct drm_device *dev, int > index) > return; > } > > - drm_crtc_init(dev, &radeon_crtc->base, &radeon_crtc_funcs); > + primary = drmm_universal_plane_alloc(dev, struct drm_plane, dev, 0, > + &radeon_primary_plane_funcs, > + radeon_primary_formats, > + ARRAY_SIZE(radeon_primary_formats), > + NULL, DRM_PLANE_TYPE_PRIMARY, > NULL); > + if (IS_ERR(primary)) > + goto err_free_crtc; > + [Severity: High] This is a pre-existing issue, but this patch introduces new failure paths like this one that expose it. If we jump to err_free_crtc and return early from radeon_crtc_init(), the array slot rdev->mode_info.crtcs[index] is left unpopulated (NULL). The caller, radeon_modeset_init(), does not check for failures and proceeds. Will this lead to a NULL pointer dereference later when other paths (like watermark or bandwidth updates) unconditionally access rdev->mode_info.crtcs[i]->base.enabled? > + if (drm_crtc_init_with_planes(dev, &radeon_crtc->base, primary, NULL, > + &radeon_crtc_funcs, NULL)) > + goto err_free_crtc; > + > + drm_plane_create_blend_mode_property(primary, > + BIT(DRM_MODE_BLEND_PREMULTI)); > > drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256); [Severity: High] This isn't a bug introduced by this patch, but the return value of drm_mode_crtc_set_gamma_size() is ignored here. If drm_mode_crtc_set_gamma_size() returns -ENOMEM, it leaves crtc->gamma_store as NULL. Later, in hardware programming paths like dce4_crtc_load_lut(), the driver assigns: r = crtc->gamma_store; and unconditionally iterates up to 256, dereferencing the NULL pointer. Should the error return from drm_mode_crtc_set_gamma_size() be handled to prevent this? > radeon_crtc->crtc_id = index; [ ... ] > @@ -713,6 +744,11 @@ static void radeon_crtc_init(struct drm_device *dev, int > index) > radeon_atombios_init_crtc(dev, radeon_crtc); > else > radeon_legacy_init_crtc(dev, radeon_crtc); > + return; > + > +err_free_crtc: > + destroy_workqueue(radeon_crtc->flip_queue); > + kfree(radeon_crtc); > } [Severity: High] Can freeing radeon_crtc here cause a use-after-free regression by leaving a dangling pointer in the DRM core? Inside __drm_crtc_init_with_planes(), the CRTC is added to the DRM core's dev->mode_config.crtc_list early via list_add_tail(). If a late initialization step within that function fails (such as drm_crtc_crc_init()), the DRM core returns an error but fails to unlink the CRTC from the list. If we immediately kfree(radeon_crtc) here, any subsequent iteration over crtc_list (like during drm_mode_config_cleanup()) will access freed memory. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
