Re: [PATCH 66/75] drm/amdgpu: dm: Convert to atomic_create_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/amdgpu: memory leak and ERR_PTR dereference in dm_dp_add_mst_connector()
- [High] drm/amdgpu: early return on OOM skips mandatory connector initialization
--

commit 7f186f384d1ae8ab956532945397b878e65c8386
Author: Maxime Ripard <mripard@kernel.org>

drm/amdgpu: dm: Convert to atomic_create_state

The amdgpu_dm connector reset implementation creates a custom state
subclass, but only initializes a pristine state without resetting any
hardware. This is equivalent to what atomic_create_state expects.
Convert to it.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> index 3a4e98b08ac1f..2c22f00f521c6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c
[ ... ]
> @@ -3199,8 +3195,16 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>  	 * Some of the properties below require access to state, like bpc.
>  	 * Allocate some default initial connector state with our reset helper.
>  	 */
> -	if (aconnector->base.funcs->reset)
> -		aconnector->base.funcs->reset(&aconnector->base);
> +	if (!aconnector->base.state &&
> +	    aconnector->base.funcs->atomic_create_state) {
> +		struct drm_connector_state *conn_state;
> +
> +		conn_state = aconnector->base.funcs->atomic_create_state(&aconnector->base);
> +		if (IS_ERR(conn_state))
> +			return;

[Severity: High]
If memory allocation fails during atomic_create_state() and we return early
here, does it skip mandatory connector initialization?

Since amdgpu_dm_connector_init_helper() returns void, the caller
amdgpu_dm_connector_init() is unaware of the failure and continues execution.
It subsequently calls amdgpu_dm_initialize_dp_connector(), which
unconditionally dereferences aconnector->dc_link->ddc.

Since aconnector->dc_link assignment is skipped in the aborted helper, could
this result in a NULL pointer dereference?

[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index a5b15bef8aa0a..cb189c098ec3e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
[ ... ]
> @@ -738,7 +738,7 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>  	 * Initialize connector state before adding the connectror to drm and
>  	 * framebuffer lists
>  	 */
> -	amdgpu_dm_connector_funcs_reset(connector);
> +	connector->state = amdgpu_dm_connector_atomic_create_state(connector);

[Severity: High]
Can this overwrite the connector->state pointer and leak the previously
allocated state?

Earlier in dm_dp_add_mst_connector(), amdgpu_dm_connector_init_helper() is
called, which allocates and initializes a new connector state and assigns it
to connector->state. Overwriting it here appears to drop custom properties
attached to the state in the interim (such as max_requested_bpc set by
drm_connector_attach_max_bpc_property).

Additionally, amdgpu_dm_connector_atomic_create_state() can return
ERR_PTR(-ENOMEM) on allocation failure. Because this is directly assigned to
connector->state without an IS_ERR() check, will the DRM core subsequently
dereference this error pointer?

>  
>  	drm_dp_mst_get_port_malloc(port);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-drm-no-more-connector-reset-v1-0-586c9c026823@kernel.org?part=66