Re: [PATCH v8 08/14] drm/amd/display: use drm backlight

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional creation of the DRM LUMINANCE property breaks brightness control on systems relying on ACPI video backlight.
- [High] Lockless access to the global `crtc->state` pointer during brightness-only commits leads to a Use-After-Free (UAF) and race condition.
- [High] TOCTOU race when accessing `dm->dc->current_state` leads to a potential NULL pointer dereference or Use-After-Free.
--

commit 2a535fc7f658113715bee173a4202a46e91b7048
Author: Mario Limonciello <mario.limonciello@amd.com>

drm/amd/display: use drm backlight

This commit converts the AMD display driver to use the DRM backlight
infrastructure, exposing brightness control through the connector LUMINANCE
property.

> 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 2fe934036e363..f7b1eaf34b59d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -5050,10 +5050,21 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
>  			old_crtc_state = drm_atomic_get_old_crtc_state(state, &acrtc->base);
>  		}
>  
> +		/*
> +		 * Apply the LUMINANCE property first: a brightness-only commit
> +		 * does not pull the CRTC into the atomic state (so new_crtc_state
> +		 * is NULL), and a modeset is skipped below - in both cases the
> +		 * backlight must still follow the requested luminance.
> +		 */
> +		drm_atomic_helper_connector_apply_luminance(new_con_state);

[Severity: High]
Does this introduce a race condition leading to a use-after-free?

Since the CRTC is intentionally not pulled into the atomic state during a
brightness-only commit, the CRTC lock is not held in this path. However,
drm_atomic_helper_connector_apply_luminance() dereferences the global
conn_state->crtc->state->active property locklessly.

If a concurrent modeset or display disable commit acquires the lock,
swaps the state, and frees the old one, this non-blocking brightness commit
could end up reading freed memory. Is it safe to call this helper without
ensuring the CRTC lock is held?

[ ... ]
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index 406a821d03cd6..1fefe3837e4cf 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
[ ... ]
> @@ -224,13 +225,17 @@ struct dc_stream_state *dm_find_stream_with_link(
>  	struct amdgpu_display_manager *dm,
>  	struct dc_link *link)
>  {
> -	struct dc_state *cur_dc_state = dm->dc->current_state;
>  	struct dc_stream_state *stream = NULL;
>  	int i;
>  
> +	if (!dm || !dm->dc || !dm->dc->current_state || !link)
> +		return NULL;
> +
> +	cur_dc_state = dm->dc->current_state;

[Severity: High]
Can this result in a double-fetch/TOCTOU race?

This code checks that dm->dc->current_state is not NULL, but then immediately
re-reads the pointer from dm->dc->current_state. 

If a concurrent modeset or teardown updates the state between the check and
the assignment, cur_dc_state could be NULL or point to freed memory when it
is subsequently dereferenced in the loop below. 

Should this be changed to read the state pointer once into cur_dc_state,
and then perform the NULL check on the local variable?

[ ... ]
> @@ -600,6 +610,13 @@ void amdgpu_dm_setup_backlight_device(struct amdgpu_display_manager *dm,
>  
>  	amdgpu_dm_update_connector_ext_caps(aconnector);
>  
> +	caps = &dm->backlight_caps[bl_idx];
> +	amdgpu_dm_backlight_fill_props(caps, power_supply_is_system_supplied() > 0,
> +				       !(amdgpu_dc_debug_mask &
> +					 DC_DISABLE_CUSTOM_BRIGHTNESS_CURVE),
> +				       &props);
> +	drm_backlight_create_property(&aconnector->base, props.max_brightness, false);

[Severity: High]
Does unconditionally creating this property break brightness control on
systems relying on ACPI video backlight?

By creating the LUMINANCE property here, we expose it to userspace before
verifying if the native backlight backend will actually be used. 

During later registration in amdgpu_dm_register_backlight_device(), if
acpi_video_backlight_use_native() is false, the driver exits early and never
links a backend to this property. 

Will modern userspace compositors attempt to use this exposed but
non-functional LUMINANCE property and silently fail to adjust brightness,
ignoring the working ACPI sysfs interface entirely?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908044035.62093-1-mario.limonciello@amd.com?part=8