Re: [PATCH v2 40/61] drm/amdgpu_dm: Convert to atomic_create_state

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

Pre-existing issues:
- [High] Missing drm_plane_cleanup() on the error path in amdgpu_dm_plane_init() causes a Use-After-Free of the plane object when the caller frees the plane.
--

--- Patch [40]: [PATCH v2 40/61] drm/amdgpu_dm: Convert to atomic_create_state ---
commit 6612d2468dd0e4946eb21b27b9d60498ca111985
Author: Maxime Ripard <[email protected]>

drm/amdgpu_dm: Convert to atomic_create_state

This commit converts the plane reset implementation to use the
atomic_create_state hook. A custom state subclass is initialized
without resetting any hardware. The conversion was performed via a
Coccinelle semantic patch.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c

[ ... ]

> @@ -1993,9 +1992,16 @@ int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  		return res;
>  #endif
>  
> -	/* Create (reset) the plane state */
> -	if (plane->funcs->reset)
> -		plane->funcs->reset(plane);
> +	/* Create the plane state */
> +	if (plane->funcs->atomic_create_state) {
> +		struct drm_plane_state *plane_state;
> +
> +		plane_state = plane->funcs->atomic_create_state(plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);

[Severity: High]
This is a pre-existing issue, but I wanted to ask about this error path.
If plane->funcs->atomic_create_state() fails and returns an error pointer,
we return the error directly without calling drm_plane_cleanup().

Since drm_universal_plane_init() was called earlier in this function, the
plane is already linked into the mode_config.plane_list. If we return an
error here, the caller initialize_plane() will free the plane memory:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:initialize_plane() {
    ...
    if (ret) {
        drm_err(adev_to_drm(dm->adev), "KMS: Failed to initialize plane\n");
        kfree(plane);
        return ret;
    }
    ...
}

Does this leave a freed pointer on the plane list that could cause a
use-after-free later during device teardown in drm_mode_config_cleanup()?

While this problem already existed for dm_plane_init_colorops() failures,
it appears this patch might expose it to a new allocation failure path.

> +
> +		plane->state = plane_state;
> +	}
>  
>  	return 0;
>  }

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