Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider: - [High] Use-after-free of `drm_backlight` in `drm_backlight_notify` during concurrent unregistration. - [High] Unprotected direct modification of the active DRM atomic connector state from an asynchronous backlight event. - [High] `drm_backlight_get_device()` returns an unreferenced pointer, leading to Use-After-Free in sysfs linking. - [High] Legacy DPMS state handling erroneously dims all connectors on a CRTC and fails to persist the DPMS OFF state across subsequent commits. - [High] Data race dereferencing live `connector->state` in atomic commit tail instead of the captured new state. - [High] Leaked backlight list entry and pending work item on connector cleanup error path causes Use-After-Free. - [High] A TOCTOU race in `drm_backlight_do_unlink` can overwrite a concurrently linked new backend pointer with NULL, leading to a NULL pointer dereference. - [High] Race condition in `drm_backlight_do_unlink` early return can lead to use-after-free during connector teardown. - [Medium] Asynchronous backlight transitions violate strict display pipeline sequencing and DPMS semantics. - [Medium] The 'backlight' sysfs symlink becomes dangling or stale on dynamic backend unlink or hot-swap. - [Medium] The `get_range` callback is invoked while holding a spinlock and from the atomic commit tail, directly violating its API contract. - [Medium] Unconditional lockdep assertion failure in `drm_backlight_connector_cleanup`. - [Low] The drm_backlight core is hardcoded to `backlight_device`, preventing the addition of other backends without modifying the core. --
commit a5f154fe3d2dddb5c9740e818e4993aaff0079d8 Author: Mario Limonciello <[email protected]> Subject: drm: add connector backlight (LUMINANCE) infrastructure This commit adds a generic per-connector LUMINANCE property to allow driving backlight brightness through the DRM atomic modeset path. It embeds a drm_backlight structure in every connector and links it to backend devices like the backlight subsystem via workqueues. > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 285aac3554dfd..3603a8cce074e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c [ ... ] > @@ -1230,6 +1231,10 @@ drm_atomic_helper_commit_encoder_bridge_disable(struct > drm_device *dev, > * it away), so we won't call disable hooks twice. > */ > bridge = drm_bridge_chain_get_first_bridge(encoder); > + > + /* Turn the backlight off before disabling the pipeline. */ > + drm_backlight_set_luminance(connector, 0); > + > drm_atomic_bridge_chain_disable(bridge, state); > drm_bridge_put(bridge); [Severity: Medium] Does this sequence violate DPMS semantics and cause visual artifacts? drm_backlight_set_luminance() schedules the backlight to turn off asynchronously via a workqueue, but drm_atomic_bridge_chain_disable() immediately disables the display pipeline synchronously. The pipeline might turn off before the backlight is actually off. [ ... ] > @@ -1744,6 +1749,10 @@ drm_atomic_helper_commit_encoder_bridge_enable(struct > drm_device *dev, struct dr > > drm_atomic_bridge_chain_enable(bridge, state); > drm_bridge_put(bridge); > + > + /* Restore the backlight once the pipeline is enabled. */ > + if (connector->state) > + > drm_atomic_helper_connector_apply_luminance(connector->state); [Severity: High] Can this cause data races by dereferencing the live connector state? Because this runs asynchronously in the non-blocking commit tail, the live connector->state might have already been swapped by a subsequent atomic commit. Should this use the captured new_conn_state instead? [ ... ] > +void drm_atomic_helper_connector_apply_luminance(const struct > drm_connector_state *conn_state) > +{ > + struct drm_connector *connector = conn_state->connector; > + > + if (conn_state->crtc && conn_state->crtc->state && > + conn_state->crtc->state->active) > + drm_backlight_set_luminance(connector, conn_state->luminance); > +} [Severity: High] Does this unintentionally override the legacy DPMS state on subsequent commits? By relying solely on the CRTC's active state, if a single connector is set to DPMS OFF (but the CRTC remains active for other cloned connectors), the next atomic commit will incorrectly restore this connector's backlight. > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > b/drivers/gpu/drm/drm_atomic_uapi.c > index 1050dddadb17e..5e687600ce45e 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c [ ... ] > + /* Handle backlight brightness coordination with DPMS state changes */ > + if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) { > + /* DPMS ON -> OFF: dim all connectors driven by this CRTC. */ > + if (crtc) > + drm_atomic_crtc_set_backlight(crtc, false); [Severity: High] Does this erroneously dim all connectors on the CRTC instead of just the target connector? When userspace sets one connector to DPMS OFF on a cloned output setup, this will unintentionally turn off the backlight for all other active connectors attached to the same CRTC. > diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c > new file mode 100644 > index 0000000000000..4251cb25079c3 > --- /dev/null > +++ b/drivers/gpu/drm/drm_backlight.c [ ... ] > +/* caller must hold @drm_backlight_lock */ > +static void __drm_backlight_set(struct drm_backlight *b, unsigned int v) > +{ > + unsigned int max = 0; > + bool can_disable = false; > + > + lockdep_assert_held(&drm_backlight_lock); > + > + if (!b->funcs || !b->funcs->get_range) > + return; > + > + b->funcs->get_range(b, &max, &can_disable); [Severity: Medium] Can this trigger a sleeping in atomic context warning? __drm_backlight_set() is called with the drm_backlight_lock spinlock held. However, it invokes the get_range callback, which the API contract states is allowed to sleep and must not be called from atomic contexts. [ ... ] > +/* caller must hold @drm_backlight_lock */ > +static void __drm_backlight_readback(struct drm_backlight *b, unsigned int v) > +{ [ ... ] > + /* > + * Reflect a hardware-side brightness change (firmware hotkeys, or a > + * legacy sysfs write while not inhibited) back into the connector's > + * committed luminance so a read-back returns the real value. > + */ > + if (connector->state) > + connector->state->luminance = min(v, max); > +} [Severity: High] Is it safe to mutate the active DRM atomic connector state directly? The connector->state represents the committed, immutable atomic state. Modifying it outside of an atomic commit transaction could cause data races and torn state when concurrent readers (like state duplication) access it. [ ... ] > +static void drm_backlight_do_unlink(struct drm_backlight *b) > +{ > + struct backlight_device *bd; > + unsigned int clients; > + > + /* > + * Stop new work first, but leave @backend in place so an in-flight > + * worker keeps a valid backend to operate on. Capture the linked > + * backlight_device (if any) while @funcs is still set. > + */ > + scoped_guard(spinlock, &drm_backlight_lock) { > + if (!b->funcs) > + return; [Severity: High] Can this early return lead to a use-after-free during connector teardown? If one thread drops the lock to call cancel_work_sync(), a concurrent thread (e.g., connector teardown) calling drm_backlight_do_unlink() will see !b->funcs and return immediately. This second thread will proceed to free the connector while the first thread is still executing cancel_work_sync(&b->work) inside the freed memory block. [Severity: Medium] Does this missing cleanup leave a dangling sysfs symlink? When unlinking the backend, the backlight sysfs symlink created in drm_sysfs_connector_add_late() is not removed, which will leave a stale symlink pointing to a freed device object. > + bd = drm_backlight_bd(b); > + clients = b->luminance_clients; > + b->funcs = NULL; > + list_del_init(&b->list); > + } > + > + cancel_work_sync(&b->work); > + > + scoped_guard(spinlock, &drm_backlight_lock) { > + if (clients && bd) > + atomic_sub(clients, &bd->drm_takeover); > + b->backend = NULL; > + } [Severity: High] Can this cause a race condition resulting in a NULL pointer dereference? After dropping the lock for cancel_work_sync(), a concurrent drm_backlight_link() could have linked a new backend. Unconditionally executing b->backend = NULL here will overwrite the newly linked backend, causing the next hardware update to dereference NULL. > + > + backlight_device_unref(bd); [Severity: Low] Does this hardcoded unref violate the backend-agnostic design? Resource cleanup is explicitly hardcoded to the backlight_device subsystem, preventing the addition of other backends without modifying the core. [ ... ] > +void drm_backlight_connector_cleanup(struct drm_connector *connector) > +{ > + struct drm_backlight *b = &connector->backlight; > + > + WARN_ON(__drm_backlight_is_linked(b)); [Severity: Medium] Will this trigger an unconditional lockdep assertion failure? __drm_backlight_is_linked() unconditionally asserts that drm_backlight_lock is held, but it is being called here without acquiring the lock. [Severity: High] Does this error path leak memory and cause a use-after-free? If a connector fails probe before being registered, drm_connector_cleanup() is called, bypassing the unregister path. This function only warns but fails to remove the backlight from drm_backlight_list or cancel its work item, leaving a dangling pointer when the connector is freed. [ ... ] > +struct backlight_device *drm_backlight_get_device(struct drm_connector > *connector) > +{ > + guard(spinlock)(&drm_backlight_lock); > + return drm_backlight_bd(&connector->backlight); > +} [Severity: High] Can this lead to a use-after-free? This returns a raw pointer to a reference-counted object without incrementing the refcount. If the backlight is unregistered concurrently, callers (like drm_sysfs_connector_add_late) will dereference a freed pointer. [ ... ] > +static int drm_backlight_notify(struct notifier_block *self, > + unsigned long event, void *data) > +{ [ ... ] > + for (;;) { > + struct drm_backlight *found = NULL; > + > + scoped_guard(spinlock, &drm_backlight_lock) { > + list_for_each_entry(b, &drm_backlight_list, > list) { > + if (drm_backlight_bd(b) == bd) { > + found = b; > + break; > + } > + } > + } > + if (!found) > + break; > + drm_backlight_do_unlink(found); [Severity: High] Is the found pointer safe to dereference outside of the spinlock? The spinlock is dropped before calling drm_backlight_do_unlink(found). Concurrently, the connector embedding this backlight could be unregistered and freed, leading to a use-after-free when found is dereferenced. > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index ef4e923a87284..06cdfe046ca89 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c [ ... ] > @@ -391,15 +393,33 @@ int drm_sysfs_connector_add(struct drm_connector > *connector) > > int drm_sysfs_connector_add_late(struct drm_connector *connector) > { > - if (connector->ddc) > - return sysfs_create_link(&connector->kdev->kobj, > - &connector->ddc->dev.kobj, "ddc"); > + struct backlight_device *bd = drm_backlight_get_device(connector); [ ... ] > + if (bd) { > + int ret = sysfs_create_link(&connector->kdev->kobj, > + &bd->dev.kobj, "backlight"); [Severity: High] Can this cause a use-after-free when creating the sysfs link? drm_backlight_get_device() does not take a reference on bd. If the backlight device is concurrently unregistered and freed, dereferencing bd->dev.kobj here will trigger a UAF. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
