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 <[email protected]> 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/[email protected]?part=8
