Re: [PATCH 1/2] drm/client: Probe connectors with the device's mode size limits

Re: [PATCH 1/2] drm/client: Probe connectors with the device's mode size limits

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: NULL pointer dereference of modes[i] in drm_client_firmware_config
- [Medium] drm: Logical bug in drm_client_target_cloned due to stale modes[i] values
--

commit 0e7e38749489efdbc43f11da085113315e670013
Author: Hugo Sales <hugo@hsal.es>

drm/client: Probe connectors with the device's mode size limits

This patch updates drm_client_modeset_probe() to use the device's mode
size limits rather than the client's dimensions when probing connectors.
It shifts the size validation to the mode pickers by calling
drm_mode_validate_size() to ensure the shared modes list retains larger
modes. This allows keeping panels' preferred modes even if smaller modes
are initially selected.

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

> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 0080a8e9556a5..e79c7257aaa2e 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -357,6 +370,8 @@ static bool drm_client_target_cloned(struct drm_device *dev,
>  			continue;
>  
>  		list_for_each_entry(mode, &connectors[i]->modes, head) {
> +			if (drm_mode_validate_size(mode, width, height) != MODE_OK)
> +				continue;
>  			if (drm_mode_match(mode, dmt_mode,
>  					   DRM_MODE_MATCH_TIMINGS |
>  					   DRM_MODE_MATCH_CLOCK |

[Severity: Medium]
Can this result in a stale modes[i] value being incorrectly accepted?

In drm_client_target_cloned(), the driver tries to find a matching 1024x768 
mode across all connectors if command-line cloning fails. 

If a connector doesn't have a 1024x768 mode, or if the mode is skipped 
because it exceeds the width and height constraints added by this patch, 
mode_replace() is bypassed:

		list_for_each_entry(mode, &connectors[i]->modes, head) {
			if (drm_mode_validate_size(mode, width, height) != MODE_OK)
				continue;
			if (drm_mode_match(mode, dmt_mode, ...))
				mode_replace(dev, &modes[i], mode);
		}
		if (!modes[i])
			can_clone = false;

Because modes[i] is not cleared before this loop, it retains whatever value 
it was assigned in the earlier command-line pass. The termination check 
if (!modes[i]) evaluates to false, and can_clone incorrectly remains true 
even if heterogeneous modes are present. 

While this is a preexisting structural flaw in the function, does adding 
the drm_mode_validate_size() check inside the 1024x768 loop trigger it by 
making it far more likely for a 1024x768 mode to be skipped?

[ ... ]

> @@ -750,7 +767,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>  		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
>  			mode_type = "non tiled";
>  			mode_replace(dev, &modes[i],
> -				     drm_connector_fallback_non_tiled_mode(connector));
> +				     drm_connector_fallback_non_tiled_mode(connector,
> +									   width, height));
>  		}
>  		crtcs[i] = crtc;

[Severity: High]
Does this code risk a NULL pointer dereference in drm_client_firmware_config()?

If a connector is part of a tiled display but some tiles are missing, the 
driver attempts to fall back to a non-tiled mode by calling 
drm_connector_fallback_non_tiled_mode(). 

This patch modifies the fallback function to filter out any modes that exceed 
the client's width and height. If no modes fit these constraints, the function 
returns NULL. 

When this happens, mode_replace() safely destroys the existing mode and 
assigns NULL to modes[i]. However, immediately following this block, 
drm_dbg_kms() unconditionally dereferences modes[i]->name when logging 
the selected mode:

		crtcs[i] = crtc;

		drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
			    connector->base.id, connector->name,
			    crtc->base.id, crtc->name,
			    mode_type, modes[i]->name);

Since the new size filter makes returning NULL far more likely, can this 
cause a crash if modes[i] becomes NULL?

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

Re: [PATCH 2/2] drm/tests: Add KUnit tests for the client mode size limits

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] drm/tests: lockless access to connector->modes violates locking rules
- [Low] drm/tests: memory leak of cloned modes on KUnit assertion failure
--

commit 806dc20896099436a3d060ce16a4a8cb84cf8467
Author: Hugo Sales <hugo@hsal.es>
Subject: drm/tests: Add KUnit tests for the client mode size limits

This commit adds KUnit tests for various client mode pickers to ensure they
skip modes larger than the client's size. It also tests that the modeset
probe properly retains the preferred mode.

> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> index 00f43fca97954..93aa919d4f317 100644
> --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c

[ ... ]

> @@ -197,6 +213,206 @@ static struct kunit_suite drm_test_pick_cmdline_test_suite = {
>  
>  kunit_test_suite(drm_test_pick_cmdline_test_suite);
>  
> +static void drm_test_pick_first_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_first_mode(connector, 1920, 1080);

[Severity: Low]
The connector->modes list is explicitly documented as being protected by
dev->mode_config.mutex. Here, the mutex is unlocked just before calling
drm_connector_first_mode(), which iterates over connector->modes.

While this is isolated test code and no concurrent modification occurs,
could this lockless iteration trigger static analysis tools or lockdep
warnings?

> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1920);
> +	KUNIT_EXPECT_EQ(test, mode->vdisplay, 1080);

[ ... ]

> +static void drm_test_pick_cmdline_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_pick_cmdline_mode(connector, 1920, 1080);

[Severity: Low]
Similarly here, drm_connector_pick_cmdline_mode() iterates over
connector->modes outside of drm->mode_config.mutex. Does this violate
the locking rules for the connector modes list?

> +	KUNIT_ASSERT_NOT_NULL(test, mode);

[ ... ]

> +static void drm_test_pick_tiled_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_get_tiled_mode(connector, 1920, 1080);

[Severity: Low]
As with the previous pickers, drm_connector_get_tiled_mode() accesses
connector->modes without the required drm->mode_config.mutex held.

> +	KUNIT_ASSERT_NOT_NULL(test, mode);

[ ... ]

> +static void drm_test_pick_clone_mode_within_limits(struct kunit *test)
> +{

[ ... ]

> +	mutex_lock(&drm->mode_config.mutex);
> +	for (i = 0; i < ARRAY_SIZE(connectors); i++) {
> +		ret = drm_helper_probe_single_connector_modes(connectors[i], 1920, 1080);
> +		KUNIT_EXPECT_GT(test, ret, 0);
> +	}
> +	mutex_unlock(&drm->mode_config.mutex);
> +
> +	KUNIT_EXPECT_TRUE(test, drm_client_target_cloned(drm, connectors, ARRAY_SIZE(connectors),
> +							 modes, offsets, enabled, 1024, 768));

[Severity: Low]
This call to drm_client_target_cloned() also iterates over
connector->modes after the config mutex has been released.

> +	for (i = 0; i < ARRAY_SIZE(connectors); i++) {
> +		KUNIT_ASSERT_NOT_NULL(test, modes[i]);

[Severity: Low]
If this KUNIT_ASSERT_NOT_NULL() assertion fails, it immediately forces an
early return from the test.

Because drm_client_target_cloned() allocates new modes via mode_replace(),
would an early return here bypass the mandatory modes_destroy() cleanup
below and leak the dynamically allocated mode objects?

Could KUnit's managed allocations (like kunit_add_action) be used to ensure
modes_destroy() always runs on cleanup?

> +		KUNIT_EXPECT_EQ(test, modes[i]->hdisplay, 1024);
> +		KUNIT_EXPECT_EQ(test, modes[i]->vdisplay, 768);
> +	}
> +	modes_destroy(drm, modes, ARRAY_SIZE(connectors));

[ ... ]

> +static void drm_test_probe_keeps_modes_above_limits(struct kunit *test)
> +{

[ ... ]

> +	ret = drm_client_modeset_probe(client, 1024, 768);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	mode = client->modesets[0].mode;
> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +	KUNIT_EXPECT_EQ(test, mode->hdisplay, 1024);
> +	KUNIT_EXPECT_EQ(test, mode->vdisplay, 768);
> +
> +	mode = drm_connector_preferred_mode(connector, 1920, 1080);

[Severity: Low]
Once again, drm_connector_preferred_mode() is called to iterate the
connector->modes list without the config mutex held.

> +	KUNIT_ASSERT_NOT_NULL(test, mode);

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