Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm: AB-BA deadlock between sysfs active ref and dp.mutex - [High] drm: UAF/NULL deref of connector->kdev in sysfs_connector_dp_link_update --
commit 1620ef6dfb353b92f81d9907563ffd328e9eb367 Author: Kory Maincent <[email protected]> Subject: drm: Introduce DisplayPort link training capabilities and state helpers This commit introduces helpers to expose DisplayPort link training capabilities and state to userspace via sysfs attributes. It adds functions to manage sink and source capabilities and exposes them through a new dp_link sysfs group. > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c > b/drivers/gpu/drm/display/drm_dp_helper.c > --- a/drivers/gpu/drm/display/drm_dp_helper.c > +++ b/drivers/gpu/drm/display/drm_dp_helper.c [ ... ] > +void drm_dp_sink_reset_caps(struct drm_connector *connector) > +{ > + struct drm_connector_dp_link_caps zero_caps = {0}; > + > + if (!connector) > + return; > + > + > drm_modeset_lock_assert_held(&connector->dev->mode_config.connection_mutex); > + > + scoped_guard(mutex, &connector->dp.mutex) { > + kfree(connector->dp.cur_link_info.link_rates); > + memcpy(&connector->dp.cur_link_info, &zero_caps, > sizeof(zero_caps)); > + kfree(connector->dp.sink_link_caps.link_rates); > + memcpy(&connector->dp.sink_link_caps, &zero_caps, > sizeof(zero_caps)); > + > + drm_sysfs_connector_dp_link_update(connector); > + } > +} [ ... ] > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c [ ... ] > +#define DRM_CONNECTOR_DP_ATTR_SHOW_SIMPLE_NESTED(_name, _info) > \ > +static ssize_t _name##_##_info##_show(struct device *device, > \ > + struct device_attribute *attr, \ > + char *buf) \ > +{ > \ > + struct drm_connector *connector = to_drm_connector(device); \ > + ssize_t size; \ > + scoped_guard(mutex, &connector->dp.mutex) \ > + size = sysfs_emit(buf, "%d\n", connector->dp._name._info); \ > + return size; \ > +} [Severity: High] Does this sequence introduce an AB-BA deadlock between the sysfs active reference and connector->dp.mutex? If a userspace thread reads a sysfs attribute like cur_link_info_nlanes, the VFS/kernfs core acquires an active reference (s_active) on the kernfs_node. The show callback then attempts to lock connector->dp.mutex via scoped_guard(). Concurrently, a hotplug or link retraining event can call drm_dp_sink_reset_caps() as seen in the drm_dp_helper.c snippet above. That thread acquires connector->dp.mutex first, and then calls drm_sysfs_connector_dp_link_update(). When drm_sysfs_connector_dp_link_update() invokes sysfs_update_group(), and the visibility drops to 0 (because nlanes is cleared to 0), it calls kernfs_remove_by_name(). This in turn calls kernfs_drain(), which blocks indefinitely waiting for all active references to drop. This leaves the kernel thread holding dp.mutex waiting for s_active to drop, and the userspace thread holding s_active waiting for dp.mutex. [ ... ] > +void drm_sysfs_connector_dp_link_update(struct drm_connector *connector) > +{ > + int ret; > + > + if (!connector->kdev) > + return; > + > + if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort && > + connector->connector_type != DRM_MODE_CONNECTOR_eDP) > + return; > + > + ret = sysfs_update_group(&connector->kdev->kobj, > + &connector_dp_link_group); [Severity: High] Is it possible for connector->kdev to be unregistered and freed between the NULL check and the sysfs_update_group() call? MST hot-unplug processes (like drm_dp_delayed_destroy_work()) can invoke drm_connector_unregister() and device_unregister(connector->kdev) without holding connection_mutex or connector->dp.mutex. Since drm_sysfs_connector_dp_link_update() does not hold a lock that serializes against connector unregistration, this looks like a TOCTOU race condition where connector->kdev->kobj could be a use-after-free or a NULL pointer dereference. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
