Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [High] Missing `disable_irq()` in `lt9611c_remove()` creates a race condition leading to Use-After-Free and unpowered hardware access. - [High] Stack buffer overflow in `lt9611c_hdmi_write_hdmi_infoframe` due to unchecked `len` parameter. - [High] Permanent module reference leak due to unbalanced `drm_bridge_get()` in `lt9611c_parse_dt()`. - [High] Self-deadlock in `lt9611c_hdmi_audio_prepare` due to double locking of `ocm_lock` via nested DRM helper callbacks. - [High] Erratic interrupt clearing sequence can permanently lock up the edge-triggered interrupt line and lose events. - [High] Ignored return value of `devm_drm_bridge_add()` can lead to successful probe with an unregistered bridge. - [High] Missing `cancel_work_sync()` in `lt9611c_bridge_suspend()` allows the HPD work to execute while the hardware is powered off. - [High] Race condition between sysfs file creation and initialization of `clientdata` causes a NULL pointer dereference. - [Medium] Synchronous firmware loading while holding a hardware mutex (`ocm_lock`). - [Medium] Ignored flash erase timeout allows firmware upgrade to silently corrupt device flash. --
--- Patch [2]: [PATCH v7 2/2] drm/bridge: Add Lontium LT9609C(EX/UXD) MIPI DSI to HDMI driver --- commit b64f10044aaf95610928583023bd2bc668abab4e Author: Sunyun Yang <[email protected]> Subject: drm/bridge: Add Lontium LT9609C(EX/UXD) MIPI DSI to HDMI driver This commit introduces the driver for the Lontium LT9611C DSI to HDMI bridge. It adds support for configuring video timing, reading EDID, handling HPD interrupts, and managing firmware upgrades. > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611c.c > b/drivers/gpu/drm/bridge/lontium-lt9611c.c > new file mode 100644 > index 0000000000000..e8bb385d15206 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/lontium-lt9611c.c > @@ -0,0 +1,1293 @@ [ ... ] > +static void lt9611c_block_erase(struct lt9611c *lt9611c) > +{ > + struct device *dev = lt9611c->dev; > + int i; > + unsigned int block_num; > + unsigned int flash_status = 0; > + u32 flash_addr = 0; > + > + for (block_num = 0; block_num < 2; block_num++) { > + flash_addr = (block_num * 0x008000); > + lt9611c_erase_op(lt9611c, flash_addr); > + msleep(100); > + i = 0; > + while (1) { > + read_flash_reg_status(lt9611c, &flash_status); > + if ((flash_status & 0x01) == 0) > + break; > + > + if (i > 50) > + break; [Severity: Medium] Does this ignore the flash erase timeout? If the polling loop in lt9611c_block_erase() times out, it breaks out silently. Since this function has no return value, lt9611c_firmware_upgrade() will proceed to write the new firmware over an unerased block, which could corrupt the device firmware. > + > + i++; > + msleep(50); > + } > + } > + > + dev_dbg(dev, "erase flash done.\n"); > +} [ ... ] > +static irqreturn_t lt9611c_irq_thread_handler(int irq, void *dev_id) > +{ > + struct lt9611c *lt9611c = dev_id; > + struct device *dev = lt9611c->dev; > + int ret; > + unsigned int irq_status; > + u8 cmd[5] = {0x52, 0x48, 0x31, 0x3a, 0x00}; > + u8 data[5]; > + > + guard(mutex)(<9611c->ocm_lock); > + > + ret = regmap_read(lt9611c->regmap, 0xe084, &irq_status); > + if (ret) { > + dev_err(dev, "failed to read irq status: %d\n", ret); > + return IRQ_HANDLED; > + } > + > + if (!(irq_status & BIT(0))) > + return IRQ_HANDLED; [Severity: High] Could this early return permanently lock up the edge-triggered interrupt line? If another status bit triggered the interrupt, this bails out without writing to the clear register (0xe0df). Because the interrupt line is configured as IRQF_TRIGGER_FALLING, leaving it asserted prevents any future falling edges and permanently locks up all future hardware interrupts. > + > + msleep(100); > + > + ret = lt9611c_read_write_flow(lt9611c, cmd, ARRAY_SIZE(cmd), data, > ARRAY_SIZE(data)); [ ... ] > +static int lt9611c_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge, > + const u8 *buffer, size_t len) > +{ > + struct lt9611c *lt9611c = bridge_to_lt9611c(bridge); > + u8 cmd[5 + LT9611C_INFOFRAME_MAX_SIZE]; > + u8 data[5]; > + int ret; > + > + cmd[0] = 0x57; > + cmd[1] = 0x48; > + cmd[2] = 0x35; > + cmd[3] = 0x3a; > + cmd[4] = 0x04;/*write vsif*/ > + memcpy(cmd + 5, buffer, len); [Severity: High] Is it possible to overflow the cmd stack buffer here? The cmd array is allocated with a fixed size of 37 bytes (5 + LT9611C_INFOFRAME_MAX_SIZE). Because the len parameter is passed directly to memcpy without validation, passing an infoframe larger than 32 bytes would corrupt the stack. > + > + guard(mutex)(<9611c->ocm_lock); > + > + ret = lt9611c_read_write_flow(lt9611c, cmd, 5 + len, > + data, ARRAY_SIZE(data)); [ ... ] > +static int lt9611c_hdmi_write_audio_infoframe(struct drm_bridge *bridge, > + const u8 *buffer, size_t len) > +{ > + struct lt9611c *lt9611c = bridge_to_lt9611c(bridge); > + u8 *cmd; > + u8 data[5]; > + int ret; > + > + guard(mutex)(<9611c->ocm_lock); [ ... ] > +static int lt9611c_hdmi_audio_prepare(struct drm_bridge *bridge, > + struct drm_connector *connector, > + struct hdmi_codec_daifmt *fmt, > + struct hdmi_codec_params *hparms) > +{ > + struct lt9611c *lt9611c = bridge_to_lt9611c(bridge); > + u8 audio_cmd[6] = {0x57, 0x48, 0x36, 0x3a}; > + u8 data[5]; > + int ret; > + > + if (hparms->sample_width == 32) > + return -EINVAL; > + > + switch (fmt->fmt) { > + case HDMI_I2S: > + audio_cmd[4] = 0x01; > + break; > + case HDMI_SPDIF: > + audio_cmd[4] = 0x02; > + break; > + default: > + return -EINVAL; > + } > + > + audio_cmd[5] = hparms->channels; > + guard(mutex)(<9611c->ocm_lock); > + > + ret = lt9611c_read_write_flow(lt9611c, audio_cmd, sizeof(audio_cmd), > + data, sizeof(data)); > + if (ret < 0) { > + dev_err(lt9611c->dev, "set audio info failed!\n"); > + return ret; > + } > + > + return > drm_atomic_helper_connector_hdmi_update_audio_infoframe(connector, > + > &hparms->cea); [Severity: High] Will this cause a self-deadlock? The ocm_lock is acquired here in lt9611c_hdmi_audio_prepare(). We then call drm_atomic_helper_connector_hdmi_update_audio_infoframe(), which internally invokes the connector's write_infoframe hook mapped to lt9611c_hdmi_write_audio_infoframe(). That inner function attempts to acquire ocm_lock again. Since Linux mutexes are not recursive, this guarantees a deadlock. > +} [ ... ] > +static int lt9611c_parse_dt(struct device *dev, > + struct lt9611c *lt9611c) > +{ > + int ret; > + > + lt9611c->dsi0_node = of_graph_get_remote_node(dev->of_node, 0, -1); > + if (!lt9611c->dsi0_node) > + return dev_err_probe(dev, -ENODEV, "failed to get remote node > for primary dsi\n"); > + > + lt9611c->dsi1_node = of_graph_get_remote_node(dev->of_node, 1, -1); > + > + ret = drm_of_find_panel_or_bridge(dev->of_node, 2, -1, NULL, > <9611c->bridge.next_bridge); > + if (ret) { > + of_node_put(lt9611c->dsi1_node); > + of_node_put(lt9611c->dsi0_node); > + return ret; > + } > + drm_bridge_get(lt9611c->bridge.next_bridge); [Severity: High] Does this code leak a module reference to the next bridge? The reference acquired by drm_bridge_get() here is never released. The driver's lt9611c_remove() function does not call drm_bridge_put() to balance this, meaning every unbind/rebind cycle leaks a reference. > + return 0; > +} [ ... ] > +static ssize_t lt9611c_firmware_store(struct device *dev, struct > device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > + int ret; > + > + lt9611c_lock(lt9611c); > + > + ret = lt9611c_firmware_upgrade(lt9611c); [Severity: Medium] Is it safe to hold ocm_lock during a synchronous firmware load? lt9611c_firmware_upgrade() calls request_firmware(), which can block for up to 60 seconds on VFS and user-mode helpers. While blocked, the IRQ threaded handler and atomic commit callbacks will be starved as they contend for the same mutex. > + if (ret < 0) > + dev_err(dev, "upgrade failure\n"); > + > + lt9611c_unlock(lt9611c); > + > + return ret < 0 ? ret : len; > +} > + > +static ssize_t lt9611c_firmware_show(struct device *dev, struct > device_attribute *attr, char *buf) > +{ > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "0x%04x\n", lt9611c->fw_version); [Severity: High] Can dev_get_drvdata(dev) return NULL here? Sysfs attributes are added via the dev_groups member of the i2c_driver struct, meaning the driver core creates them before the probe function finishes. In lt9611c_probe(), i2c_set_clientdata() is called very late. If userspace reads this attribute concurrently before probe completes, this will dereference a NULL pointer. > +} [ ... ] > +static int lt9611c_probe(struct i2c_client *client) > +{ [ ... ] > + lt9611c->bridge.hdmi_audio_max_i2s_playback_channels = 8; > + lt9611c->bridge.hdmi_audio_dai_port = 2; > + > + devm_drm_bridge_add(dev, <9611c->bridge); [Severity: High] Shouldn't we check the return value of devm_drm_bridge_add()? If devres allocation fails, the bridge won't be added to the DRM bridge chain, but probe will continue and return success. This leaves the subsystem in an inconsistent state. > + > + /* Attach primary DSI */ > + lt9611c->dsi0 = lt9611c_attach_dsi(lt9611c, lt9611c->dsi0_node); > + if (IS_ERR(lt9611c->dsi0)) { [ ... ] > +static void lt9611c_remove(struct i2c_client *client) > +{ > + struct lt9611c *lt9611c = i2c_get_clientdata(client); > + > + cancel_work_sync(<9611c->work); > + regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), > lt9611c->supplies); > + of_node_put(lt9611c->dsi1_node); > + of_node_put(lt9611c->dsi0_node); > +} [Severity: High] Does this leave a race condition leading to a use-after-free? The IRQ was registered via devm_request_threaded_irq(). Because there is no explicit disable_irq() call here, the interrupt line remains active until devres cleanup runs after remove() returns. If an interrupt fires after cancel_work_sync(), the threaded handler will run, re-queue the work item, and attempt I2C access while the device is unpowered. > + > +static int lt9611c_bridge_suspend(struct device *dev) > +{ > + struct lt9611c *lt9611c = dev_get_drvdata(dev); > + int ret; > + > + dev_dbg(lt9611c->dev, "suspend\n"); > + disable_irq(lt9611c->client->irq); > + > + gpiod_set_value_cansleep(lt9611c->reset_gpio, 1); > + > + ret = regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), > lt9611c->supplies); [Severity: High] Should this flush the workqueue before disabling regulators? If an interrupt fired and scheduled lt9611c->work right before disable_irq() was called, the work item stays in the queue. When it executes, it will trigger an I2C read in lt9611c_bridge_detect() while the device regulators are disabled, leading to timeouts or a kernel panic. > + if (ret) > + dev_err(lt9611c->dev, "regulator bulk disable failed.\n"); > + > + return ret; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
