Re: [PATCH] drm/tiny: arcpgu: Replace simple display pipe with regular atomic helpers

[sashiko-bot@kernel.org]

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 <bonatto.davide@gmail.com>

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/20260909092455.131962-1-bonatto.davide@gmail.com?part=1