Re: [PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The commit claims to inline `drm_simple_encoder_init` for the entire meson driver, but it only modifies `meson_encoder_hdmi.c`, leaving other encoders unconverted.

Pre-existing issues:
- [High] Missing `drm_bridge_remove()` on error paths in `meson_encoder_hdmi_probe()` leaves the bridge registered in the global `bridge_list`, leading to a dangling pointer and potential Use-After-Free.
--

commit 640b38eb942af61527b421bb5d62cb275bca9574
Author: Liam Zuiderhoek <zuiderhoekl@gmail.com>

GPU: drm: meson: inline call to drm_simple_encoder_init()

This commit inlines the function drm_simple_encoder_init for the meson
driver. It replaces the simple encoder init call with drm_encoder_init
and a custom cleanup struct.

> This commit inlines the function drm_simple_encoder_init for the meson
> driver.

[Severity: Medium]
Is the commit message description accurate for the scope of this patch?

The commit states it inlines the function for the meson driver, but this patch 
only modifies the HDMI encoder (meson_encoder_hdmi.c). 

The CVBS (meson_encoder_cvbs_probe) and DSI (meson_encoder_dsi_probe) encoders 
in the same driver still use drm_simple_encoder_init().

Should this patch update those files as well, or should the commit message be 
clarified to specify it only targets the HDMI encoder?

> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 55c0601df3c62..0dd3a76ad959e 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c

[ ... ]

> @@ -407,8 +411,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv)
>  	meson_encoder_hdmi->priv = priv;
>  
>  	/* Encoder */
> -	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
> -				      DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
> +			       &drm_simple_encoder_funcs_cleanup,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
>  	if (ret) {
>  		dev_err_probe(priv->dev, ret, "Failed to init HDMI encoder\n");
>  		goto err_put_node;

[Severity: High]
This is a pre-existing issue, but does this error path leave the bridge 
registered in the global bridge list, leading to a potential use-after-free?

In meson_encoder_hdmi_probe(), drm_bridge_add() is called before the encoder 
initialization. 

If we fail here at drm_encoder_init() (or later during 
drm_bridge_connector_init()), the code jumps to err_put_node which does not 
call drm_bridge_remove():

err_put_node:
	of_node_put(remote);
	return ret;

Similarly, if cec_notifier_conn_register() fails later in the probe, it returns 
directly without cleaning up the bridge.

The cleanup function meson_encoder_hdmi_remove() will not catch this because 
priv->encoders[MESON_ENC_HDMI] is only assigned at the very end of a successful 
probe. 

If the still-registered bridge is later found via of_drm_find_and_get_bridge() 
and its callbacks are invoked, could it dereference the freed priv pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629173902.29415-1-zuiderhoekl@gmail.com?part=1