Re: [PATCH v4 5/5] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:

New issues:
- [High] `cdn_dp_connected_port` unconditionally returns the first port on legacy or extcon devices, breaking hotplug detection.
- [High] The driver erroneously skips valid `extcon` devices during probe due to a flawed error pointer check.
- [High] `cdn_dp_switch_port` performs full video configuration during an EDID read, breaking port switching and EDID detection.
- [High] Multiple encoders are registered without an `atomic_check` callback to enforce mutual exclusion, enabling hardware corruption.
- [High] Dangling pointers in `dp->next_bridge_list` cause a double-put Use-After-Free during component rebinds or deferred probes.
- [High] Dangling pointers in `dp->edid_cache` cause a double-free memory corruption during component rebinds.
- [High] Lack of bounds validation between DT endpoints (`bridge_count`) and probed PHYs (`ports`) leads to NULL pointer dereferences and uninitialized memory access.
- [High] `cdn_dp_bridge_atomic_enable` bypasses PHY switching if the CDN-DP controller is already active.
- [High] Failed PHY switching in `cdn_dp_switch_port` leaves the device in an inconsistent state.
- [Medium] Out-of-bounds write in `cdn_dp_parse_next_bridge_dt` when parsing device tree endpoints.
- [Medium] Data race on the `connected` flag of the bridges.

Pre-existing issues:
- [Critical] `devm_` allocations inside the component `bind` callback cause resource leaks, duplicate registrations, and Use-After-Free vulnerabilities.
--

--- Patch [5]: [PATCH v4 5/5] drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection ---
commit ae89dd4402c4e98ab965c5bd29f2eb19fa5d48bc
Author: Chaoyi Chen <chaoyi.chen@rock-chips.com>

drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection

The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And
the CDN-DP can be switched to output to one of the PHYs. If both ports
are plugged into DP, DP will select the first port for output.

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index 1c1c1116b5ebe..251e76692eb63 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
[ ... ]
> @@ -191,14 +192,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]
What happens here for legacy or extcon devices where `next_bridge_valid` is
false?

If `!dp->next_bridge_valid` evaluates to true, the loop immediately returns
`dp->port[0]` on the very first iteration. This seems to completely ignore
the physical connection state in `lanes[i]`, always falsely reporting the port
as connected, which could break DP hotplug detection entirely.

>  	}
> +
> +	if (dp->next_bridge_valid) {
[ ... ]
> @@ -253,12 +267,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]
Is it possible for a device tree to define more endpoints than probed PHYs?

There doesn't seem to be a bounds check ensuring `dp->bridge_count` is less
than or equal to `dp->ports`. If a misconfiguration causes `dp_bridge->id`
to index beyond `dp->ports`, `dp->port[dp_bridge->id]` would return NULL.
This would cause a kernel panic when checking `!port->phy_enabled` below.

> +	struct cdn_dp_port *prev_port = NULL;
> +	const struct drm_edid *drm_edid = NULL;
> +	int i, ret;
>  
>  	mutex_lock(&dp->lock);
> +
> +	/* More than one port is available */
> +	if (dp->bridge_count > 1 && !port->phy_enabled) {
> +		for (i = 0; i < dp->bridge_count; i++) {
> +			/* Another port already enable */
> +			if (dp->bridge_list[i] != dp_bridge && dp->bridge_list[i]->enabled)
> +				goto get_cache;
> +			/* Find already enabled port */
> +			if (dp->port[i]->phy_enabled)
> +				prev_port = dp->port[i];
> +		}
> +
> +		/* Switch to current port */
> +		if (prev_port) {
> +			ret = cdn_dp_switch_port(dp, prev_port, port);
> +			if (ret)
> +				goto get_cache;
> +		}
> +	}

[Severity: High]
Are we trying to perform full video configuration during an EDID read?

When switching the PHY, `cdn_dp_switch_port` internally calls
`cdn_dp_config_video(dp)`. However, during an EDID read operation, no display
mode is committed yet, meaning `mode->clock` may be 0. Since
`cdn_dp_config_video` uses `mode->clock` in a timing loop, this will fail.

This forces `cdn_dp_switch_port` to fail, resulting in a fallback to a
stale or NULL EDID cache, completely breaking output detection on secondary
ports.

> +
>  	drm_edid = drm_edid_read_custom(connector, cdn_dp_get_edid_block, dp);
> +	/* replace edid cache */
> +	if (dp->edid_cache[dp_bridge->id])
> +		drm_edid_free(dp->edid_cache[dp_bridge->id]);

[Severity: High]
Could this result in a double-free on a dangling pointer?

In `cdn_dp_unbind()`, the cached EDIDs are freed but the pointers in
`dp->edid_cache` are not set to NULL. Since the `dp` context survives a
component unbind/rebind cycle, if the component is rebound, this check will
evaluate to true and attempt to free the dangling pointer again.

> +	dp->edid_cache[dp_bridge->id] = drm_edid_dup(drm_edid);
> +
>  	mutex_unlock(&dp->lock);
> +	return drm_edid;
[ ... ]
> @@ -550,6 +598,50 @@ static bool cdn_dp_check_link_status(struct cdn_dp_device *dp)
>  	return drm_dp_channel_eq_ok(link_status, min(port->lanes, sink_lanes));
>  }
>  
> +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port *prev_port,
> +			      struct cdn_dp_port *port)
> +{
> +	int ret;
> +
> +	ret = cdn_dp_disable_phy(dp, prev_port);
> +	if (ret)
> +		goto out;
> +	ret = cdn_dp_enable_phy(dp, port);
> +	if (ret)
> +		goto out;
> +
> +	ret = cdn_dp_get_sink_capability(dp);
> +	if (ret) {
> +		cdn_dp_disable_phy(dp, port);
> +		goto out;
> +	}

[Severity: High]
Does this error path leave the driver in an inconsistent state?

If `cdn_dp_get_sink_capability` fails, the code disables both the previous
and target PHYs and returns an error. However, it fails to update the
controller's internal state machine, leaving `dp->active` as true.

Because `dp->active` is true but no PHY is actually enabled, any subsequent
hardware operations will assume the hardware is active, potentially leading
to bus errors or hardware timeouts.

> +
> +	dp->lanes = port->lanes;
> +
> +	if (!cdn_dp_check_link_status(dp)) {
[ ... ]
> @@ -599,6 +692,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]
Will `cdn_dp_enable` actually switch the PHY if one is already active?

The driver sets `dp->active_port = dp_bridge->id` and calls `cdn_dp_enable(dp)`.
However, `cdn_dp_enable` contains an early return if `dp->active` is already
true.

If Port A is active and an atomic commit requests enabling Port B,
`cdn_dp_enable` will return immediately without powering on Port B's PHY.
Subsequent video configuration and link training commands will then be sent
while the target PHY is powered off.

>  	if (ret) {
>  		DRM_DEV_ERROR(dp->dev, "Failed to enable bridge %d\n",
[ ... ]
> @@ -831,6 +930,17 @@ static int cdn_dp_audio_mute_stream(struct drm_bridge *bridge,
>  	return ret;
>  }
>  
> +static void cdn_dp_bridge_hpd_notify(struct drm_bridge *bridge,
> +				     struct drm_connector *connector,
> +				     enum drm_connector_status status)
> +{
> +	struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge);
> +	struct cdn_dp_device *dp = bridge_to_dp(bridge);
> +
> +	dp->bridge_list[dp_bridge->id]->connected = status == connector_status_connected;

[Severity: Medium]
Could this assignment cause a data race?

This writes to `dp->bridge_list[dp_bridge->id]->connected` without holding
`dp->lock`. Concurrently, this flag is read by `cdn_dp_pd_event_work` inside
a loop (after releasing `dp->lock`), and by `cdn_dp_bridge_mode_valid`
without holding any locks. This unsynchronized access might lead to stale reads
across cores.

> +	schedule_work(&dp->event_work);
> +}
> +
>  static const struct drm_bridge_funcs cdn_dp_bridge_funcs = {
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> @@ -841,6 +951,7 @@ static const struct drm_bridge_funcs cdn_dp_bridge_funcs = {
>  	.atomic_disable = cdn_dp_bridge_atomic_disable,
>  	.mode_valid = cdn_dp_bridge_mode_valid,
>  	.mode_set = cdn_dp_bridge_mode_set,
> +	.hpd_notify = cdn_dp_bridge_hpd_notify,

[Severity: High]
Should this bridge implement an `atomic_check` callback to enforce mutual
exclusion?

The RK3399 CDN-DP controller can only drive one port at a time. By registering
multiple encoders without an `atomic_check` to prevent both outputs from being
enabled simultaneously, userspace could commit an atomic state that enables both.

This would cause concurrent calls to `cdn_dp_bridge_atomic_enable`, overwriting
shared registers and leading to hardware corruption or kernel timeouts.

>  
>  	.dp_audio_prepare = cdn_dp_audio_prepare,
>  	.dp_audio_mute_stream = cdn_dp_audio_mute_stream,
[ ... ]
> @@ -1033,6 +1145,101 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>  		return ret;
>  	}
>  
> +	if (last_bridge) {
> +		fwnode_handle_put(connector->fwnode);
> +		connector->fwnode = fwnode_handle_get(of_fwnode_handle(last_bridge->of_node));
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	/* If device use extcon, do not use hpd bridge */
> +	for (i = 0; i < dp->ports; i++) {
> +		if (dp->port[i]->extcon) {
> +			dp->bridge_count = 1;
> +			return 0;
> +		}
> +	}
> +
> +	/* 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: Medium]
What prevents an out-of-bounds write here if there are more than 2 endpoints?

The `dp->next_bridge_list` array is statically sized to `MAX_PHY` (2). The loop
blindly increments `count` and writes to the array without bounds checking. A
misconfigured device tree could easily overflow this array.

> +	}
> +
> +out:
> +	dp->bridge_count = count ? count : 1;
> +	return ret;
> +}
> +
> +static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
> +{
[ ... ]
> +
> +	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
> +
>  	for (i = 0; i < dp->ports; i++) {
>  		port = dp->port[i];
>  
> @@ -1046,7 +1253,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: Critical]
This is a pre-existing issue, but won't these `devm_` allocations cause memory
leaks and Use-After-Free during component unbind?

The driver uses `devm_drm_bridge_alloc`, `devm_drm_bridge_add`, and
`devm_extcon_register_notifier` inside `cdn_dp_bind`. These tie resources to
the platform device (`dp->dev`), not the component lifecycle.

Upon unbind, these allocations aren't freed, leaking memory on every rebind.
Critically, the extcon notifier is never unregistered. If an event fires after
unbind, `cdn_dp_pd_event_work` is executed, discovering `dp->fw_loaded == true`
and attempting to dereference `dp->fw->data` which was released in `unbind`,
leading to a kernel panic.

>  
> @@ -1055,15 +1262,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]
Does leaving these pointers in the array cause a Use-After-Free on rebind?

The array elements aren't set to NULL after calling `drm_bridge_put()`. Since
`dp` persists across component bind/unbind cycles, a subsequent `bind` failure
would iterate over this array and put the dangling pointers a second time,
corrupting the bridge refcounts.

> +
> +	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]
Same as above, should `dp->edid_cache[i]` and `dp->next_bridge_list[i]` be
cleared to NULL after freeing?

These dangling pointers will cause double-frees during a subsequent bind cycle
or EDID read if the component is rebound.

>  
>  	pm_runtime_disable(dev);
>  	if (dp->fw_loaded)
> @@ -1114,10 +1335,10 @@ static int cdn_dp_probe(struct platform_device *pdev)
>  	int ret;
>  	int i;
>  
> -	dp = devm_drm_bridge_alloc(dev, struct cdn_dp_device, bridge,
> -				   &cdn_dp_bridge_funcs);
> -	if (IS_ERR(dp))
> -		return PTR_ERR(dp);
> +	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
> +	if (!dp)
> +		return -ENOMEM;
> +
>  	dp->dev = dev;
>  
>  	match = of_match_node(cdn_dp_dt_ids, pdev->dev.of_node);

[Severity: High]
This isn't a bug introduced by this patch, but does the probe fail to
recognize valid extcon devices?

Just below this in `cdn_dp_probe`, there is a check:

    if (IS_ERR(phy) || PTR_ERR(extcon) != -ENODEV)
        continue;

If `extcon_get_edev_by_phandle()` successfully returns a valid pointer, its
address is evaluated against `-ENODEV`, which is always true. This skips the
valid extcon port and leaves `dp->ports` as 0, failing the probe with `-EINVAL`.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727064408.101-1-kernel@airkyi.com?part=5

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy