Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Critical] Hardware DMA scans out freed memory when plane is disabled without CRTC disable (Use-After-Free). - [High] NULL Pointer Dereference during CRTC enable without a primary plane. - [High] Missing `.prepare_fb` callback breaks implicit synchronization. - [Medium] Missing plane `.atomic_check` hook bypasses hardware scaling constraints. Pre-existing issues: - [High] Sleeping functions called from atomic context. -- commit 776de1d240335d0996c4eca5f0b2980de524e7c7 Author: Davide Bonatto <[email protected]> drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers The drm_simple_display_pipe helper is considered legacy/deprecated in favor of explicitly managing individual plane, CRTC, and encoder objects using standard atomic helpers. > diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c > index c93d61ac0bb75..bd24049845350 100644 > --- a/drivers/gpu/drm/tiny/arcpgu.c > +++ b/drivers/gpu/drm/tiny/arcpgu.c [ ... ] > @@ -117,8 +123,8 @@ static const u32 arc_pgu_supported_formats[] = { > > static void arc_pgu_set_pxl_fmt(struct arcpgu_drm_private *arcpgu) > { > - const struct drm_framebuffer *fb = arcpgu->pipe.plane.state->fb; > - uint32_t pixel_format = fb->format->format; > + const struct drm_framebuffer *fb = arcpgu->plane.state->fb; > + u32 pixel_format = fb->format->format; [Severity: High] Can this result in a NULL pointer dereference if the CRTC is enabled without a primary plane? Because the CRTC .atomic_check hook has been omitted in this patch, the driver loses the drm_atomic_helper_check_crtc_primary_plane() validation. Userspace can now submit an atomic commit that enables the CRTC without a bound primary plane framebuffer. During the commit, arc_pgu_crtc_atomic_enable() calls arc_pgu_mode_set(), which then calls arc_pgu_set_pxl_fmt(). If fb is NULL here, dereferencing fb->format->format will cause a kernel panic. [ ... ] > -static void arc_pgu_enable(struct drm_simple_display_pipe *pipe, > - struct drm_crtc_state *crtc_state, > - struct drm_plane_state *plane_state) > +static void arc_pgu_crtc_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > { > - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe); > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > arc_pgu_mode_set(arcpgu); > > clk_prepare_enable(arcpgu->clk); [Severity: High] This isn't a bug introduced by this patch, but does calling clk_prepare_enable() here violate atomic context rules? The DRM subsystem guidelines specify that CRTC atomic enable callbacks execute in atomic context during non-blocking commits, where sleeping is strictly forbidden. Since clk_prepare_enable() can sleep, it could lead to deadlocks or system instability. [ ... ] > -static void arc_pgu_disable(struct drm_simple_display_pipe *pipe) > +static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > { > - struct arcpgu_drm_private *arcpgu = pipe_to_arcpgu_priv(pipe); > + struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc); > > clk_disable_unprepare(arcpgu->clk); [Severity: High] This is also a pre-existing issue, but similar to the enable path, is it safe to call clk_disable_unprepare() here? This function can sleep, which is unsafe to call during the atomic_disable callback. [ ... ] > -static void arc_pgu_update(struct drm_simple_display_pipe *pipe, > - struct drm_plane_state *state) > +static void arc_pgu_plane_atomic_update(struct drm_plane *plane, > + struct drm_atomic_commit *state) > { > - struct arcpgu_drm_private *arcpgu; > + struct arcpgu_drm_private *arcpgu = plane_to_arcpgu(plane); > + struct drm_plane_state *new_plane_state = > drm_atomic_get_new_plane_state(state, plane); > struct drm_gem_dma_object *gem; > > - if (!pipe->plane.state->fb) > + if (!new_plane_state->fb) > return; [Severity: Critical] Could this early return cause the hardware DMA to scan out freed memory? Due to the missing CRTC .atomic_check hook, userspace can submit a commit that disables the primary plane while keeping the CRTC active. If new_plane_state->fb is NULL, this function returns early without updating the hardware. Since the CRTC remains enabled, the hardware DMA will continue scanning from the previously configured buffer address after the DRM core frees it, leading to memory corruption or an information leak. [ ... ] > +static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { > + .mode_valid = arc_pgu_crtc_mode_valid, > + .atomic_enable = arc_pgu_crtc_atomic_enable, > + .atomic_disable = arc_pgu_crtc_atomic_disable, > +}; [Severity: High] Is the .atomic_check hook intentionally omitted here? Without it, the driver loses the validation that the simple KMS helper previously provided to enforce a bound primary plane. This allows an invalid atomic state to proceed to the commit phase, leading to the NULL pointer dereference or use-after-free scenarios mentioned above. [ ... ] > +static const struct drm_plane_helper_funcs arc_pgu_plane_helper_funcs = { > + .atomic_update = arc_pgu_plane_atomic_update, > +}; [Severity: Medium] Does this structure need an .atomic_check function to validate hardware scaling constraints? The simple KMS helper previously enforced DRM_PLANE_NO_SCALING via drm_atomic_helper_check_plane_state(). Without this validation, the driver might incorrectly accept atomic commits with unsupported scaling or positioning constraints from userspace. [Severity: High] Will the omission of the .prepare_fb callback break implicit synchronization? In the simple KMS helper, this was handled automatically via drm_gem_plane_helper_prepare_fb(). Without this callback, the DRM core cannot extract the implicit dma_fence from the GEM object's DMA-buf reservation, meaning the atomic commit will immediately scan out the buffer without waiting for GPU rendering to finish. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
