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 <[email protected]> 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/[email protected]?part=6
