Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Use-After-Free due to devres and drmm lifetime mismatch. - [High] Use-After-Free caused by explicit devm_kfree() without drm_plane_cleanup(). - [Medium] OF node reference leak when breaking out of for_each_child_of_node(). -- commit a08081e9643ffc88f8c23d8338f577d6209738f8 Author: Jason Xiang <[email protected]> drm/logicvc: Fix port node reference leak in logicvc_crtc_init() This commit uses devm_add_action_or_reset() to drop the port node reference obtained from of_graph_get_port_by_id(), fixing a node reference leak on unbind and probe error paths. > diff --git a/drivers/gpu/drm/logicvc/logicvc_crtc.c > b/drivers/gpu/drm/logicvc/logicvc_crtc.c > index 81e5b80736fa7..4de48822ca21b 100644 > --- a/drivers/gpu/drm/logicvc/logicvc_crtc.c > +++ b/drivers/gpu/drm/logicvc/logicvc_crtc.c [ ... ] > @@ -274,6 +279,10 @@ int logicvc_crtc_init(struct logicvc_drm *logicvc) > > crtc->drm_crtc.port = of_graph_get_port_by_id(of_node, 1); > > + ret = devm_add_action_or_reset(dev, logicvc_crtc_of_node_put, > crtc->drm_crtc.port); > + if (ret) > + return ret; [Severity: High] This isn't a bug introduced by this patch, but this new error return introduces another path that triggers a use-after-free due to a lifetime mismatch between devres and drmm. The CRTC is allocated using devres: drivers/gpu/drm/logicvc/logicvc_crtc.c:logicvc_crtc_init() { ... crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL); ... } However, the DRM device is also managed by devres: drivers/gpu/drm/logicvc/logicvc_drm.c:logicvc_drm_probe() { ... logicvc = devm_drm_dev_alloc(dev, &logicvc_drm_driver, struct logicvc_drm, drm_dev); ... } Since devres executes in reverse order of allocation, the CRTC and planes are freed by devm_kzalloc unwinding before devm_drm_dev_alloc unwinding calls drm_dev_put(). When drm_dev_put() runs, it calls drm_mode_config_cleanup(), which iterates over the mode_config lists and calls .destroy() hooks on the already-freed CRTC and plane structures. Can we convert the CRTC allocation to use drmm_kzalloc() or similar DRM-managed APIs to prevent this memory corruption on teardown? While reviewing this, I noticed a couple of other pre-existing issues in the driver's layer initialization. [Severity: Medium] This is a pre-existing issue, but does logicvc_layers_init() leak the device node reference if layer initialization fails? Looking at logicvc_layers_init() in drivers/gpu/drm/logicvc/logicvc_layer.c: drivers/gpu/drm/logicvc/logicvc_layer.c:logicvc_layers_init() { ... for_each_child_of_node(layers_node, layer_node) { ... ret = logicvc_layer_init(logicvc, layer_node, index); if (ret) { of_node_put(layers_node); goto error; } ... } The for_each_child_of_node() macro automatically drops the reference on the previous child node and acquires it for the next one. If the loop is terminated early via the goto error path, wouldn't the caller need to manually drop the reference to the current layer_node using of_node_put()? [Severity: High] This is a pre-existing issue, but does the error unwinding path in logicvc_layers_init() cause a use-after-free in the DRM core? If logicvc_layer_init() succeeds for at least one layer but fails for a subsequent layer, the error path iterates over the successfully initialized layers: drivers/gpu/drm/logicvc/logicvc_layer.c:logicvc_layers_init() { ... error: list_for_each_entry_safe(layer, next, &logicvc->layers_list, list) logicvc_layer_fini(logicvc, layer); ... } This calls logicvc_layer_fini(), which explicitly frees the layer memory: drivers/gpu/drm/logicvc/logicvc_layer.c:logicvc_layer_fini() { ... devm_kfree(dev, layer); } The layer's drm_plane was already added to the DRM device's mode_config.plane_list during initialization. Because drm_plane_cleanup() is never called to unlink the plane from the core list, the explicitly freed memory remains accessible to the DRM core. When probe ultimately unwinds and drm_mode_config_cleanup() is called, will it dereference the freed plane? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
