Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Double free of device node reference during display pipeline building. - [High] Missing error check for platform_device_register_data and resource leak of ovl_adaptor. - [Medium] Memory leak of device node reference on initial endpoint parsing failure. -- commit f5070502c5d9a79054533fcb309f450daf5da836 Author: WenTao Liang <[email protected]> fix: drm/mediatek: mtk_drm_probe: of_node_get in loop overwrites previous node references without release In the for_each_child_of_node loop, private->comp_node[comp_id] and private->mutex_node are assigned via of_node_get without first releasing any previously stored reference for the same index. When the same comp_id or mmsys_id matches multiple nodes, earlier node references are overwritten and permanently leaked. > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index c86a3f54f35bc..2c0d8db9ade29 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -1138,6 +1138,7 @@ static int mtk_drm_probe(struct platform_device *pdev) > > id = of_alias_get_id(node, "mutex"); > if (id < 0 || id == private->data->mmsys_id) { > + of_node_put(private->mutex_node); > private->mutex_node = of_node_get(node); > dev_dbg(dev, "get mutex for mmsys %d", > private->data->mmsys_id); > } > @@ -1153,7 +1154,7 @@ static int mtk_drm_probe(struct platform_device *pdev) > > if (!mtk_drm_find_mmsys_comp(private, comp_id)) > continue; > - > + of_node_put(private->comp_node[comp_id]); > private->comp_node[comp_id] = of_node_get(node); > > /* [Severity: High] This isn't a bug introduced by this patch, but looking at mtk_drm_probe() slightly earlier in the same function, is there a missing error check for platform_device_register_data()? drivers/gpu/drm/mediatek/mtk_drm_drv.c:mtk_drm_probe() { ... if (mtk_drm_find_mmsys_comp(private, DDP_COMPONENT_DRM_OVL_ADAPTOR)) { ovl_adaptor = platform_device_register_data(dev, "mediatek-disp-ovl-adaptor", PLATFORM_DEVID_AUTO, (void *)private->mmsys_dev, sizeof(*private->mmsys_dev)); private->ddp_comp[DDP_COMPONENT_DRM_OVL_ADAPTOR].dev = &ovl_adaptor->dev; ... } If platform_device_register_data() fails, it returns an ERR_PTR. Would taking the address of &ovl_adaptor->dev on an ERR_PTR and passing it to mtk_ddp_comp_init() result in an invalid pointer dereference? Additionally, if the registration succeeds, there doesn't seem to be a corresponding platform_device_unregister() call in the error paths (like err_node or err_pm) or in mtk_drm_remove(). Could this leak orphaned platform devices permanently on probe failures? [Severity: High] This is another pre-existing issue, but in the related mtk_drm_of_ddp_path_build_one() function, is there a potential double-free of the device node reference? drivers/gpu/drm/mediatek/mtk_drm_drv.c:mtk_drm_of_ddp_path_build_one() { ... ret = mtk_drm_of_get_ddp_ep_cid(next, 1, cpath, &next, &temp_path[idx]); of_node_put(prev); if (ret) { of_node_put(next); break; } ... } If mtk_drm_of_get_ddp_ep_cid() returns an error like -ENOENT (e.g., at the end of the pipeline) without modifying next, prev and next would point to the same node. Since of_node_put(prev) is called unconditionally, wouldn't the subsequent of_node_put(next) in the error block result in a double-free of the same node reference? [Severity: Medium] This is also a pre-existing issue, but earlier in mtk_drm_of_ddp_path_build_one(), does the error return path leak a device node reference? drivers/gpu/drm/mediatek/mtk_drm_drv.c:mtk_drm_of_ddp_path_build_one() { ... } else { if (next) dev_err(dev, "Invalid component %pOF\n", next); else dev_err(dev, "Cannot find first endpoint for path %d\n", cpath); return ret; } ... } If mtk_drm_of_get_ddp_ep_cid() populates next with a reference but returns an error, won't returning ret directly leak the reference held by next? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
