Re: [PATCH v5 6/6] 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 7 potential issue(s) to consider:

New issues:
- [High] Stale pointers in `dp->next_bridge_list` cause a double free / refcount underflow on repeated or failed bind cycles.
- [High] `cdn_dp_switch_port` unconditionally configures video, breaking EDID reads for newly connected monitors.
- [High] Missing `atomic_check` allows simultaneous activation of multiple conflicting encoders.
- [High] `cdn_dp_connected_port` bypasses the physical lane check for extcon configurations, breaking hotplug detection.
- [High] Index mismatch between packed PHY ports and DT endpoints causes uninitialized memory reads and guaranteed NULL dereferences.
- [Medium] Device tree endpoint iteration lacks a bounds check against `MAX_PHY`, leading to a heap buffer overflow.

Pre-existing issues:
- [High] Extcon notifiers can schedule work after `cancel_work_sync()`, leading to Use-After-Free.
--

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

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

This commit adds support for multiple bridges in the Rockchip CDN-DP
driver to enable PHY port selection via the DRM AUX HPD bridge framework.
It parses device tree endpoints to register one or two bridges and handles
switching the DP controller output between them based on plug states.

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index 74e1f5422a6e2..5e513f6930cb9 100644
> --- 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 break hotplug detection for legacy extcon-based devices?

If dp->next_bridge_valid is false, this loop unconditionally returns
dp->port[0] on the first iteration without verifying if the physical lanes
are greater than zero. This causes the driver to perpetually report the
display as connected.

>  	}
> +
> +	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]
Can this loop read uninitialized stack memory?

The lanes array is populated in the previous loop up to dp->ports, which
tracks successfully probed PHYs. However, this loop iterates up to
dp->bridge_count, which tracks DT endpoints. If a device tree specifies
more endpoints than successfully probed PHYs, this will read out of bounds
on the stack.

> +		}
> +	}
> +
>  	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];
> +	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) {

[Severity: High]
Is there a risk of a NULL pointer dereference here?

If the device tree has more endpoints than successfully probed PHYs,
dp_bridge->id could exceed the bounds of initialized dp->port pointers.
In that scenario, port would be NULL, and evaluating !port->phy_enabled
will crash.

> +		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);

[Severity: High]
Will this sequence fail to read the EDID for a newly connected monitor?

Switching the port here calls cdn_dp_switch_port(), which unconditionally
executes cdn_dp_config_video(dp). During EDID probing, no DRM display mode
has been committed yet, so dp->mode.clock is 0. 

This causes cdn_dp_config_video() to fail its bandwidth calculation loop
and return -EINVAL, which aborts the port switch and forces the driver to
fall back to a cached EDID.

> +			if (ret)
> +				goto get_cache;
> +		}
> +	}
[ ... ]
> @@ -838,6 +948,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,
>  
>  	.dp_audio_prepare = cdn_dp_audio_prepare,

[Severity: High]
Because there is only a single DP hardware controller backing multiple
potential bridges, is there a risk of hardware state corruption if
userspace commits a state that maps CRTCs to both ports simultaneously?

Without an .atomic_check callback in cdn_dp_bridge_funcs to enforce mutual
exclusivity, cdn_dp_bridge_atomic_enable() could execute for the second
port and overwrite the video configuration while the first is active.

[ ... ]
> @@ -1030,6 +1142,101 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
[ ... ]
> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp)
> +{
[ ... ]
> +	/* 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]
Is there a potential heap buffer overflow here?

The count variable is incremented and used as an index for
dp->next_bridge_list[count], but there is no bounds check against MAX_PHY.
If a misconfigured device tree provides 3 or more endpoints, this will
overwrite adjacent fields in the cdn_dp_device structure.

> +	}
[ ... ]
> @@ -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]
Can this result in a double free and refcount underflow?

If cdn_dp_bind() fails and executes this error path, it calls
drm_bridge_put() but leaves the pointers intact in dp->next_bridge_list.
If the master bind is retried later and fails again, it will perform
another drm_bridge_put() on the same stale pointers. 

The same pattern also appears below during unbind.

> +
> +	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);

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free
during unbind?

Extcon notifiers are registered using devm_extcon_register_notifier().
While cancel_work_sync(&dp->event_work) cancels pending work here, the
devm-managed notifier remains active until devres cleanup, which happens
after unbind returns. 

A hotplug event in this window could reschedule dp->event_work, executing
it after the structures are freed.

> -	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]);
> +	}

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

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