Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
New issues: - [High] `cdn_dp_connected_port()` unconditionally returns the first port on extcon systems, breaking hotplug detection. - [High] Dangling pointers in `dp->next_bridge_list` cause double-free and use-after-free on bind retry or unbind. - [High] Mismatch between `dp->ports` and `dp->bridge_count` causes uninitialized stack reads and NULL pointer dereferences. - [High] Buffer overflow when parsing DT endpoints in `cdn_dp_parse_next_bridge_dt()`. - [High] Memory and resource leaks due to devres (`devm_`) allocations in the component bind callback. - [High] Hardware multiplexing logic error allows multiple encoders to be enabled simultaneously, corrupting hardware state. - [High] Double-free / UAF of cached EDIDs upon unbind/bind cycles. - [High] `cdn_dp_switch_port()` unconditionally attempts video configuration during an EDID probe, aborting the port switch. - [Medium] Data race on multiple bridge connection state variables. Pre-existing issues: - [High] Notifier chain corruption via `devm_extcon_register_notifier()` inside the component bind callback. - [High] Use-After-Free via uncancelled extcon notifiers after component unbind. -- commit 8f73288a10a1e2cd66218daa8be78785997dc0e3 Author: Chaoyi Chen <[email protected]> Subject: drm/rockchip: cdn-dp: Add multiple bridges to support PHY port selection This commit adds support for multiple bridges to allow flexible selection of the output port on RK3399 CDN-DP controllers. It registers a separate encoder and bridge for each PHY port and parses DT connections to map them properly. > diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c > b/drivers/gpu/drm/rockchip/cdn-dp-core.c > index 9068118859e21..f2eaad61ba440 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 unconditionally return the first port on extcon systems? In cdn_dp_connected_port(), the loop evaluates if (!dp->next_bridge_valid) and returns the port immediately on the first iteration. It completely ignores the lanes connection check that was present in the old code. For systems relying on extcon for hotplug detection, this might cause the driver to always report the first port as connected to userspace, even when nothing is plugged in. > } > + > + 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]; > + } > + } > + > return NULL; > } [Severity: High] Can this loop read uninitialized memory from the lanes array? The lanes array is only populated up to dp->ports, but this loop iterates up to dp->bridge_count. If a device tree defines more endpoints than there are successfully probed PHYs, dp->bridge_count will exceed dp->ports, causing out of bounds reads on the stack memory. > @@ -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 dereference a NULL pointer? If dp->bridge_count exceeds dp->ports due to a PHY probe failure, dp_bridge->id can be greater than or equal to dp->ports. Indexing dp->port with dp_bridge->id would return NULL, leading to a kernel panic when port->phy_enabled is evaluated 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; > + } > + } > + > 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]); > + dp->edid_cache[dp_bridge->id] = drm_edid_dup(drm_edid); [Severity: High] Can this cause a double-free on the cached EDID? In cdn_dp_unbind(), the cached EDIDs are freed but the array pointers are not set to NULL. Since the dp context structure outlives the unbind and bind cycle, the next time the component is bound and this EDID read function is called, it might attempt to free the dangling pointer again. [ ... ] > @@ -551,6 +599,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) > +{ [ ... ] > + 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); > + if (ret) > + dev_err(dp->dev, "Failed to configure video: %d\n", > ret); > + } > + > +out: > + return ret; > +} [Severity: High] Will this video configuration fail and abort the port switch during EDID probing? When reading an EDID on an inactive port, cdn_dp_bridge_edid_read() calls cdn_dp_switch_port(). However, there is no display mode assigned yet during early EDID probing, so dp->mode.clock is 0. Calling cdn_dp_config_video() here calculates a symbol value of 0, which triggers an arithmetic validation loop failure returning -EINVAL. This might abort the port switch entirely. [ ... ] > @@ -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] Does this logic allow multiple encoders to be enabled simultaneously and corrupt hardware state? If a second encoder is enabled, cdn_dp_bridge_atomic_enable() sets dp->active_port, but cdn_dp_enable() will exit early because dp->active is already true. The hardware port switch is never executed. Without an atomic_check to prevent concurrent commits, the shared hardware controller might be placed into an invalid state. [ ... ] > @@ -639,9 +736,11 @@ static void cdn_dp_bridge_atomic_enable(struct > drm_bridge *bridge, struct drm_at > static void cdn_dp_bridge_atomic_disable(struct drm_bridge *bridge, struct > drm_atomic_state *state) > { > struct cdn_dp_device *dp = bridge_to_dp(bridge); > + struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge); > int ret; > > mutex_lock(&dp->lock); > + dp_bridge->enabled = false; > > if (dp->active) { > ret = cdn_dp_disable(dp); [Severity: High] Will this unconditionally disable the shared hardware regardless of other active encoders? Because the DP controller is shared, if one encoder is disabled while another is still active, cdn_dp_disable() turns off the shared hardware controller. This would instantly crash the stream for the other active encoder. [ ... ] > @@ -828,6 +927,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; > + schedule_work(&dp->event_work); > +} [Severity: Medium] Is this updating connection state without holding the appropriate lock? The connection flag is modified here without acquiring dp->lock. At the same time, cdn_dp_pd_event_work() reads these connection states iteratively while explicitly dropping dp->lock. This might cause a data race when evaluating compound connection flags for reporting to userspace. [ ... ] > @@ -1003,26 +1106,35 @@ 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) > +{ > + 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 overflow the next_bridge_list array? The for_each_of_graph_port_endpoint loop iterates over all endpoints defined in the device tree and stores each resolved bridge in dp->next_bridge_list[count]. Since next_bridge_list is a fixed-size array of MAX_PHY (2), a device tree defining 3 or more endpoints would cause a buffer overflow, corrupting adjacent driver memory. [ ... ] > + for (i = 0; i < dp->bridge_count; i++) { > + dp_bridge = devm_drm_bridge_alloc(dev, struct cdn_dp_bridge, > bridge, > + &cdn_dp_bridge_funcs); > + if (IS_ERR(dp_bridge)) { > + ret = PTR_ERR(dp_bridge); > + goto out; > + } > + dp_bridge->id = i; > + dp_bridge->parent = dp; > + if (!dp->next_bridge_valid) > + dp_bridge->connected = true; > + dp->bridge_list[i] = dp_bridge; > + } [Severity: High] Does allocating devm resources inside the component bind callback cause memory leaks? The component framework requires resources allocated in bind() to be freed in unbind(). The driver uses devm_drm_bridge_alloc() and then calls devm_drm_bridge_add() inside cdn_bridge_add(). These resources are tied to the lifetime of the component device, not the bind cycle. Every time the component is bound, such as after an -EPROBE_DEFER or unbind/rebind, new bridge memory is allocated and new bridges are registered, leaking memory and piling up duplicate DRM bridges. [ ... ] > for (i = 0; i < dp->ports; i++) { > port = dp->port[i]; > > @@ -1045,7 +1252,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 the bind callback corrupt the notifier chain? Because devm_extcon_register_notifier is managed by device removal, it does not unregister the notifier in cdn_dp_unbind(). When the component is bound a second time, the exact same event_nb object is re-registered to the extcon notifier chain. Adding an already-linked node to a list creates an infinite cycle, leading to a hard lockup the next time the notifier fires. > @@ -1054,15 +1261,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]); > + > + 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]); > + } [Severity: High] Could this leave dangling pointers in dp->next_bridge_list causing a double-free? If cdn_dp_bind() encounters an error and jumps to the out block, or when cdn_dp_unbind() is called, the driver calls drm_bridge_put() on the pointers but fails to set them to NULL. Since the dp context structure is allocated in probe() and survives component bind retries, a subsequent bind failure or unbind will call drm_bridge_put() on the exact same dangling pointers. [Severity: High] This isn't a bug introduced by this patch, but does failing to unregister the notifiers during unbind cause a use-after-free? The driver correctly calls cancel_work_sync(&dp->event_work), but fails to unregister the event sources because devm_extcon_register_notifier() is managed by device removal. If a hotplug event fires between unbind() and remove(), or if the work executes concurrently, it will call schedule_work(&dp->event_work) on memory that may have already been freed. > + > + for (i = 0; i < MAX_PHY; i++) > + drm_edid_free(dp->edid_cache[i]); [Severity: High] Are these pointers left dangling after being freed? The cached EDIDs are freed via drm_edid_free() here, but the array indices are not reset to NULL. The dp context outlives the unbind/bind cycle, meaning cdn_dp_bridge_edid_read() might later attempt to free these dangling pointers again. > pm_runtime_disable(dev); > if (dp->fw_loaded) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
