Re: [PATCH v6 26/28] drm/tidss: Implement readout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] drm/tidss: uninitialized stack memory info leak in crtc readout
- [High] drm/tidss: integer overflow in calc_pixel_clock_hz() on 32-bit archs
- [High] drm/tidss: unhandled clock enable failure causes external abort crash
- [Medium] drm/tidss: memory leak of readout framebuffer on multi-crtc hardware
- [Medium] drm/tidss: permanent clock reference leak via double-enable in readout
- [Medium] drm/tidss: plane readout fails unconditionally on multi-crtc hardware
- [Medium] drm/tidss: clock enable count leak and underflow in CRTC readout/install
--

commit 5f7c5b078a348c064d70aaabd04294cf1916f772
Author: Maxime Ripard <mripard@kernel.org>

drm/tidss: Implement readout support

With the hardware readout infrastructure now in place in the KMS
framework, we can provide it for the tidss driver.

> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 3da49f77fa080..666b950278ee4 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
[ ... ]
> @@ -370,6 +374,93 @@ static struct drm_crtc_state *tidss_crtc_create_state(struct drm_crtc *crtc)
>  	return &tstate->base;
>  }
>  
> +static int tidss_crtc_readout_state(struct drm_crtc *crtc,
> +				    struct drm_atomic_sro_state *state,
> +				    struct drm_crtc_state *crtc_state)
> +{
[ ... ]
> +	struct drm_display_mode mode;
> +	int ret;
> +

[Severity: High]
Can this uninitialized stack memory be leaked to userspace?

This struct is allocated on the stack without zero-initialization. When it is
passed to dispc_vp_readout_mode(), only some fields are populated, and the
flags field is modified using a bitwise OR, preserving the stack garbage. The
struct is then copied to the CRTC state via drm_atomic_set_mode_for_crtc(),
which can expose the uninitialized kernel stack memory directly to userspace.

> +	tidss_runtime_get(tidss);
> +
> +	if (!dispc_vp_is_enabled(dispc, tcrtc->hw_videoport))
> +		goto out;
> +
> +	/*
> +	 * The display is active, we need to enable our clock to have
> +	 * proper reference count.
> +	 */
> +	WARN_ON(dispc_vp_enable_clk(tidss->dispc, tcrtc->hw_videoport));

[Severity: High]
Does this unhandled clock enable failure cause a kernel panic on ARM platforms?

If dispc_vp_enable_clk() fails here, the code logs a warning but proceeds to
access the display controller's MMIO registers in dispc_vp_readout_mode(). On
ARM architectures, reading from an unclocked peripheral triggers a bus error
and an immediate external abort crash.

[Severity: Medium]
Can this clock enable cause an underflow in the reference count on error?

If this initial dispc_vp_enable_clk() fails, tidss_crtc_atomic_disable() will
later unconditionally call dispc_vp_disable_clk(), underflowing the clock
enable count. Additionally, if the clock enables successfully but a subsequent
error occurs during readout, there is no error cleanup to disable the clock.

> +
> +	tstate->base.active = 1;
> +	tstate->base.enable = 1;
[ ... ]
> +static void tidss_crtc_install_state(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *crtc_state)
> +{
> +	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> +	struct drm_device *ddev = crtc->dev;
> +	struct tidss_device *tidss = to_tidss(ddev);
> +	int ret;
> +
> +	tidss_runtime_get(tidss);
> +
> +	ret = dispc_vp_enable_clk(tidss->dispc, tcrtc->hw_videoport);
> +	if (ret != 0)
> +		return;

[Severity: Medium]
Does this second clock enable cause a permanent reference count leak?

The video port clock was already enabled during tidss_crtc_readout_state().
Enabling it a second time here double-increments the reference count. Since
tidss_crtc_atomic_disable() only disables the clock once, this leaves a
permanent clock reference leak on the success path, preventing the hardware
from entering low-power states.

> +
> +	/* Turn vertical blanking interrupt reporting on. */
> +	drm_crtc_vblank_on(crtc);
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index fdd3b166c1181..50e6d9fb92083 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
[ ... ]
> +static unsigned long calc_pixel_clock_hz(unsigned int htotal,
> +					 unsigned int vtotal,
> +					 unsigned int refresh,
> +					 unsigned int freq_div)
> +{
> +	unsigned long rate = (unsigned long)htotal * vtotal * refresh;
> +
> +	return (rate * 1000) / freq_div;
> +}

[Severity: High]
Does this multiplication overflow on 32-bit architectures?

On 32-bit platforms (such as the ARM32 K2G SoCs supported by this driver),
rate uses 32-bit unsigned arithmetic. For a standard 1080p60 display, rate is
roughly 148,500,000. Multiplying this by 1000 yields 148,500,000,000, which
drastically exceeds the 32-bit maximum. This overflow results in a corrupted
clock frequency and breaks refresh rate calculations.

[ ... ]
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index b0d5d6959b885..32b80a3f8f7d3 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
[ ... ]
> +static struct drm_crtc *tidss_plane_readout_crtc(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +
> +	if (dev->num_crtcs != 1)
> +		return ERR_PTR(-EINVAL);

[Severity: Medium]
Will this intentionally break plane state readout on devices with multiple
video ports?

The hardware natively supports multiple CRTCs (e.g., AM65x has two video
ports), but this hardcoded check causes plane readout to fail unconditionally
on any multi-CRTC configuration. Hardware routing registers likely need to be
read to dynamically determine which CRTC the plane is connected to.

> +
> +	return list_first_entry(&dev->mode_config.crtc_list, struct drm_crtc, head);
> +}
> +
> +static int tidss_plane_atomic_readout_state(struct drm_plane *plane,
> +					    struct drm_atomic_sro_state *state,
> +					    struct drm_plane_state *plane_state)
> +{
[ ... ]
> +	fb = tidss_plane_readout_fb(plane);
> +	if (IS_ERR(fb)) {
> +		ret = PTR_ERR(fb);
> +		goto err_runtime_pm;
> +	}
> +
> +	crtc = tidss_plane_readout_crtc(plane);
> +	if (IS_ERR(crtc)) {
> +		ret = PTR_ERR(crtc);
> +		goto err_runtime_pm;
> +	}

[Severity: Medium]
Does this error path leak the dummy framebuffer?

tidss_plane_readout_fb() allocates and initializes fb with a reference count
of 1. If tidss_plane_readout_crtc() fails (which it does unconditionally on
multi-CRTC hardware), the code jumps to err_runtime_pm, bypassing the
assignment to plane_state->fb. Since drm_framebuffer_put() is never called,
the framebuffer memory and its IDR entry are permanently leaked.

> +
> +	plane_state->fb = fb;
> +	plane_state->crtc = crtc;
> +	plane_state->visible = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=26