Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm: use-after-free on connector->kdev in drm_sysfs_connector_remove() - [High] drm: kdev race in drm_sysfs_connector_add() causes -EEXIST failure - [Medium] drm: unlocked nlanes read in dp_link_attr_visible() races with updates - [Medium] drm: missing LTTPR validation in drm_dp_sink_sync_caps() --
commit 9bbc7a53acf715de24f598695d290cb7880247c1 Author: Kory Maincent <[email protected]> drm: Introduce DisplayPort link training capabilities and state helpers This commit adds DisplayPort link training capability and state helpers to drm_dp_helper.c and exposes source, sink, and current link parameters through a dp_link sysfs attribute group on DisplayPort and eDP connectors. > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c > b/drivers/gpu/drm/display/drm_dp_helper.c > index 8473dbcd8808a..5e53c6a9d35fd 100644 > --- a/drivers/gpu/drm/display/drm_dp_helper.c > +++ b/drivers/gpu/drm/display/drm_dp_helper.c [ ... ] > @@ -5135,6 +5136,30 @@ int drm_dp_read_dpcd_sink_rates(struct drm_dp_aux *aux, > } > EXPORT_SYMBOL_GPL(drm_dp_read_dpcd_sink_rates); > > +/** > + * drm_dp_get_max_lane_count - Get the max lane count from DPCD and LTTPR > + * capabilities > + * @dpcd: DPCD receiver capabilities > + * @lttpr_common_caps: LTTPR common capabilities (NULL if no LTTPR supported) > + * > + * Returns: Number of max lane count > + */ > +int drm_dp_get_max_lane_count(const u8 dpcd[DP_RECEIVER_CAP_SIZE], > + const u8 > lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE]) > +{ > + int nlanes, lttpr_nlanes; > + > + nlanes = dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK; > + if (lttpr_common_caps) { > + lttpr_nlanes = drm_dp_lttpr_max_lane_count(lttpr_common_caps); > + if (lttpr_nlanes) > + nlanes = min(nlanes, lttpr_nlanes); > + } > + > + return nlanes; > +} > +EXPORT_SYMBOL_GPL(drm_dp_get_max_lane_count); > + > /** > * drm_dp_read_edp_sink_rates() - read the sink rates from the eDP DPCD > * @aux: DisplayPort AUX channel > @@ -5196,3 +5221,284 @@ int drm_dp_read_dsc_dpcd(struct drm_dp_aux *aux, [ ... ] > +int drm_dp_sink_sync_caps(struct drm_connector *connector, struct drm_dp_aux > *aux, > + const struct drm_dp_tunnel *tunnel) > +{ > + u8 lttpr_common_caps[DP_LTTPR_COMMON_CAP_SIZE] = {0}; > + struct drm_connector_dp_link_caps link_caps; > + int sink_rates[DP_MAX_SUPPORTED_RATES] = {0}; > + u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE] = {0}; > + u8 dpcd_caps[DP_RECEIVER_CAP_SIZE] = {0}; > + struct drm_dp_desc desc = {0}; > + int num_sink_rates, ret; > + u8 edp_dpcd_rev = 0; > + > + if (!connector) > + return -ENODEV; > + > + ret = drm_dp_read_dpcd_caps(aux, dpcd_caps); > + if (ret < 0) { > + drm_dbg_kms(aux->drm_dev, "%s: error reading DPCD capabilities > (%d)\n", > + aux->name, ret); > + return ret; > + } > + > + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) { > + ret = drm_dp_dpcd_read_byte(aux, DP_EDP_DPCD_REV, > &edp_dpcd_rev); > + if (ret < 0) > + drm_dbg_kms(aux->drm_dev, "%s: No DPCD eDP revision > found\n", > + aux->name); > + } > + > + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort && > + dpcd_caps[DP_DPCD_REV] >= DP_DPCD_REV_13) { > + ret = drm_dp_read_lttpr_common_caps(aux, dpcd_caps, > lttpr_common_caps); > + if (ret < 0) { > + drm_dbg_kms(aux->drm_dev, > + "%s: error reading LTTPR caps, assuming no > LTTPR (%d)\n", > + aux->name, ret); > + /* LTTPR capabilities are read block by block. Clean the > + * entire capabilities if the read fails to avoid > + * partial garbage > + */ > + memset(lttpr_common_caps, 0, sizeof(lttpr_common_caps)); > + } > + } [ ... ] > + link_caps.nlanes = drm_dp_get_max_lane_count(dpcd_caps, > lttpr_common_caps); > + link_caps.nlink_rates = num_sink_rates; > + link_caps.link_rates = sink_rates; > + link_caps.dsc = drm_dp_sink_supports_dsc(dsc_dpcd); > + > + return drm_dp_sink_set_caps(connector, &link_caps); > +} > +EXPORT_SYMBOL_GPL(drm_dp_sink_sync_caps); [Severity: Medium] Can drm_dp_sink_sync_caps() and drm_dp_get_max_lane_count() clamp sink_link_caps.nlanes and sink_link_caps.link_rates using unvalidated repeater bytes when a sink does not have repeaters present? In drivers/gpu/drm/display/drm_dp_helper.c, drm_dp_sink_sync_caps() calls drm_dp_read_lttpr_common_caps(aux, dpcd_caps, lttpr_common_caps) for any DisplayPort 1.3 or newer sink and only zeroes lttpr_common_caps if the read returns a negative error code. If a sink without repeaters succeeds on that read and returns non-zero bytes where lttpr_common_caps[0] < 0x14 or drm_dp_lttpr_count(lttpr_common_caps) <= 0, neither drm_dp_sink_sync_caps(), drm_dp_get_max_lane_count(), nor the 8b/10b path in drm_dp_read_dpcd_sink_rates() validates the repeater revision or count the way drm_dp_get_128b132b_link_rates() does: drivers/gpu/drm/display/drm_dp_helper.c:drm_dp_get_128b132b_link_rates() { ... if (lttpr_common_caps && drm_dp_lttpr_count(lttpr_common_caps) > 0) { /* We have a repeater */ if (lttpr_common_caps[0] >= 0x20 && ... } In addition, drm_dp_sink_sync_caps() passes tunnel to drm_dp_read_dpcd_sink_rates(), but drm_dp_get_max_lane_count() does not account for drm_dp_tunnel_max_dprx_lane_count(tunnel). Should lttpr_common_caps be validated (checking lttpr_common_caps[0] >= 0x14 and drm_dp_lttpr_count(lttpr_common_caps) > 0) before clamping nlanes and 8b/10b link rates, and should tunnel lane count limits also be applied? [ ... ] > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index 7b16d32b6ff65..ac9762a066db4 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -340,6 +340,151 @@ static const struct attribute_group > *connector_dev_groups[] = { [ ... ] > +static umode_t dp_link_attr_visible(struct kobject *kobj, > + struct attribute *a, int n) > +{ > + struct device *dev = kobj_to_dev(kobj); > + struct drm_connector *connector = to_drm_connector(dev); > + struct drm_connector_dp_link_caps *caps; > + > + if (a == &dev_attr_source_link_caps_nlanes.attr || > + a == &dev_attr_source_link_caps_link_rates.attr || > + a == &dev_attr_source_link_caps_dsc.attr) > + caps = &connector->dp.source_link_caps; > + else if (a == &dev_attr_sink_link_caps_nlanes.attr || > + a == &dev_attr_sink_link_caps_link_rates.attr || > + a == &dev_attr_sink_link_caps_dsc.attr) > + caps = &connector->dp.sink_link_caps; > + else > + caps = &connector->dp.cur_link_info; > + > + /* Checking nlanes is enough to decide whether the attributes should > + * be exposed: without lanes there is no working DisplayPort link > + */ > + return caps->nlanes ? a->mode : 0; > +} [Severity: Medium] What prevents a data race on caps->nlanes in dp_link_attr_visible() when sink capabilities or active link parameters are updated concurrently? In drivers/gpu/drm/display/drm_dp_helper.c, drm_dp_sink_set_caps(), drm_dp_set_cur_link_params(), and drm_dp_sink_reset_caps() modify sink_link_caps.nlanes and cur_link_info.nlanes under connector->dp.mutex and release that lock before drm_sysfs_connector_dp_link_update() acquires connector->dp.sysfs_mutex: drivers/gpu/drm/display/drm_dp_helper.c:drm_dp_sink_set_caps() { ... scoped_guard(mutex, &connector->dp.mutex) { /* Only refresh the group when the visibility changes. */ update = !connector->dp.sink_link_caps.nlanes != !link_caps->nlanes; ret = drm_dp_copy_caps(&connector->dp.sink_link_caps, link_caps); if (ret) return ret; } if (update) drm_sysfs_connector_dp_link_update(connector); ... } Meanwhile, dp_link_attr_visible() in drivers/gpu/drm/drm_sysfs.c reads caps->nlanes without holding connector->dp.mutex during sysfs_create_group(), sysfs_update_group(), or sysfs_group_change_owner(). If a writer modifies caps->nlanes on another processor while create_files() in fs/sysfs/group.c iterates over connector_dp_link_attrs, could dp_link_attr_visible() observe different values of caps->nlanes across the three attributes in the same group, or cause a concurrent sysfs_group_change_owner() call to see non-zero visibility before the kernfs nodes are created? > + > +static const struct attribute_group connector_dp_link_group = { > + .name = "dp_link", > + .attrs = connector_dp_link_attrs, > + .is_visible = dp_link_attr_visible, > +}; > + > +/** > + * drm_sysfs_connector_dp_link_update - Update the visibility of the DP link > + * attributes > + * @connector: DisplayPort connector > + */ > +void drm_sysfs_connector_dp_link_update(struct drm_connector *connector) > +{ > + int ret; > + > + if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort && > + connector->connector_type != DRM_MODE_CONNECTOR_eDP) > + return; > + > + scoped_guard(mutex, &connector->dp.sysfs_mutex) { > + if (!connector->kdev) > + return; > + > + ret = sysfs_update_group(&connector->kdev->kobj, > + &connector_dp_link_group); > + } > + if (ret) > + drm_err(connector->dev, > + "failed to update DP link sysfs attributes: %d\n", ret); > +} > +EXPORT_SYMBOL_GPL(drm_sysfs_connector_dp_link_update); [Severity: High] Could drm_sysfs_connector_dp_link_update() race with drm_sysfs_connector_remove() and trigger a use-after-free on connector->kdev? In drivers/gpu/drm/drm_sysfs.c, drm_sysfs_connector_dp_link_update() checks connector->kdev and passes &connector->kdev->kobj to sysfs_update_group() under connector->dp.sysfs_mutex, without holding connector->mutex. However, when drm_connector_unregister() calls drm_sysfs_connector_remove(), connector->kdev is unregistered and freed before connector->kdev is cleared and without taking connector->dp.sysfs_mutex: drivers/gpu/drm/drm_sysfs.c:drm_sysfs_connector_remove() { if (!connector->kdev) return; ... device_unregister(connector->kdev); connector->kdev = NULL; } Since device_unregister(connector->kdev) calls device_del() and put_device() (freeing connector->kdev via drm_sysfs_release()) before clearing connector->kdev, wouldn't a concurrent call to drm_sysfs_connector_dp_link_update() from drm_dp_sink_set_caps(), drm_dp_sink_sync_caps(), drm_dp_set_cur_link_params(), or drm_dp_sink_reset_caps() observe a non-null connector->kdev and dereference &connector->kdev->kobj while or after it is freed? > + > int drm_sysfs_connector_add(struct drm_connector *connector) > { > struct drm_device *dev = connector->dev; > @@ -376,6 +521,18 @@ int drm_sysfs_connector_add(struct drm_connector > *connector) > > connector->kdev = kdev; > > + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort || > + connector->connector_type == DRM_MODE_CONNECTOR_eDP) { > + scoped_guard(mutex, &connector->dp.sysfs_mutex) > + r = sysfs_create_group(&connector->kdev->kobj, > + &connector_dp_link_group); > + > + if (r) { > + drm_err(dev, "failed to create DP connector sysfs: > %d\n", r); > + goto err_dp_sysfs; > + } > + } > + > if (dev_fwnode(kdev)) { > r = component_add(kdev, &typec_connector_ops); > if (r) > @@ -384,6 +541,9 @@ int drm_sysfs_connector_add(struct drm_connector > *connector) > > return 0; > > +err_dp_sysfs: > + connector->kdev = NULL; > + device_del(kdev); > err_free: > put_device(kdev); > return r; [Severity: High] What happens if drm_sysfs_connector_dp_link_update() runs between assigning connector->kdev = kdev and acquiring connector->dp.sysfs_mutex in drm_sysfs_connector_add()? In drivers/gpu/drm/drm_sysfs.c, drm_sysfs_connector_add() publishes connector->kdev = kdev outside connector->dp.sysfs_mutex before calling sysfs_create_group(&connector->kdev->kobj, &connector_dp_link_group). If drm_sysfs_connector_dp_link_update() runs in that window (for example, from a concurrent hotplug or detect worker), it acquires connector->dp.sysfs_mutex first, sees a non-null connector->kdev, and calls sysfs_update_group(). Because the dp_link directory does not exist yet, internal_create_group() in fs/sysfs/group.c creates the dp_link directory when source_link_caps.nlanes is non-zero. When drm_sysfs_connector_add() then acquires connector->dp.sysfs_mutex and calls sysfs_create_group(), kernfs_create_dir_ns() fails because the directory already exists and triggers sysfs_warn_dup(). That jumps to err_dp_sysfs, where connector->kdev is cleared and device_del(kdev) and put_device(kdev) run outside connector->dp.sysfs_mutex, aborting connector registration and leaving kdev teardown unsynchronized with drm_sysfs_connector_dp_link_update(). [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
