[PATCH] drm/amd/display: Fix NULL pointer dereferences in dm_update_crtc_state()

[Aurabindo Pillai]

On 2025-03-11 22:34, Srinivasan Shanmugam wrote:
> Added checks for NULL values after retrieving drm_new_conn_state and
> drm_old_conn_state to prevent dereferencing NULL pointers.
> 
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:10751 dm_update_crtc_state()
> 	warn: 'drm_new_conn_state' can also be NULL
> 
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c
>      10672 static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>      10673                          struct drm_atomic_state *state,
>      10674                          struct drm_crtc *crtc,
>      10675                          struct drm_crtc_state *old_crtc_state,
>      10676                          struct drm_crtc_state *new_crtc_state,
>      10677                          bool enable,
>      10678                          bool *lock_and_validation_needed)
>      10679 {
>      10680         struct dm_atomic_state *dm_state = NULL;
>      10681         struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
>      10682         struct dc_stream_state *new_stream;
>      10683         int ret = 0;
>      10684
>      ...
>      10703
>      10704         /* TODO This hack should go away */
>      10705         if (connector && enable) {
>      10706                 /* Make sure fake sink is created in plug-in scenario */
>      10707                 drm_new_conn_state = drm_atomic_get_new_connector_state(state,
>      10708                                                                         connector);
> 
> drm_atomic_get_new_connector_state() can't return error pointers, only NULL.
> 
>      10709                 drm_old_conn_state = drm_atomic_get_old_connector_state(state,
>      10710                                                                         connector);
>      10711
>      10712                 if (IS_ERR(drm_new_conn_state)) {
>                                       ^^^^^^^^^^^^^^^^^^
> 
>      10713                         ret = PTR_ERR_OR_ZERO(drm_new_conn_state);
> 
> Calling PTR_ERR_OR_ZERO() doesn't make sense.  It can't be success.
> 
>      10714                         goto fail;
>      10715                 }
>      10716
>      ...
>      10748
>      10749                 dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
>      10750
> --> 10751                 ret = fill_hdr_info_packet(drm_new_conn_state,
>                                                       ^^^^^^^^^^^^^^^^^^ Unchecked dereference
> 
>      10752                                            &new_stream->hdr_static_metadata);
>      10753                 if (ret)
>      10754                         goto fail;
>      10755
> 
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> Cc: Tom Chung <chiahsuan.chung at amd.com>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
> Cc: Roman Li <roman.li at amd.com>
> Cc: Alex Hung <alex.hung at amd.com>
> Cc: Aurabindo Pillai <aurabindo.pillai at amd.com>
> Reported-by: Dan Carpenter <dan.carpenter at linaro.org>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 +++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1b92930119cc..e3df11662fff 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10727,11 +10727,20 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>   		drm_old_conn_state = drm_atomic_get_old_connector_state(state,
>   									connector);
>   
> -		if (IS_ERR(drm_new_conn_state)) {
> -			ret = PTR_ERR_OR_ZERO(drm_new_conn_state);
> -			goto fail;
> +		/* Check if drm_new_conn_state is valid */
> +		if (drm_new_conn_state) {
> +			dm_new_conn_state = to_dm_connector_state(drm_new_conn_state);
> +
> +			/* Attempt to fill HDR info packet */
> +			ret = fill_hdr_info_packet(drm_new_conn_state,
> +						   &new_stream->hdr_static_metadata);
> +			if (ret)
> +				goto fail;
>   		}
>   
> +		if (drm_old_conn_state)
> +			dm_old_conn_state = to_dm_connector_state(drm_old_conn_state);
> +
>   		dm_new_conn_state = to_dm_connector_state(drm_new_conn_state);
>   		dm_old_conn_state = to_dm_connector_state(drm_old_conn_state);

This line above becomes a duplicate now. With that removed, the patch is:

Reviewed-by: Aurabindo Pillai <aurabindo.pillai at amd.com>
>   
> @@ -10766,11 +10775,6 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>   
>   		dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
>   
> -		ret = fill_hdr_info_packet(drm_new_conn_state,
> -					   &new_stream->hdr_static_metadata);
> -		if (ret)
> -			goto fail;
> -
>   		/*
>   		 * If we already removed the old stream from the context
>   		 * (and set the new stream to NULL) then we can't reuse

-- 
Thanks & Regards,
Aurabindo Pillai