Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [Critical] Missing unregistration of `drm_backlight` on connector registration error paths leads to List Corruption and Use-After-Free. - [High] `drm_backlight_get_device()` returns an unreferenced pointer, causing a Use-After-Free vulnerability. - [High] Calling a sleeping function (`kobject_uevent_env`) while holding a spinlock in `drm_backlight_link()`. - [High] Out-of-band modification of atomic state in `__drm_backlight_real_changed()`. - [High] Dangling sysfs 'backlight' symlink permanently leaked during connector unregistration. - [High] Global shared property bounds corruption in `__drm_backlight_update_prop_range()`. - [Medium] Unconditional lockdep assertion failure and unsafe list access in backlight registration functions. --
commit 97672c232ba720c2f01bd5c726f55338a93bdc07 Author: Mario Limonciello (AMD) <[email protected]> drm: link connectors to backlight devices This commit introduces DRM backlight helpers to connect hardware backlight devices to DRM connectors. It adds a generic LUMINANCE property and manages the links, sysfs symlinks, and legacy sysfs inhibitions. > --- /dev/null > +++ b/drivers/gpu/drm/drm_backlight.c [ ... ] > +/* caller must hold @drm_backlight_lock */ > +static void __drm_backlight_real_changed(struct drm_backlight *b, uint64_t v) > +{ [ ... ] > + /* Update the atomic state directly. > + * For atomic drivers, the luminance value is stored in > + * connector->state->luminance, not in the legacy property array. > + * We update it unconditionally to reflect the hardware state, > + * regardless of DPMS. > + */ > + if (connector->state) > + connector->state->luminance = set; [Severity: High] Is it safe to mutate the atomic state out-of-band here? When the hardware brightness changes, this function acts as a notifier callback and directly assigns connector->state->luminance without executing an atomic commit or acquiring DRM modeset locks. Since the state object is expected to be strictly immutable during normal operation, could this cause data races against concurrent commits modifying or duplicating the state? [ ... ] > +static void __drm_backlight_update_prop_range(struct drm_backlight *b) > +{ [ ... ] > + /* Update property range to match hardware capabilities. > + * Range of 0-0 indicates no backing device. > + * Range of 1-max for normal operation. > + * Range of 0-max means that the display would turn off at 0 > + */ > + if (prop->values[1] != max) { > + prop->values[0] = max ? (can_disable ? 0 : 1) : 0; > + prop->values[1] = max; > + } > +} [Severity: High] Can this corrupt the permissible brightness range for other connectors? The LUMINANCE DRM property is created once per device in drm_mode_create_standard_properties() and shared across all connectors on that device. When __drm_backlight_update_prop_range() dynamically overwrites the global prop->values[0] and prop->values[1] array, it changes the metadata for the shared property. If a driver exposes multiple display connectors with backlights that have different maximum brightness ranges, it seems this would overwrite the bounds for all of them. [ ... ] > +void drm_backlight_register(struct drm_backlight *b) > +{ > + if (!b) > + return; > + > + WARN_ON(__drm_backlight_is_registered(b)); > + > + guard(spinlock)(&drm_backlight_lock); > + list_add(&b->list, &drm_backlight_list); > +} [Severity: Medium] Does this trigger an unconditional lockdep splat? Looking at drm_backlight_register(), it calls __drm_backlight_is_registered(b) before acquiring the drm_backlight_lock spinlock. However, the helper contains: static bool __drm_backlight_is_registered(...) { lockdep_assert_held(&drm_backlight_lock); ... } Since the lock isn't held yet, this appears to trigger an assertion failure and creates a data race when accessing the list. Should the lock be acquired before checking the registration status? [ ... ] > +void drm_backlight_link(struct drm_backlight *b, struct backlight_device *bd) > +{ [ ... ] > + guard(spinlock)(&drm_backlight_lock); > + send_uevent = __drm_backlight_link(b, bd); > + > + if (send_uevent) > + kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, > + (char **)ep); > +} [Severity: High] Could this lead to a sleep-in-atomic issue? The guard(spinlock)(&drm_backlight_lock) acquires the spinlock for the entire duration of the function scope. Later in the function, it calls kobject_uevent_env(), which performs memory allocations using GFP_KERNEL and can sleep. Is there a way to drop the lock before sending the uevent? [ ... ] > +struct backlight_device *drm_backlight_get_device(struct drm_backlight *b) > +{ > + if (!b) > + return NULL; > + > + guard(spinlock)(&drm_backlight_lock); > + return b->link; > +} [Severity: High] Could this result in a use-after-free when creating the sysfs link? In drm_backlight_get_device(), the pointer b->link is returned while briefly holding a spinlock. However, it doesn't take a reference to the underlying backlight_device before returning. When drm_sysfs_connector_add_late() uses this pointer to call sysfs_create_link(), what prevents the backlight driver from concurrently unregistering and freeing the device before the kobj is accessed? > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c [ ... ] > @@ -845,6 +847,8 @@ int drm_connector_register(struct drm_connector > *connector) > if (connector->registration_state != DRM_CONNECTOR_INITIALIZING) > goto unlock; > > + drm_backlight_register(connector->backlight); > + > ret = drm_sysfs_connector_add(connector); > if (ret) > goto unlock; [Severity: Critical] Does the error path here leak the backlight registration? If a subsequent operation like drm_sysfs_connector_add() fails, the function aborts via goto unlock but doesn't call drm_backlight_unregister(). Later, when drm_connector_cleanup() calls drm_backlight_free(), it performs kfree(b). Since the object was never unregistered, it remains part of drm_backlight_list, which would lead to list corruption and a use-after-free. Should drm_backlight_unregister() be called on the error paths? [ ... ] > @@ -931,6 +935,8 @@ EXPORT_SYMBOL(drm_connector_dynamic_register); > void drm_connector_unregister(struct drm_connector *connector) > { > mutex_lock(&connector->mutex); > + drm_backlight_unregister(connector->backlight); > + > if (connector->registration_state != DRM_CONNECTOR_REGISTERED) { > mutex_unlock(&connector->mutex); > return; > } [ ... ] > drm_sysfs_connector_remove_early(connector); > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c [ ... ] > void drm_sysfs_connector_remove_early(struct drm_connector *connector) > { > + if (connector->backlight && > drm_backlight_get_device(connector->backlight)) > + sysfs_remove_link(&connector->kdev->kobj, "backlight"); > + > if (connector->ddc) [Severity: High] Is the backlight sysfs symlink permanently leaked during unregistration? In drm_connector_unregister(), drm_backlight_unregister() is called first, which clears the backlight link. Shortly after, drm_sysfs_connector_remove_early() is called. It checks if drm_backlight_get_device(connector->backlight) returns a non-NULL pointer. Because the backlight was just unregistered, this check fails and the symlink is never removed from sysfs. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
