[PATCH v6 7/7] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Tue Aug 4 00:38:09 PDT 2026
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
New issues:
- [High] `cdn_dp_connected_port` unconditionally returns `dp->port[0]` for extcon setups, completely breaking Hot-Plug Detection.
- [High] A mismatch between the number of PHYs (`dp->ports`) and DT endpoints (`dp->bridge_count`) causes uninitialized stack reads and NULL pointer dereferences.
- [High] Out-of-bounds array write in `cdn_dp_parse_next_bridge_dt` when a device tree has more than two endpoints.
- [High] The error path in `cdn_dp_bind()` and successful path in `cdn_dp_unbind()` do not clear `dp->next_bridge_list` pointers after dropping references, leading to a Double-Put / Use-After-Free.
- [High] Double-free of EDID cache pointers across component unbind/rebind cycles.
- [High] Missing mutual exclusion in `atomic_check` allows simultaneous enablement of multiple bridges sharing a single DP controller.
- [High] Using `devm_drm_bridge_alloc()` and `devm_drm_bridge_add()` inside the component `bind` callback leads to memory leaks and dangling global DRM objects on unbind/rebind cycles.
- [High] `cdn_dp_switch_port` unconditionally programs video timings during EDID reads, causing failures when no valid mode is set.
- [High] Failure to clean up a successfully initialized `drm_encoder` in `cdn_bridge_add` error paths leads to Use-After-Free.
- [Medium] `INIT_WORK` is called after exposing the bridge to external subsystems, allowing an early HPD notification to schedule uninitialized work.
Pre-existing issues:
- [High] Using `devm_extcon_register_notifier` inside a component `bind` function causes notifier chain corruption and UAF on rebind.
--
--- Patch [7]: [PATCH v6 7/7] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection ---
commit 47d7fbb7737dc45e037477d5f8fc38f597fe33c9
Author: Chaoyi Chen <chaoyi.chen at rock-chips.com>
drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection
This patch adds support for multiple bridges in the Rockchip CDN-DP controller
to allow users to flexibly select the output port between two USB/DP combo
PHYs. It introduces independent encoders and bridges for each PHY port based
on the DRM AUX HPD bridge, requiring proper DT configuration.
[ ... ]
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
[ ... ]
> @@ -192,14 +193,27 @@ static int cdn_dp_get_sink_count(struct cdn_dp_device *dp, u8 *sink_count)
> static struct cdn_dp_port *cdn_dp_connected_port(struct cdn_dp_device *dp)
> {
> struct cdn_dp_port *port;
> - int i, lanes;
> + int i, lanes[MAX_PHY];
>
> for (i = 0; i < dp->ports; i++) {
> port = dp->port[i];
> - lanes = cdn_dp_get_port_lanes(port);
> - if (lanes)
> + lanes[i] = cdn_dp_get_port_lanes(port);
> + if (!dp->next_bridge_valid)
> return port;
[Severity: High]
Does this early return unconditionally bypass the connection state evaluation
for extcon setups?
If !dp->next_bridge_valid evaluates to true, it looks like this will return
dp->port[0] on the first iteration without verifying whether the lanes
indicate an active connection, which would break hot-plug detection for
extcon-based devices.
> }
> +
> + if (dp->next_bridge_valid) {
> + /* If more than one port is available, pick the last active port */
> + if (dp->active_port >= 0 && lanes[dp->active_port])
> + return dp->port[dp->active_port];
> +
> + /* If the last active port is not available, pick an available port in order */
> + for (i = 0; i < dp->bridge_count; i++) {
> + if (lanes[i])
> + return dp->port[i];
> + }
[Severity: High]
Could this loop read uninitialized stack memory?
If the device tree defines more bridge endpoints in dp->bridge_count than the
number of physical PHY ports in dp->ports, this loop iterates past the
initialized bounds of the lanes array.
> + }
> +
> return NULL;
> }
[ ... ]
> @@ -254,12 +268,45 @@ static const struct drm_edid *
> cdn_dp_bridge_edid_read(struct drm_bridge *bridge, struct drm_connector *connector)
> {
> struct cdn_dp_device *dp = bridge_to_dp(bridge);
> - const struct drm_edid *drm_edid;
> + struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
> + struct cdn_dp_port *port = dp->port[dp_bridge->id];
[Severity: High]
Could this also lead to a NULL pointer dereference?
If dp_bridge->id exceeds the configured PHY ports, dp->port will be NULL.
The subsequent check for port->phy_enabled below will then dereference this
NULL pointer.
> + struct cdn_dp_port *prev_port = NULL;
> + const struct drm_edid *drm_edid = NULL;
[ ... ]
> +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port *prev_port,
> + struct cdn_dp_port *port)
> +{
> + int ret;
[ ... ]
> + dp->lanes = port->lanes;
> +
> + if (!cdn_dp_check_link_status(dp)) {
> + dev_info(dp->dev, "Connected with sink; re-train link\n");
> +
> + ret = cdn_dp_train_link(dp);
> + if (ret) {
> + dev_err(dp->dev, "Training link failed: %d\n", ret);
> + goto out;
> + }
> +
> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
> + if (ret) {
> + dev_err(dp->dev, "Failed to idle video %d\n", ret);
> + goto out;
> + }
> +
> + ret = cdn_dp_config_video(dp);
[Severity: High]
Will this video configuration fail when called from the EDID read path?
cdn_dp_switch_port() is invoked from cdn_dp_bridge_edid_read() where no
valid display mode has been established yet. Since dp->mode.clock is likely
zero at this point, cdn_dp_config_video() might fail with an invalid argument
or divide-by-zero, which would break the port switch entirely.
> + if (ret)
> + dev_err(dp->dev, "Failed to configure video: %d\n", ret);
> + }
> +
> +out:
> + return ret;
> +}
[ ... ]
> @@ -600,6 +693,9 @@ static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_at
>
> mutex_lock(&dp->lock);
>
> + if (dp->next_bridge_valid)
> + dp->active_port = dp_bridge->id;
> +
> ret = cdn_dp_enable(dp);
[Severity: High]
Is there missing mutual exclusion to prevent simultaneous enablement of
multiple bridges sharing this single DP controller?
Since the driver creates multiple DRM connectors (one per PHY port) that
share a single physical DP hardware block, without an atomic_check to ensure
only one connector is active at a time, userspace might try to simultaneously
enable CRTCs on multiple connectors. Both could race to program the shared
dp->regs and GRF registers.
> if (ret) {
> DRM_DEV_ERROR(dp->dev, "Failed to enable bridge %d\n",
[ ... ]
> -static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> +static int cdn_bridge_add(struct device *dev,
> + struct drm_bridge *bridge,
> + struct drm_bridge *next_bridge,
> + struct drm_encoder *encoder)
> {
> struct cdn_dp_device *dp = dev_get_drvdata(dev);
> - struct drm_encoder *encoder;
> + struct drm_device *drm_dev = dp->drm_dev;
> + struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;
> struct drm_connector *connector;
> - struct cdn_dp_port *port;
> - struct drm_device *drm_dev = data;
> - int ret, i;
> -
> - ret = cdn_dp_parse_dt(dp);
> - if (ret < 0)
> - return ret;
> -
> - dp->drm_dev = drm_dev;
> - dp->connected = false;
> - dp->active = false;
> - dp->active_port = -1;
> - dp->fw_loaded = false;
> -
> - INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
> -
> - encoder = &dp->encoder.encoder;
> + int ret;
>
> encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev,
> dev->of_node);
> @@ -1003,26 +1106,35 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>
> drm_encoder_helper_add(encoder, &cdn_dp_encoder_helper_funcs);
>
> - dp->bridge.ops =
> - DRM_BRIDGE_OP_DETECT |
> - DRM_BRIDGE_OP_EDID |
> - DRM_BRIDGE_OP_HPD |
> - DRM_BRIDGE_OP_DP_AUDIO;
> - dp->bridge.of_node = dp->dev->of_node;
> - dp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> - dp->bridge.hdmi_audio_dev = dp->dev;
> - dp->bridge.hdmi_audio_max_i2s_playback_channels = 8;
> - dp->bridge.hdmi_audio_spdif_playback = 1;
> - dp->bridge.hdmi_audio_dai_port = -1;
> -
> - ret = devm_drm_bridge_add(dev, &dp->bridge);
> + bridge->ops =
> + DRM_BRIDGE_OP_DETECT |
> + DRM_BRIDGE_OP_EDID |
> + DRM_BRIDGE_OP_HPD |
> + DRM_BRIDGE_OP_DP_AUDIO;
> + bridge->of_node = dp->dev->of_node;
> + bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
> + bridge->hdmi_audio_dev = dp->dev;
> + bridge->hdmi_audio_max_i2s_playback_channels = 8;
> + bridge->hdmi_audio_spdif_playback = 1;
> + bridge->hdmi_audio_dai_port = -1;
> +
> + ret = devm_drm_bridge_add(dev, bridge);
[Severity: High]
Will using devres-managed memory allocation and registration inside a
component bind callback cause memory leaks?
Because cdn_dp_bind() is a component bind callback, the devm lifecycle
is tied to the parent device which was bound during probe. When the
component is unbound, the devm cleanup does not fire. Upon rebinding,
new memory is allocated and new bridges are added to the global list,
leaking the old ones and leaving dangling objects.
This applies to devm_drm_bridge_alloc() usage below as well.
> if (ret)
> return ret;
[ ... ]
> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
> +{
> + struct device_node *np = dp->dev->of_node;
> + struct device_node *port __free(device_node) = of_graph_get_port_by_id(np, 1);
> + struct drm_bridge *bridge;
> + int count = 0;
> + int ret = 0;
> + int i;
[ ... ]
> + /* One endpoint may correspond to one next bridge. */
> + for_each_of_graph_port_endpoint(port, dp_ep) {
> + struct device_node *next_bridge_node __free(device_node) =
> + of_graph_get_remote_port_parent(dp_ep);
> +
> + bridge = of_drm_find_bridge(next_bridge_node);
> + if (!bridge) {
> + ret = -EPROBE_DEFER;
> + goto out;
> + }
> +
> + dp->next_bridge_valid = true;
> + dp->next_bridge_list[count] = drm_bridge_get(bridge);
> + count++;
[Severity: High]
Can this loop overflow the next_bridge_list array?
If the device tree specifies more than MAX_PHY (2) endpoints, this
unconditionally increments count and writes past the bounds of the array,
which would corrupt the cdn_dp_device structure.
> + }
> +
> +out:
> + dp->bridge_count = count ? count : 1;
> + return ret;
> +}
> +
> +static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> +{
> + struct cdn_dp_device *dp = dev_get_drvdata(dev);
[ ... ]
> + for (i = 0; i < dp->bridge_count; i++) {
> + encoder = &dp->bridge_list[i]->encoder.encoder;
> + bridge = &dp->bridge_list[i]->bridge;
> + next_bridge = dp->next_bridge_list[i];
> + ret = cdn_bridge_add(dev, bridge, next_bridge, encoder);
[Severity: High]
Does this error path leave an initialized encoder hanging without cleanup?
If cdn_bridge_add() succeeds at drm_simple_encoder_init() but fails on a
subsequent step like drm_bridge_connector_init(), it returns an error but
leaves the encoder initialized and attached to the drm_dev.
Additionally, if the loop here fails on the second iteration and jumps to
out, it frees resources but leaves the first successfully initialized encoder
still attached to the drm_dev. When the DRM core later traverses its encoder
lists, this could result in a use-after-free.
> + if (ret)
> + goto out;
> + }
> +
> + INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
[Severity: Medium]
Is it safe to initialize this work struct after the bridge has been exposed
to external subsystems?
The bridges are created and attached to the global chain just prior to this,
making them visible. If an external bridge or connected sink fires an HPD
notification immediately, cdn_dp_bridge_hpd_notify() could execute and call
schedule_work() before INIT_WORK() has run, potentially causing a crash.
> for (i = 0; i < dp->ports; i++) {
> port = dp->port[i];
>
> @@ -1043,7 +1250,7 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> if (ret) {
> DRM_DEV_ERROR(dev,
> "register EXTCON_DISP_DP notifier err\n");
> - return ret;
> + goto out;
[Severity: High]
This is a pre-existing issue, but does using devm_extcon_register_notifier()
in a component bind callback cause notifier chain corruption?
Similar to the bridge devm issue, since the device isn't torn down on
component unbind, the devres cleanup never fires. On a subsequent re-bind,
it registers the same notifier block again, which corrupts the linked list
of the extcon notifier chain.
> }
> }
>
> @@ -1052,15 +1259,29 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> schedule_work(&dp->event_work);
>
> return 0;
> +
> +out:
> + for (i = 0; i < MAX_PHY; i++)
> + drm_bridge_put(dp->next_bridge_list[i]);
[Severity: High]
Could this error path cause a double-put or use-after-free on bind retries?
The dp structure is allocated in probe, so it persists across component
bind/unbind cycles. When dropping references here, the pointers in
dp->next_bridge_list are not set to NULL. If a subsequent bind retry fails,
this error path might execute again and call drm_bridge_put() on the already
freed pointers.
> +
> + return ret;
> }
>
> static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
> {
> struct cdn_dp_device *dp = dev_get_drvdata(dev);
> - struct drm_encoder *encoder = &dp->encoder.encoder;
> + struct drm_encoder *encoder;
> + int i;
>
> cancel_work_sync(&dp->event_work);
> - encoder->funcs->destroy(encoder);
> + for (i = 0; i < dp->bridge_count; i++) {
> + encoder = &dp->bridge_list[i]->encoder.encoder;
> + encoder->funcs->destroy(encoder);
> + drm_bridge_put(dp->next_bridge_list[i]);
> + }
> +
> + for (i = 0; i < MAX_PHY; i++)
> + drm_edid_free(dp->edid_cache[i]);
[Severity: High]
Does this leave stale EDID pointers in the cache?
This frees the cached EDIDs but fails to set the pointers to NULL. Because
the dp context persists across component unbind/rebind, a subsequent EDID
read via cdn_dp_bridge_edid_read() might attempt to free the same stale
pointer again before replacing it, corrupting memory.
>
> pm_runtime_disable(dev);
> if (dp->fw_loaded)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260804070730.68-1-kernel@airkyi.com?part=7
More information about the linux-phy
mailing list