Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [Low] Undeclared identifier `dev` in `kmb_dsi_init()` breaks the build. Pre-existing issues: - [High] Devres-managed resources are bound to the wrong device, causing memory leaks on probe deferrals. - [High] The MIPI DSI host remains registered indefinitely if the DRM probe defers, leading to a Use-After-Free upon module unload. - [High] Calling `drm_crtc_cleanup()` on an uninitialized CRTC causes a NULL pointer dereference and kernel panic. - [High] Ignored return value from `kmb_dsi_encoder_init()` leads to incomplete DRM initialization and crashes. - [High] Ignored return value from `kmb_initialize_clocks()` leads to hardware operations without enabled clocks. -- commit fe279368e5a0418668dc2dae4e274724b0887c0b Author: Yuho Choi <[email protected]> drm/kmb: Fix device reference leaks in probe kmb_probe() takes a reference to its own platform device only to keep a local dev pointer, and passes an extra reference to the DSI platform device into kmb_dsi_host_bridge_init(). Neither reference is released. > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c > index 7c2eb1152fc2f..f8ef618e3a810 100644 > --- a/drivers/gpu/drm/kmb/kmb_drv.c > +++ b/drivers/gpu/drm/kmb/kmb_drv.c [ ... ] > @@ -515,20 +515,24 @@ static int kmb_probe(struct platform_device *pdev) > > of_node_put(dsi_in); > of_node_put(dsi_node); > - ret = kmb_dsi_host_bridge_init(get_device(&dsi_pdev->dev)); > + ret = kmb_dsi_host_bridge_init(&dsi_pdev->dev); > > - if (ret == -EPROBE_DEFER) { > - return -EPROBE_DEFER; > - } else if (ret) { > - DRM_ERROR("probe failed to initialize DSI host bridge\n"); > - return ret; > + if (ret) { > + if (ret != -EPROBE_DEFER) { > + DRM_ERROR("probe failed to initialize DSI host > bridge\n"); > + kmb_dsi_host_unregister(NULL); > + } > + goto err_put_dsi_pdev; > } [Severity: High] This isn't a bug introduced by this patch, but if kmb_probe() returns -EPROBE_DEFER, it explicitly skips calling kmb_dsi_host_unregister(NULL). If the module is subsequently unloaded while still in this deferred state, could this cause a use-after-free since the globally allocated dsi_host would remain registered in the MIPI DSI core's internal list pointing to unloaded memory? [ ... ] > diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c > index 59d0e856392fc..a7df845088842 100644 > --- a/drivers/gpu/drm/kmb/kmb_dsi.c > +++ b/drivers/gpu/drm/kmb/kmb_dsi.c [ ... ] > @@ -1410,7 +1430,6 @@ int kmb_dsi_mode_set(struct kmb_dsi *kmb_dsi, struct > drm_display_mode *mode, > struct kmb_dsi *kmb_dsi_init(struct platform_device *pdev) > { > struct kmb_dsi *kmb_dsi; > - struct device *dev = get_device(&pdev->dev); > > kmb_dsi = devm_kzalloc(dev, sizeof(*kmb_dsi), GFP_KERNEL); [Severity: Low] Does the removal of the dev variable declaration cause a build failure? It looks like dev is still being used here in devm_kzalloc() without being declared in kmb_dsi_init(). [Severity: High] This is a pre-existing issue, but is the memory allocation tied to the wrong platform device? The devm_kzalloc() call resolves dev to dsi_pdev->dev, which is the device returned by of_find_device_by_node(), rather than the device actively being probed. Since dsi_pdev isn't cleaned up when kmb_probe() returns -EPROBE_DEFER, won't this leak a new kmb_dsi structure into dsi_pdev's devres list on every probe deferral? [Severity: High] This is a pre-existing issue, but does the error path in kmb_probe() cause a NULL pointer dereference? If kmb_setup_mode_config() fails before kmb->crtc is initialized, the error path unconditionally jumps here: drivers/gpu/drm/kmb/kmb_drv.c:kmb_probe() { ... err_free: drm_crtc_cleanup(&kmb->crtc); ... } Since devm_drm_dev_alloc() zeroes the memory, kmb->crtc.dev is NULL. Calling drm_crtc_cleanup() on this uninitialized CRTC would dereference this NULL pointer. [Severity: High] This is a pre-existing issue, but are we ignoring the return value from kmb_dsi_encoder_init() here? drivers/gpu/drm/kmb/kmb_drv.c:kmb_setup_mode_config() { ... ret = kmb_dsi_encoder_init(drm, kmb->kmb_dsi); /* Set the CRTC's port so that the encoder component can find it */ kmb->crtc.port = of_graph_get_port_by_id(drm->dev->of_node, 0); ret = drm_vblank_init(drm, drm->mode_config.num_crtc); ... } The result in ret is immediately overwritten by drm_vblank_init(). If encoder initialization fails, the driver proceeds to register the DRM device with an incomplete display pipeline, which could lead to crashes during subsequent modeset operations. [Severity: High] This is a pre-existing issue, but is it safe to ignore the error code returned from kmb_initialize_clocks()? drivers/gpu/drm/kmb/kmb_drv.c:kmb_hw_init() { ... /* Enable display clocks */ kmb_initialize_clocks(kmb, &pdev->dev); /* Allocate LCD interrupt resources */ irq_lcd = platform_get_irq(pdev, 0); ... } If clock initialization fails, the driver silently continues execution and attempts to access hardware MMIO registers. Accessing unclocked MMIO registers usually triggers a synchronous external abort and hangs the system. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
