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

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

New issues:
- [Low] The commit message claims to inline `drm_simple_encoder_init()` for the meson driver, but it only modifies the HDMI encoder, leaving the deprecated helper in use by other encoders in the same driver.

Pre-existing issues:
- [High] The `meson_encoder_hdmi` structure (which embeds `struct drm_encoder`) is allocated with `devm_drm_bridge_alloc()`, but the encoder is initialized with `drm_encoder_init()`. This creates a mismatch in lifecycles that leads to a use-after-free when the platform device is unbound but userspace still holds a reference to the DRM device.
--

--- Patch [1]: [PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init() ---
commit bb202202974455054588168b82acfc49ff85b23f
Author: Liam Zuiderhoek <[email protected]>

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. It replaces the simple encoder init call with drm_encoder_init
> and a custom cleanup struct.

[Severity: Low]
Does this commit miss updating other encoders in the meson driver?

The commit message implies it inlines the function for the meson driver as a
whole, but meson_encoder_cvbs_probe() in meson_encoder_cvbs.c and
meson_encoder_dsi_probe() in meson_encoder_dsi.c still use
drm_simple_encoder_init().

Additionally, is the include for <drm/drm_simple_kms_helper.h> still needed
in meson_encoder_hdmi.c now that the helper has been open-coded?

> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 0c7a72cb514ae..70a543358041f 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);

[Severity: High]
This is a pre-existing issue, but does this allocation lifecycle mismatch
lead to a use-after-free?

Looking at meson_encoder_hdmi_probe(), the structure embedding the encoder is
allocated with devm_drm_bridge_alloc(), tying its lifetime to the platform
device:

meson_encoder_hdmi_probe() {
    ...
    meson_encoder_hdmi = devm_drm_bridge_alloc(priv->dev, ...);
    ...
}

However, the encoder is initialized here with drm_encoder_init(), tying its
cleanup to the DRM device's release.

If the platform device is unbound, devres automatically frees
meson_encoder_hdmi. If a userspace process holds an open file descriptor to
the DRM device during unbind, the DRM device outlives the platform device.

When the userspace process eventually closes the fd, drm_dev_release()
calls drm_mode_config_cleanup(), which iterates over the mode config list
and accesses the freed meson_encoder_hdmi->encoder to call its destroy
callback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic