Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm: NULL pointer dereference due to inverted drmm cleanup order - [Medium] drm: memory leak on connector initialization error paths - [Medium] drm: missing IS_ERR check causes invalid pointer dereference --
commit a2cbdd2f3a09967f7b7cc240d15ab5e45033083e Author: Luca Ceresoli <[email protected]> drm/display: bridge-connector: use a dynamic connector This commit switches the DRM bridge connector initialization from drmm-managed functions to dynamic variants. This prepares the driver for bridge hotplugging, allowing connectors to be removed without removing the entire card. > diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c > b/drivers/gpu/drm/display/drm_bridge_connector.c > index 2aff745f01478..1c222e27767d4 100644 > --- a/drivers/gpu/drm/display/drm_bridge_connector.c > +++ b/drivers/gpu/drm/display/drm_bridge_connector.c [ ... ] > @@ -1113,6 +1120,9 @@ drm_bridge_connector_initialize(struct > drm_bridge_connector *bridge_connector, > > drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs); > > + if (!connector->state) > + connector->state = drm_bridge_connector_create_state(connector); > + [Severity: Medium] Does this code assign an error pointer directly to connector->state? If drm_bridge_connector_create_state() returns ERR_PTR(-ENOMEM), this will poison the state pointer without validation, potentially leading to an invalid pointer dereference later. > if (bridge_connector->bridge_hpd) > connector->polled = DRM_CONNECTOR_POLL_HPD; > else if (bridge_connector->bridge_detect) [ ... ] > @@ -1130,9 +1140,26 @@ drm_bridge_connector_initialize(struct > drm_bridge_connector *bridge_connector, > if (ret) > return ERR_PTR(ret); > > + ret = drm_connector_dynamic_register(connector); > + if (ret) > + return ERR_PTR(ret); > + [Severity: Medium] Does this code skip manual cleanup on this error path? Since the switch to dynamic initialization, the connector is referenced and has to be put if initialization fails (as noted in an earlier comment: "From now on the connector is referenced and has to be put"). Returning ERR_PTR(ret) directly skips drm_connector_put() and bridge cleanup, leaking the initialized dynamic connector and bridge references. This also seems to occur on earlier error paths in this function, such as after drm_connector_hdmi_audio_init(). > return connector; > } [ ... ] > @@ -1155,6 +1182,7 @@ struct drm_connector *drm_bridge_connector_init(struct > drm_device *drm, > { > struct drm_bridge_connector *bridge_connector; > struct drm_connector *connector; > + int ret; > > bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), > GFP_KERNEL); > if (!bridge_connector) > @@ -1164,6 +1192,10 @@ struct drm_connector *drm_bridge_connector_init(struct > drm_device *drm, > if (IS_ERR(connector)) > return connector; > > + ret = drmm_add_action_or_reset(drm, drmm_bridge_connector_fini, > bridge_connector); > + if (ret) > + return ERR_PTR(ret); > + [Severity: High] Does this inverted drmm cleanup order cause a NULL pointer dereference? This drmm action is registered after the CEC drmm action (which happens inside drm_bridge_connector_initialize()). Since drmm actions run in LIFO order, the connector is destroyed and zeroed out before the CEC adapter unregisters. The CEC unregister action would then dereference connector->cec.data, which is already NULL, leading to a crash. > return connector; > } While reviewing this patch, I noticed a pre-existing issue in the HDMI dynamic initialization path that this patch calls into: drivers/gpu/drm/drm_connector.c:drm_connector_hdmi_dynamic_init() { ... ret = drm_connector_dynamic_init(dev, connector, funcs, connector_type, ddc); if (ret) return ret; if (ret) drm_connector_cleanup(connector); return drm_connector_hdmi_initialize(dev, connector, hdmi_funcs); } [Severity: Medium] This is a pre-existing issue, but does this code contain a dead cleanup path? The code evaluates if (ret) return ret; followed immediately by if (ret) drm_connector_cleanup(connector);. The cleanup is dead code. If the subsequent drm_connector_hdmi_initialize() fails, the successfully initialized connector is leaked. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
