Re: [PATCH 1/2] drm/amd/display: Fall back to the native mode, not the first, on eDP/LVDS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm: uninitialized stack memory read in create_stream_for_sink()
--

commit c9acc168fa3f685b9a0bbaf87f6642a89b782aef
Author: Hugo Sales <hugo@hsal.es>
drm/amd/display: Fall back to the native mode, not the first, on eDP/LVDS

This commit updates create_stream_for_sink() to fall back to the native
mode stored in the encoder when the preferred mode is no longer present
in the connector's mode list. This is limited to eDP and LVDS panels where
amdgpu handles scaling automatically.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/5983

> 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 428a5f9377c62..a1dc3e28764cf 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
> @@ -1470,6 +1472,7 @@ create_stream_for_sink(struct drm_connector *connector,
>  	struct dc_stream_state *stream = NULL;
>  	struct drm_display_mode mode;
>  	struct drm_display_mode saved_mode;
> +	struct drm_display_mode native_mode;
>  	struct drm_display_mode *freesync_mode = NULL;
>  	bool native_mode_found = false;
>  	bool recalculate_timing = false;

[ ... ]

> @@ -1529,6 +1532,31 @@ create_stream_for_sink(struct drm_connector *connector,
>  			break;
>  		}
>  	}
> +
> +	/*
> +	 * After a probe with size limits smaller than the preferred mode,
> +	 * connector->modes no longer has that mode, and the first mode left
> +	 * would become the stream timing. fbdev emulation probes with limits
> +	 * that video= can set below the native size. The encoder has a copy of
> +	 * the preferred mode from amdgpu_dm_get_native_mode(), made on every
> +	 * probe before the limits apply, so use that copy. Only do this for eDP
> +	 * and LVDS panels, which take only their native timing. On other
> +	 * connectors mode_valid can also reject the preferred mode, and then
> +	 * the first valid mode is the right one
> +	 */
> +	if (!native_mode_found && aconnector && !aconnector->mst_root && aconnector->drm_edid &&
> +	    (connector->connector_type == DRM_MODE_CONNECTOR_eDP ||
> +	     connector->connector_type == DRM_MODE_CONNECTOR_LVDS)) {
> +		struct drm_encoder *encoder = amdgpu_dm_connector_to_encoder(connector);
> +
> +		if (encoder && to_amdgpu_encoder(encoder)->native_mode.clock) {
> +			drm_mode_copy(&native_mode, &to_amdgpu_encoder(encoder)->native_mode);

[Severity: Medium]
Does this result in an uninitialized stack memory read?

The native_mode variable is declared on the stack without initialization.
When passed to drm_mode_copy(), that function starts by attempting to
preserve the existing list node pointers of the destination:

void drm_mode_copy(struct drm_display_mode *dst, 
                   const struct drm_display_mode *src)
{
        struct list_head head = dst->head;

        *dst = *src;
        dst->head = head;
}

Because native_mode is uninitialized, reading dst->head accesses garbage
data. While this won't corrupt an active list (since the garbage is written
back to a local variable that is never linked), it still triggers undefined
behavior and can cause Kernel Memory Sanitizer (KMSAN) warnings.

Could we initialize native_mode when it is declared to avoid this?
For example: struct drm_display_mode native_mode = { 0 };

> +			drm_mode_set_crtcinfo(&native_mode, CRTC_INTERLACE_HALVE_V);
> +			preferred_mode = &native_mode;
> +			native_mode_found = true;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014223041.171317-1-hugo@hsal.es?part=1