Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] `devm_drm_of_get_bridge()` in the hot-pluggable `lt7911exc_dsi_host_attach()` callback leaks devres memory and causes use-after-free upon panel unbind/rebind. - [High] Double removal of the DRM bridge due to a flawed `list_empty` check in `lt7911exc_remove()`, leading to double mutex destruction and use-after-free. - [High] Firmware upgrade clears the `upgrade` flag AFTER sending the hotplug event, causing potential modesets to fail silently. - [Medium] Direct mutation of `pre_enable_prev_first` on an external `drm_bridge` object owned by a downstream driver. --
--- Patch [2]: [PATCH v15 2/2] drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge --- commit 4c1fba2c711f35f533a59a3680b0b1fda9a82f94 Author: Sunyun Yang <[email protected]> drm/bridge: Add Lontium LT7911EXC eDP to MIPI DSI bridge Add support for the Lontium LT7911EXC bridge chip, which converts eDP input to MIPI DSI output using an internal firmware-controlled pipeline. > diff --git a/drivers/gpu/drm/bridge/lontium-lt7911exc.c > b/drivers/gpu/drm/bridge/lontium-lt7911exc.c > new file mode 100644 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/lontium-lt7911exc.c [ ... ] > +static void lt7911exc_firmware_upgrade_work(struct work_struct *work) > +{ [ ... ] > + if (ret) > + dev_err(dev, "failed to read version after upgrade\n"); > + > + /* Notify DRM to re-trigger modeset after firmware upgrade */ > + if (lt7911exc->bridge.dev) > + drm_kms_helper_hotplug_event(lt7911exc->bridge.dev); [Severity: High] Could this hotplug event fire before the upgrade flag is cleared? Since drm_kms_helper_hotplug_event() triggers modesets synchronously (or fast async), it might call lt7911exc_atomic_pre_enable() while lt7911exc->upgrade is still true. If that happens, pre_enable() will silently skip writing the hardware enable registers, resulting in a black screen. Would it be safer to clear the upgrade flag before firing the hotplug event? > + > + goto out_release_fw; > + > +out_mcu_run: > + scoped_guard(mutex, <7911exc->ocm_lock) > + lt7911exc_inside_mcu_run(lt7911exc); > + > +out_release_fw: > + release_firmware(fw); > + > +out_unlock_upgrade: > + scoped_guard(mutex, <7911exc->ocm_lock) { > + if (!lt7911exc->removed) > + lt7911exc->upgrade = false; > + } > + mutex_unlock(<7911exc->upgrade_lock); > +} [ ... ] > +static int lt7911exc_dsi_host_attach(struct mipi_dsi_host *host, struct > mipi_dsi_device *dev) > +{ > + struct lt7911exc *lt7911exc = dsi_host_to_lt7911exc(host); > + > + if (dev->lanes > 4) { > + dev_err(lt7911exc->dev, "unsupported number of data > lanes(%u)\n", dev->lanes); > + return -EINVAL; > + } > + > + lt7911exc->output_bridge = devm_drm_of_get_bridge(lt7911exc->dev, > host->dev->of_node, 1, 0); [Severity: High] Could this devm allocation leak devres memory and cause a use-after-free? The devm_drm_of_get_bridge() allocation is tied to the host device (lt7911exc->dev), but it runs in lt7911exc_dsi_host_attach() which can be called repeatedly if the downstream DSI panel is unbound and rebound. Because the allocation is tied to the host device, it won't be freed until the host driver unbinds. This leaks memory on every panel rebind. Also, the retained drm_panel_bridge will hold a dangling pointer after the panel is destroyed, risking a use-after-free if accessed later. > + if (IS_ERR(lt7911exc->output_bridge)) > + return PTR_ERR(lt7911exc->output_bridge); > + > + /* > + * LT7911EXC requires the panel to be powered and ready before > + * enabling MIPI DSI output. Keep default order (panel first) > + * regardless of panel's prepare_prev_first setting, since the > + * chip's internal firmware handles panel initialization. > + */ > + lt7911exc->output_bridge->pre_enable_prev_first = false; [Severity: Medium] Is it safe to directly mutate the state of an external bridge object? This output_bridge is owned and managed by the downstream driver. Setting its pre_enable_prev_first field from this host driver violates object ownership and could override necessary configuration expected by the downstream driver. > + > + drm_bridge_add(<7911exc->bridge); > + > + return 0; > +} [ ... ] > +static void lt7911exc_remove(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct lt7911exc *lt7911exc = dev_get_drvdata(dev); > + > + if (!lt7911exc) > + return; > + > + scoped_guard(mutex, <7911exc->ocm_lock) { > + lt7911exc->removed = true; > + lt7911exc->upgrade = true; > + } > + > + cancel_work_sync(<7911exc->work); > + > + dev_set_drvdata(dev, NULL); > + > + mipi_dsi_host_unregister(<7911exc->dsi_host); > + > + if (!list_empty(<7911exc->bridge.list)) > + drm_bridge_remove(<7911exc->bridge); [Severity: High] Will this result in a double free of the bridge? During module unload, mipi_dsi_host_unregister() detaches all DSI children, which calls lt7911exc_dsi_host_detach(). That explicitly calls drm_bridge_remove() and moves the list node to the global bridge_lingering_list. When execution reaches this list_empty() check, it will evaluate to false because the node is on the lingering list (and its next pointer isn't pointing to itself). This causes drm_bridge_remove() to be erroneously called a second time on the same bridge, leading to a double free and double mutex_destroy(). > + > + gpiod_set_value_cansleep(lt7911exc->reset_gpio, 1); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
