Re: [PATCH v3 06/40] drm/hisilicon/kirin: Convert to atomic_create_state

[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 driver cleanup due to premature freeing of DRM mode objects embedded in `struct kirin_drm_private`.
--

commit 263cd9f02efa601fc3d304e38a61e0c61ac2a73e
Author: Maxime Ripard <mripard@kernel.org>

drm/hisilicon/kirin: Convert to atomic_create_state

The plane only initializes a pristine state in its reset hook
using drm_atomic_helper_plane_reset(), which is equivalent to what
atomic_create_state expects. Convert to it.

[ ... ]

> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index 5f4e738bc94e3..bca839dd600bd 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -837,7 +837,7 @@ static struct drm_plane_funcs ade_plane_funcs = {
>  	.update_plane	= drm_atomic_helper_update_plane,
>  	.disable_plane	= drm_atomic_helper_disable_plane,
>  	.destroy = drm_plane_cleanup,
> -	.reset = drm_atomic_helper_plane_reset,
> +	.atomic_create_state = drm_atomic_helper_plane_create_state,
>  	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>  };

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the kirin DRM
driver, I noticed a pre-existing use-after-free issue during driver cleanup.

In drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c,
kirin_drm_private_cleanup() frees the memory backing the DRM planes and CRTCs:

drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c:kirin_drm_private_cleanup() {
    ...
	devm_kfree(dev->dev, kirin_priv);
    ...
}

However, both the initialization error path and the unbind path call this
cleanup before drm_mode_config_cleanup():

drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c:kirin_drm_kms_init() {
    ...
err_private_cleanup:
	kirin_drm_private_cleanup(dev);
err_mode_config_cleanup:
	drm_mode_config_cleanup(dev);
    ...
}

The same pattern appears in the unbind path:

drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c:kirin_drm_kms_cleanup() {
    ...
	kirin_drm_private_cleanup(dev);
	drm_mode_config_cleanup(dev);
    ...
}

Can this lead to a use-after-free? When drm_mode_config_cleanup() executes, it
iterates over dev->mode_config.plane_list and accesses the plane objects
embedded in the kirin_priv structure that was just freed by devm_kfree().

Additionally, since the structure is devm-managed, could userspace holding the
DRM file descriptor open trigger a use-after-free if the DRM core accesses the
freed objects after unbind completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-drm-no-more-plane-reset-v3-0-1877c7aa57b3@kernel.org?part=6