Re: [PATCH 5/6] drm/meson: Open-code drm_simple_encoder_init() in encoder_hdmi

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `drm_encoder` is embedded in a `devm`-allocated structure but is never explicitly removed from the DRM device's encoder list on unbind, leading to a use-after-free.
- [High] Memory leak and dangling bridge pointer on probe error paths due to missing `drm_bridge_remove()`.
--

commit b2fc3d021ba4d558d56f29caf06c9583a6e5c48f
Author: Naman Arora <namanarora029@gmail.com>

drm/meson: Open-code drm_simple_encoder_init() in encoder_hdmi

The helper drm_simple_encoder_init() is a thin wrapper around
drm_encoder_init() with a simple destroy-only encoder funcs struct.
Remove the dependency on drm_simple_kms_helper by open-coding the
encoder initialization directly in the driver.

> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 55c0601df3c62..0da46af7371ea 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
[ ... ]
> @@ -407,8 +410,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,
> +			       &meson_encoder_hdmi_drm_encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free when
unbinding the device?

The meson_encoder_hdmi structure, which embeds the drm_encoder, is allocated
via devm_drm_bridge_alloc(). This means its memory is freed when the device
unbinds. However, meson_encoder_hdmi_remove() fails to call
drm_encoder_cleanup(), leaving the encoder linked in the DRM device's
mode_config.encoder_list.

If a userspace process keeps the DRM device file descriptor open, the DRM
device outlives the driver binding. When the DRM device is finally destroyed,
drm_mode_config_cleanup() iterates over the encoder_list and calls the
destroy hook on the already freed encoder:

drivers/gpu/drm/meson/meson_encoder_hdmi.c:meson_encoder_hdmi_remove() {
	if (priv->encoders[MESON_ENC_HDMI]) {
		meson_encoder_hdmi = priv->encoders[MESON_ENC_HDMI];
		drm_bridge_remove(&meson_encoder_hdmi->bridge);
		...
	}
}

Is it necessary to explicitly remove the encoder from the list during remove?

>  	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 leak the bridge and
leave a dangling pointer in the global bridge list?

Earlier in meson_encoder_hdmi_probe(), drm_bridge_add() increments the
bridge's reference count and adds it to the global bridge_list:

	drm_bridge_add(&meson_encoder_hdmi->bridge);

If the initialization steps fail here and jump to err_put_node, the code
returns without calling drm_bridge_remove(). The devm cleanup only drops the
initial reference taken by devm_drm_bridge_alloc().

This leaves the bridge in the global list indefinitely. Furthermore, because
the parent meson_drm structure is subsequently freed by devm upon probe
failure, meson_encoder_hdmi->priv becomes a dangling pointer. If another
driver searches the list via of_drm_find_and_get_bridge() and calls an op on
this bridge, it could dereference the freed priv structure, causing a
use-after-free crash.

Should drm_bridge_remove() be called on this and other error paths?

>  	}

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic