Re: [PATCH] drm/amd/display: Fix dereference-before-check for dc_link

["Timur Kristóf" <timur.kristof@gmail.com>]

On Fri, 2025-11-21 at 20:32 +0530, Srinivasan Shanmugam wrote:
> The function dereferences amdgpu_dm_connector->dc_link early to
> initialize verified_link_cap and dc, but later still checks
> amdgpu_dm_connector->dc_link for NULL in the analog path.
> 
> This late NULL check is redundant, introduce a local dc_link pointer,
> use it consistently, and drop the superfluous NULL check while using
> dc_link->link_id.id instead.
> 
> The function uses dc_link at the very beginning without checking if
> it
> is NULL.  But later in the code, it suddenly checks if dc_link is
> NULL.
> 
> This check is too late to be useful, because the code has already
> used
> dc_link earlier.  So this NULL check does nothing.
> 
> We simplify the code by storing amdgpu_dm_connector->dc_link in a
> local
> dc_link variable and using it throughout the function.  Since dc_link
> is
> already dereferenced early, the later NULL check is unnecessary and
> is
> removed.
> 
> Fixes the below:
>   amdgpu_dm_connector_get_modes():
>   variable dereferenced before check 'amdgpu_dm_connector->dc_link'
> 
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c
>   8845				&amdgpu_dm_connector->dc_link-
> >verified_link_cap;
>   8846		const struct dc *dc = amdgpu_dm_connector->dc_link-
> >dc;
>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                                       Dereference
>   ...
> 
>   8856
>   8857			if (amdgpu_dm_connector->dc_sink &&
>   8858			    amdgpu_dm_connector->dc_link &&
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                             Checked too late.
>                             Presumably this NULL check could be
> removed?
>   ...
> 
> Fixes: c8f52ca2cefb ("drm/amd/display: Cleanup uses of the analog
> flag")
> Reported by: Dan Carpenter <dan.carpenter@linaro.org>
> Cc: Timur Kristóf <timur.kristof@gmail.com>
> Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Cc: Roman Li <roman.li@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Tom Chung <chiahsuan.chung@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>

Nice catch!

Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>


> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 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 8a0555365719..6eb88a96cc33 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8818,11 +8818,11 @@ static int
> amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
> +	struct dc_link *dc_link = amdgpu_dm_connector->dc_link;
>  	struct drm_encoder *encoder;
>  	const struct drm_edid *drm_edid = amdgpu_dm_connector-
> >drm_edid;
> -	struct dc_link_settings *verified_link_cap =
> -			&amdgpu_dm_connector->dc_link-
> >verified_link_cap;
> -	const struct dc *dc = amdgpu_dm_connector->dc_link->dc;
> +	struct dc_link_settings *verified_link_cap = &dc_link-
> >verified_link_cap;
> +	const struct dc *dc = dc_link->dc;
>  
>  	encoder = amdgpu_dm_connector_to_encoder(connector);
>  
> @@ -8834,9 +8834,8 @@ static int amdgpu_dm_connector_get_modes(struct
> drm_connector *connector)
>  				drm_add_modes_noedid(connector,
> 1920, 1080);
>  
>  		if (amdgpu_dm_connector->dc_sink &&
> -		    amdgpu_dm_connector->dc_link &&
>  		    amdgpu_dm_connector->dc_sink->edid_caps.analog
> &&
> -		   
> dc_connector_supports_analog(amdgpu_dm_connector->dc_link-
> >link_id.id)) {
> +		    dc_connector_supports_analog(dc_link-
> >link_id.id)) {
>  			/* Analog monitor connected by DAC load
> detection.
>  			 * Add common modes. It will be up to the
> user to select one that works.
>  			 */