Re: [PATCH RFC v3 3/8] drm: Introduce DisplayPort link training capabilities and state helpers

[sashiko-bot@kernel.org]

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 <kory.maincent@bootlin.com>
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/20261012-feat_link_cap-v3-0-7517f4aba0f3@bootlin.com?part=3