Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing cleanup on error paths in `ch7033_bridge_attach()` causes a resource leak and use-after-free. - [High] `ch7033_bridge_detach()` incorrectly cleans up uninitialized resources if the bridge was attached with `DRM_BRIDGE_ATTACH_NO_CONNECTOR`. - [High] HPD is enabled before connector initialization is complete, causing a NULL pointer dereference race condition. -- commit 0d74a63f280c9b3d909b1d5c59227a795c81f570 Author: Piyush Patle <[email protected]> drm/bridge: chrontel-ch7033: replace deprecated DRM_ERROR macro Replace DRM_ERROR() with device-aware drm_err() using bridge->dev. No functional changes. > diff --git a/drivers/gpu/drm/bridge/chrontel-ch7033.c > b/drivers/gpu/drm/bridge/chrontel-ch7033.c > index 54d49d4882c85..0e845f789f2e9 100644 > --- a/drivers/gpu/drm/bridge/chrontel-ch7033.c > +++ b/drivers/gpu/drm/bridge/chrontel-ch7033.c > @@ -302,7 +302,7 @@ static int ch7033_bridge_attach(struct drm_bridge *bridge, > priv->next_bridge->type, > priv->next_bridge->ddc); > if (ret) { > - DRM_ERROR("Failed to initialize connector\n"); > + drm_err(bridge->dev, "Failed to initialize connector\n"); > return ret; > } [Severity: High] This isn't a bug introduced by this patch, but does this early return leak resources? If drm_connector_init_with_ddc() fails here in ch7033_bridge_attach(), we return without calling drm_bridge_hpd_disable(). Does this leave a dangling pointer in next_bridge for the HPD callback (ch7033_hpd_event) and its data (priv)? If an HPD event fires later after the device probing failed, could it lead to a use-after-free? Also, if the subsequent drm_connector_attach_encoder() call fails: drivers/gpu/drm/bridge/chrontel-ch7033.c:ch7033_bridge_attach() { ... if (ret) { drm_err(bridge->dev, "Failed to initialize connector\n"); return ret; } return drm_connector_attach_encoder(&priv->connector, encoder); } Does returning that error directly fail to roll back drm_connector_init_with_ddc() and leave the HPD enabled, leaking the connector structure and the registered callback? [Severity: High] This is also a pre-existing issue, but is there a race condition with when HPD is enabled in this same function? Looking at ch7033_bridge_attach() in drivers/gpu/drm/bridge/chrontel-ch7033.c: drivers/gpu/drm/bridge/chrontel-ch7033.c:ch7033_bridge_attach() { ... if (priv->next_bridge->ops & DRM_BRIDGE_OP_HPD) { drm_bridge_hpd_enable(priv->next_bridge, ch7033_hpd_event, priv); } drm_connector_helper_add(connector, &ch7033_connector_helper_funcs); ret = drm_connector_init_with_ddc(...); ... } Since drm_bridge_hpd_enable() is called before drm_connector_init_with_ddc(), if an HPD event fires immediately, ch7033_hpd_event() is invoked. That callback checks if priv->bridge.dev is set (which is true here) and then passes priv->connector.dev to drm_helper_hpd_irq_event(). Since priv->connector.dev is only populated later during drm_connector_init_with_ddc(), will it still be NULL here, causing drm_helper_hpd_irq_event() to dereference a NULL pointer? [Severity: High] Finally, this is another pre-existing issue, but does ch7033_bridge_detach() clean up uninitialized resources? If ch7033_bridge_attach() is called with DRM_BRIDGE_ATTACH_NO_CONNECTOR, it returns early and skips drm_connector_init_with_ddc(). However, ch7033_bridge_detach() unconditionally calls: drivers/gpu/drm/bridge/chrontel-ch7033.c:ch7033_bridge_detach() { ... drm_connector_cleanup(&priv->connector); } Since priv is allocated with devm_drm_bridge_alloc() (which zeroes memory), priv->connector contains zero-initialized list heads. Because drm_connector_cleanup() iterates over &connector->probed_modes using list_for_each_entry_safe(), could this dereference NULL list pointers and cause a kernel crash when the uninitialized connector is cleaned up? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
