Re: [PATCH v2 11/15] drm/hisilicon/kirin: remove dependency on DRM simple helpers

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

Pre-existing issues:
- [Critical] The `dsi_data` structure, which embeds the `drm_encoder` object via `dw_dsi`, is allocated using `devm_kzalloc`. This violates DRM lifetime rules and leads to a use-after-free when the platform device unbinds.
--

--- Patch [11]: [PATCH v2 11/15] drm/hisilicon/kirin: remove dependency on DRM simple helpers ---
commit b9a0117f6ac1e860030dfb53be58e44730659ad8
Author: Diogo Silva <[email protected]>

drm/hisilicon/kirin: remove dependency on DRM simple helpers

This patch removes the deprecated simple KMS helpers by open-coding
drm_simple_encoder_init() to call drm_encoder_init() directly and
providing driver-local drm_encoder_funcs.

> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> index 15042365dec00..62c5bd3277da1 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c

[ ... ]

> @@ -687,6 +687,10 @@ static int dsi_encoder_atomic_check(struct drm_encoder *encoder,
>  	return 0;
>  }
>  
> +static const struct drm_encoder_funcs dw_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,

[Severity: Critical]
This is a pre-existing issue, but could this setup lead to a use-after-free
when the platform device unbinds?

The encoder is embedded in the dsi_data structure, which is allocated
using devm_kzalloc() in dsi_probe():

drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c:dsi_probe() {
    ...
    data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
    ...
}

If userspace holds an open file descriptor when the platform device unbinds,
the DRM device and its mode objects will outlive the platform device. The
devres cleanup will free the dsi_data memory, but the DRM device will still
hold a pointer to the encoder in its internal lists.

When the DRM device is finally released, drm_mode_config_cleanup() will
iterate over its encoder list and invoke this .destroy callback
(drm_encoder_cleanup) on the already-freed memory.

Since you are refactoring the encoder initialization here, would this be a
good time to transition to drmm_encoder_alloc() or drmm_kzalloc() to tie the
allocation lifetime to the DRM device instead of the platform device?

> +};
> +
>  static const struct drm_encoder_helper_funcs dw_encoder_helper_funcs = {
>  	.atomic_check	= dsi_encoder_atomic_check,
>  	.mode_valid	= dsi_encoder_mode_valid,

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

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