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

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

[sashiko-bot]

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

Re: [PATCH 2/2] drm/amd/display: Add KUnit tests for the eDP/LVDS native mode fallback

[sashiko-bot]

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

commit 852bbf13f64b2d512665604270ff18f1f920af2e
Author: Hugo Sales <hugo@hsal.es>

drm/amd/display: Add KUnit tests for the eDP/LVDS native mode fallback

Leave only a scaled mode in connector->modes and put the larger native
mode in the encoder, as a size-limited probe does. On eDP and LVDS,
create_stream_for_sink() must build a stream with the native timing and
the scaled mode as its source size. On HDMI it must keep the first mode
in the list as the timing.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
> index 56fd133bc4056..55e2c38499f4a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
> @@ -4547,6 +4547,115 @@ static void dm_test_create_stream_hdmi_vsif(struct kunit *test)
[ ... ]
> +static struct dc_stream_state *
> +dm_test_native_mode_pruned_stream(struct kunit *test, int connector_type)
> +{
[ ... ]
> +	stream = create_stream_for_sink(&ctx->aconnector->base, ctx->mode,
> +					ctx->dm_state, NULL, 8,
> +					PIXEL_ENCODING_RGB, false);

[Severity: Medium]
Does this call to create_stream_for_sink() trigger a read of uninitialized
stack memory?

In create_stream_for_sink(), the variable native_mode is declared on the
stack without initialization:

amdgpu_dm_connector.c:create_stream_for_sink() {
	...
	struct drm_display_mode native_mode;
	...
	if (!native_mode_found && aconnector && ...) {
		...
		drm_mode_copy(&native_mode, &to_amdgpu_encoder(encoder)->native_mode);
		...
	}
}

The drm_mode_copy() function begins by saving the destination's list head:

drm_modes.c:drm_mode_copy() {
	struct list_head head = dst->head;
	...
}

Because native_mode is uninitialized, this reads uninitialized stack memory,
which could trigger KMSAN panics on systems with memory sanitizers enabled.
Should native_mode be zero-initialized or initialized using drm_mode_init()
first?

> +
> +	drm_edid_free(ctx->aconnector->drm_edid);
> +	KUNIT_ASSERT_NOT_NULL(test, stream);
> +
> +	return stream;
> +}

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