Re: [PATCH v7 04/12] drm: add connector backlight (LUMINANCE) infrastructure

[sashiko-bot@kernel.org]

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 <mario.limonciello@amd.com>
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/20260831210531.2334221-1-mario.limonciello@amd.com?part=4