Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/panel: double free/UAF of secondary DSI device on host unbind - [Low] drm/panel: NULL pointer dereference in nt36850_disable() for single-DSI panels - [Low] drm/panel: Unused backlight field in struct panel_info --
commit c61a43c5fad101fbd326985c7fa4cea8c858d4bd Author: Dmitry Baryshkov <[email protected]> drm/panel: add Novatek NT36850 panel driver Add support for the Truly WQHD dual-DSI command mode panel built around the Novatek NT36850, found on the display card for the Qualcomm SDM845 and SM8150 HDK. The driver is based on panel-novatek-nt36523, the power and command sequences follow the vendor device tree. > diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36850.c > b/drivers/gpu/drm/panel/panel-novatek-nt36850.c > new file mode 100644 > index 0000000000000..d3530e3d5754b > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-novatek-nt36850.c [ ... ] > +struct panel_info { > + struct drm_panel panel; > + struct mipi_dsi_device *dsi[2]; > + const struct panel_desc *desc; > + enum drm_panel_orientation orientation; > + > + struct gpio_desc *reset_gpio; > + struct backlight_device *backlight; [Severity: Low] Is this backlight pointer used anywhere else in the driver? It appears to be declared here but never initialized or accessed since the backlight is handled entirely by the drm_panel framework via drm_panel_of_backlight(). > + struct regulator *vddio; > + struct regulator *avdd; > + struct regulator *avee; > +}; [ ... ] > +static int nt36850_disable(struct drm_panel *panel) > +{ > + struct panel_info *pinfo = to_panel_info(panel); > + struct mipi_dsi_device *dsi0 = pinfo->dsi[0]; > + struct mipi_dsi_device *dsi1 = pinfo->dsi[1]; > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = NULL }; > + > + dsi0->mode_flags &= ~MIPI_DSI_MODE_LPM; > + dsi1->mode_flags &= ~MIPI_DSI_MODE_LPM; [Severity: Low] Could this cause a NULL pointer dereference if a single-DSI panel descriptor is ever added? The driver's probe function conditionally initializes dsi1 only when is_dual_dsi is true, but nt36850_disable() unconditionally fetches and dereferences it here, and then passes it to mipi_dsi_dual(). > + > + mipi_dsi_dual(mipi_dsi_dcs_set_display_off_multi, &dsi_ctx, dsi0, dsi1); > + mipi_dsi_msleep(&dsi_ctx, 120); > + mipi_dsi_dual(mipi_dsi_dcs_enter_sleep_mode_multi, &dsi_ctx, dsi0, > dsi1); [ ... ] > +static int nt36850_probe(struct mipi_dsi_device *dsi) > +{ [ ... ] > + /* If the panel is dual dsi, register DSI1 */ > + if (pinfo->desc->is_dual_dsi) { > + struct mipi_dsi_device_info dsi_info = { > + .type = "dsi-secondary", > + }; > + > + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1); > + if (!dsi1) { > + dev_err(dev, "cannot get secondary DSI node.\n"); > + return -ENODEV; > + } > + > + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1); > + of_node_put(dsi1); > + if (!dsi1_host) > + return dev_err_probe(dev, -EPROBE_DEFER, "cannot get > secondary DSI host\n"); > + > + pinfo->dsi[1] = devm_mipi_dsi_device_register_full(dev, > dsi1_host, &dsi_info); [Severity: High] Will this cause a double free or use-after-free during device unbind? The devres cleanup action for the secondary DSI device is tied to the primary DSI device (dev). However, the secondary DSI device is instantiated as a child of dsi1_host->dev. If the secondary DSI host is unbound via sysfs or module removal, mipi_dsi_host_unregister() will iterate over all its children and unconditionally call mipi_dsi_device_unregister(), freeing the secondary DSI device. Later, when the primary DSI device is unbound, its devres cleanup executes and calls mipi_dsi_device_unregister() on the already-freed pointer. Should this lifecycle be tied directly to the secondary host's unbind instead? > + if (IS_ERR(pinfo->dsi[1])) { > + dev_err(dev, "cannot get secondary DSI device\n"); > + return PTR_ERR(pinfo->dsi[1]); > + } > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
