Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Missing primary plane visibility check allows CRTC to be enabled with disabled plane, causing DMA scanout from physical address 0. --
commit 0780d3c6b6d7797f539151d0b77405608d2d3ea1 Author: Davide Bonatto <[email protected]> Subject: 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. Migrate arcpgu to explicitly route mode validation and atomic hooks through standard helper functions. > diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c > index c93d61ac0bb75..c6308fee568db 100644 > --- a/drivers/gpu/drm/tiny/arcpgu.c > +++ b/drivers/gpu/drm/tiny/arcpgu.c [ ... ] > @@ -189,54 +203,127 @@ static void arc_pgu_mode_set(struct arcpgu_drm_private > *arcpgu) [ ... ] > +static int arc_pgu_plane_atomic_check(struct drm_plane *plane, > + struct drm_atomic_commit *state) > +{ > + struct drm_plane_state *new_plane_state = > drm_atomic_get_new_plane_state(state, plane); > + struct drm_crtc_state *crtc_state; > + > + if (!new_plane_state->crtc) > + return 0; > + > + crtc_state = drm_atomic_get_new_crtc_state(state, > new_plane_state->crtc); > + if (!crtc_state) > + return -EINVAL; > + > + return drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, > + DRM_PLANE_NO_SCALING, > + DRM_PLANE_NO_SCALING, > + false, true); [Severity: High] Does this code allow the CRTC to remain enabled without a visible primary plane, potentially causing the DMA controller to scan from physical address 0? When drm_atomic_helper_check_plane_state() runs, it returns 0 (success) if the plane is not visible (e.g., when the framebuffer is NULL). If a userspace application issues an atomic commit that sets the primary plane's FB_ID to 0 without also disabling the CRTC, the atomic core sets new_plane_state->fb = NULL and new_plane_state->visible = false. Could we add a check here verifying that the primary plane is visible when the CRTC is enabled (e.g., if (crtc_state->enable && !new_plane_state->visible) return -EINVAL;) to prevent scanning from address 0? > +} > + > +static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc, > + struct drm_atomic_commit *state) > +{ > + struct drm_crtc_state *crct_state = > drm_atomic_get_new_crtc_state(state, crtc); > + > + if (!crct_state->enable) > + return 0; > + > + if (!crct_state->plane_mask) > + return -EINVAL; [Severity: High] Is this check sufficient to guarantee that the primary plane has a valid framebuffer? Setting the primary plane's FB_ID to 0 does not change the plane's crtc mapping, meaning crct_state->plane_mask remains non-zero. This check will incorrectly approve the commit, allowing it to proceed. The commit would then invoke arc_pgu_plane_atomic_disable() which writes 0 to ARCPGU_REG_BUF0_ADDR. Since the CRTC remains enabled, the hardware DMA controller could begin scanning out pixels from physical address 0, causing bus errors, IOMMU faults, or memory corruption. > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
