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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-free during drm_encoder_cleanup() due to bridge refcount dropping to 0.
--

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

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

The helper drm_simple_encoder_init() is replaced with an open-coded
drm_encoder_init() to remove the dependency on drm_simple_kms_helper.
A simple destroy-only encoder funcs struct is added.

> diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> index 8b26a0031cde0..9797e3f57e0be 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
[ ... ]
> @@ -257,8 +260,9 @@ int meson_encoder_cvbs_probe(struct meson_drm *priv)
>  	meson_encoder_cvbs->priv = priv;
>  
>  	/* Encoder */
> -	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_cvbs->encoder,
> -				      DRM_MODE_ENCODER_TVDAC);
> +	ret = drm_encoder_init(priv->drm, &meson_encoder_cvbs->encoder,
> +			       &meson_encoder_cvbs_drm_encoder_funcs,
> +			       DRM_MODE_ENCODER_TVDAC, NULL);

[Severity: High]
This is a pre-existing issue, but does tying the encoder cleanup directly to
drm_encoder_cleanup() lead to a use-after-free during device unbind?

If the driver is unbound while userspace holds an open DRM device
reference, the platform unbind will drop the devm reference to the
meson_encoder_cvbs bridge container. The bridge is then kept alive
only by the attach reference.

When userspace finally closes the file descriptor, drm_mode_config_cleanup()
calls the encoder's destroy hook, which maps to drm_encoder_cleanup().

Inside drm_encoder_cleanup(), it drops the final bridge reference:

drm_encoder.c:drm_encoder_cleanup() {
    ...
    list_for_each_entry_safe(bridge, next, &tmplist, chain_node)
        drm_bridge_detach(bridge);
    ...

This frees the meson_encoder_cvbs container. However, drm_encoder_cleanup()
continues to access the embedded encoder:

    drm_mode_object_unregister(dev, &encoder->base);
    kfree(encoder->name);
    list_del(&encoder->head);
    ...
    memset(encoder, 0, sizeof(*encoder));
}

Could this sequence corrupt memory when it accesses the already freed
encoder structure?

>  	if (ret)
>  		return dev_err_probe(priv->dev, ret,
>  				     "Failed to init CVBS encoder\n");

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

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