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

[Chung, ChiaHsuan (Tom)]

The original code will check the drm_new_conn_state if it's valid in here

10712                 if (IS_ERR(drm_new_conn_state)) {

After that the drm_new_conn_state does not touch by anyone before call the

--> 10751                 ret = fill_hdr_info_packet(drm_new_conn_state,

I think it should be no issue in this case.

We call the PTR_ERR_OR_ZERO() just because we need to get the error code 
and return to the caller.

  10713                         ret = PTR_ERR_OR_ZERO(drm_new_conn_state);

Maybe it's just a false warning?

Tom

On 3/12/2025 10:34 AM, 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);
>   
> @@ -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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20250312/06d1531a/attachment-0001.htm>