Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] Syntax error due to a malformed diff leaving an incomplete function call. - [High] Use-After-Free due to lifecycle mismatch between drmm-allocated encoder and devm-allocated bridge. Pre-existing issues: - [High] Missing IS_ERR check on devm_drm_panel_bridge_add_typed results in potential ERR_PTR dereference. -- --- Patch [1]: [PATCH v2] drm/ingenic: fix bridge allocation --- commit b8ae6c3aa521f256f398e75faabccc79e7491a95 Author: H. Nikolaus Schaller <[email protected]> drm/ingenic: fix bridge allocation This commit modifies the Ingenic DRM driver to use the devm bridge allocation API to fix a registration warning. > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c [ ... ] > @@ -1316,10 +1316,26 @@ static int ingenic_drm_bind(struct device *dev, bool > has_components) > > drm_encoder_helper_add(encoder, > &ingenic_drm_encoder_helper_funcs); > > - ib->bridge.funcs = &ingenic_drm_bridge_funcs; > + ib->bridge = devm_drm_bridge_alloc(priv->dev, struct drm_bridge, > + base, > &ingenic_drm_bridge_funcs); [Severity: High] Does this code introduce a lifecycle mismatch? The encoder wrapper `ib` is allocated via drmm (tied to the DRM device's lifecycle) earlier in this function: ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder, NULL, DRM_MODE_ENCODER_DPI, NULL); However, the bridge is allocated here via devm (tied to the platform device). If the platform device is unbound while the DRM device is still held open by userspace, the devm infrastructure will free the bridge while the drmm-managed encoder continues to exist. Could this lead to a use-after-free when the DRM device is later cleaned up and iterates over the encoder's bridge chain containing the freed bridge pointer? > + if (IS_ERR(ib->bridge)) { > + ret = PTR_ERR(ib->bridge); > + goto err_drvdata; > + } > + > + ib->bridge->of_node = priv->dev->of_node; > ib->next_bridge = bridge; [Severity: High] This is a pre-existing issue, but does `bridge` need an error check before we assign it here? Earlier in this function, `devm_drm_panel_bridge_add_typed()` is called: if (panel) { bridge = devm_drm_panel_bridge_add_typed(dev, panel, DRM_MODE_CONNECTOR_DPI); drm_panel_put(panel); } If `devm_drm_panel_bridge_add_typed()` fails, it returns an ERR_PTR. If this unvalidated pointer is stored in `ib->next_bridge`, it will eventually be passed to `drm_bridge_attach()`, which could attempt to dereference the error pointer. > > ret = drm_bridge_attach(encoder, &ib->bridge, NULL, > + ib->bridge->interlace_allowed = true; [Severity: Critical] Does this result in a syntax error? It looks like the `ret = drm_bridge_attach(...)` call from the original code was partially left behind, leaving an unclosed function call and a trailing comma. Will the compiler fail when trying to parse this? > + > + ret = devm_drm_bridge_add(priv->dev, ib->bridge); > + if (ret) { > + dev_err(dev, "Failed to register DRM bridge: %d\n", > ret); > + goto err_drvdata; > + } > + > + ret = drm_bridge_attach(encoder, ib->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > if (ret) { > dev_err(dev, "Unable to attach bridge\n"); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/5db9a2ad762f1515431fff714fc7d6d93363ccb2.1787491144.git....@goldelico.com?part=1
