Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider: - [High] The HDMI interrupt handler checks a register offset macro instead of the read value, causing interrupts to be silently ignored. - [High] Deadlock between the PM runtime suspend callback and the threaded IRQ handler. - [High] Calling `drm_hdcp_update_content_protection()` without holding `connection_mutex` triggers a WARN_ON and creates a state race. - [High] The delayed work `hdcp_work` is not canceled during driver removal, leading to a Use-After-Free. - [High] Double detachment of the MIPI DSI device due to mixing manual and devm-managed teardown. - [High] Unclamped DDC FIFO read count in `it61620_hdmi_hdcp_ksvlist_rd` can cause a heap buffer overflow or an infinite loop. - [High] Incorrect SHA-1 hash length computation breaks HDCP authentication. - [High] edid_read accesses powered-off hardware, causing regmap_read failures and pervasive use of uninitialized stack variables. - [High] Premature destruction of `ddc_lock` in `it61620_remove()` before the bridge is unregistered. - [Medium] PM runtime and regulator state leaks in probe errors and remove path. - [Medium] Incorrect regulator disable order in it61620_poweroff(). - [Medium] `it61620_parse_dt()` is declared to return `unsigned int` but returns negative error codes. - [Medium] Unprotected DDC hardware access in `it61620_hdmi_ddc_abort()`. - [Medium] Uninitialized stack variables are written to hardware registers if unsupported audio parameters are supplied. - [Medium] Unsigned underflow in abs(htotal - 2880) causes incorrect delay calculation. --
--- Patch [2]: [PATCH v8 2/2] drm/bridge: Add ITE IT61620 MIPI DSI to HDMI bridge driver --- commit 0da87ff1083d89041b3e6585c68651e55d9e8de2 Author: Pet Weng <[email protected]> drm/bridge: Add ITE IT61620 MIPI DSI to HDMI bridge driver This adds support for the ITE IT61620 bridge chip which converts MIPI DSI input to HDMI output. The Driver implements the basic bridge functions and integrates with the DRM bridge and connector frameworks. > diff --git a/drivers/gpu/drm/bridge/ite-it61620.c > b/drivers/gpu/drm/bridge/ite-it61620.c > index 0000000000000..95771e5a39114 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/ite-it61620.c [ ... ] > @@ -353,10 +353,10 @@ static void it61620_mipi_set_d2v_video_timing(struct > it61620 *it61620, > if (it61620->dev_ver != DEV_VERSION_A0) > regmap_update_bits(it61620->mipirx_regmap, RX_REG_SELDCLK, > 0x1c, 0x04); > > if (hdew > 1920) { > if (htotal < 4272) { [Severity: Medium] Could htotal - 2880 underflow here? Since htotal is defined as a u32 and can be less than 2880, the result might wrap around to a very large positive integer. The abs() macro would return this unsigned value unchanged, resulting in a large value being divided by 24 and cast to u8, which could configure incorrect video timing delays. > d2vffrd_adr_dly = abs(htotal - 2880) / 24; > regmap_write(it61620->mipirx_regmap, RX_REG_DSC_VFRD, > d2vffrd_adr_dly); > } else { [ ... ] > @@ -482,10 +482,10 @@ static void it61620_hdmi_ddc_abort(struct it61620 > *it61620) > static unsigned int it61620_hdmi_read_ddc_status(struct it61620 *it61620) > { > unsigned int val; > > regmap_read(it61620->tx_regmap, TX_REG_DDC_STATUS, &val); [Severity: High] Is it safe to return val without checking if regmap_read() succeeded? If the device is powered off, regmap_read() will fail and leave val uninitialized. This uninitialized stack variable is then returned and used as a loop termination condition in it61620_hdmi_ddc_wait(), which could lead to unpredictable polling loop behavior. > return val; > } [ ... ] > @@ -628,10 +628,10 @@ static int it61620_hdmi_hdcp_ksvlist_rd(struct it61620 > *it61620, > if (it61620_hdmi_ddc_wait(it61620) < 0) { > it61620_hdmi_ddc_abort(it61620); > ret = -EIO; > dev_dbg(it61620->dev, "ddc fail"); > break; > } > > regmap_read(it61620->tx_regmap, TX_REG_DDC_FIFO_STS, &count); > count &= M_DDC_STAGE_NUM; [Severity: High] Can the hardware report a count that exceeds the remaining bytes requested? The loop blindly accepts the hardware count and writes it to the output buffer. If count is not clamped to (bytenum - i), it could overflow the sha1_input buffer. Additionally, if the hardware reports 0 bytes, would this cause an infinite loop? > it61620_hdmi_get_ddc_fifo(it61620, (out + i), count); > i += count; > } [ ... ] > @@ -668,10 +668,10 @@ static int it61620_hdmi_setup_sha1_input(struct it61620 > *it61620, u8 *input) > input[count++] = bstatus[0]; > input[count++] = bstatus[1]; > > regmap_update_bits(it61620->tx_regmap, TX_REG_SHA_SEL, 0x70, 0x70); > regmap_update_bits(it61620->tx_regmap, TX_REG_HDCP_CTRL2, B_EN_M0_RD, > B_EN_M0_RD); > regmap_bulk_read(it61620->tx_regmap, TX_REG_ANM0V1, &input[count], 8); > > regmap_update_bits(it61620->tx_regmap, TX_REG_HDCP_CTRL2, B_EN_M0_RD, > 0x00); [Severity: High] Should the count variable be incremented by 8 after reading the M0 value into the input buffer? Because count is returned without accounting for these 8 bytes, the caller might compute the SHA-1 hash with an incomplete length, which could cause HDCP repeater authentication to consistently fail. > return count; > } [ ... ] > @@ -754,10 +754,10 @@ static int it61620_hdmi_enable_hdcp(struct it61620 > *it61620) > if (auth_cs == CS_AUTH_DONE) { > it61620_hdmi_enable_avmute(it61620, false); > regmap_update_bits(it61620->tx_regmap, TX_REG_INT1, > B_INT_AUTH_F, > B_INT_AUTH_F); > regmap_update_bits(it61620->tx_regmap, TX_REG_INT_MASK1, > B_INT_AUTH_F, > B_INT_AUTH_F); > drm_dbg(drm, "auth done"); > it61620->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_ENABLED; [Severity: High] Could calling drm_hdcp_update_content_protection() in this context trigger a warning from the DRM core? The DRM core expects the mode config connection_mutex to be held when updating this state. Modifying connector state concurrently with atomic commits might create data races. > drm_hdcp_update_content_protection(it61620->connector, > it61620->hdcp_cp); > return true; > } > > drm_dbg(drm, "auth fail"); > it61620_hdmi_enable_avmute(it61620, true); [Severity: Medium] Is there a risk of concurrent hardware access here? While standard EDID reads hold the ddc_lock, this abort routine is called on HDCP error paths without acquiring the lock. If userspace concurrently attempts an EDID read, it might cause state corruption on the DDC bus. > it61620_hdmi_ddc_abort(it61620); > return false; > } [ ... ] > @@ -876,10 +876,10 @@ static void it61620_hdmi_irq_hdcp_auth_fail(struct > it61620 *it61620) > { > struct drm_device *drm = it61620->drm; > > if (it61620->hdcp_state == CP_GOING) { > it61620_hdmi_enable_avmute(it61620, true); > drm_dbg(drm, "auth fail after done, set avmute"); > it61620->hdcp_cp = DRM_MODE_CONTENT_PROTECTION_DESIRED; [Severity: High] Could calling drm_hdcp_update_content_protection() here from the threaded IRQ handler cause lockdep splats or state races? Similar to the previous usage, the connection_mutex is not held here. > drm_hdcp_update_content_protection(it61620->connector, > it61620->hdcp_cp); > it61620_start_hdcp_work(it61620); > } > } [ ... ] > @@ -930,10 +930,10 @@ static void it61620_hdmi_interrupt_handler(struct > it61620 *it61620) > regmap_read(it61620->tx_regmap, TX_REG_INT3, &int_status3); > > regmap_write(it61620->tx_regmap, TX_REG_INT1, int_status1); > regmap_write(it61620->tx_regmap, TX_REG_INT2, int_status2); > regmap_write(it61620->tx_regmap, TX_REG_INT3, int_status3); [Severity: High] Does this conditional evaluate the TX_REG_INT1 macro (which expands to the register address 0x10) instead of the int_status1 variable that was just read? Since TX_REG_INT1 & B_HPD_CHG always evaluates to 0, this would silently ignore Hot-Plug Detect and other interrupts. > if (TX_REG_INT1 & B_HPD_CHG) > it61620_hdmi_irq_hpd(it61620); > > if (TX_REG_INT1 & B_RXSEN_CHANGE) > it61620_hdmi_irq_rxsen_chg(it61620); > > if (TX_REG_INT1 & B_INT_AUTH_F) > it61620_hdmi_irq_hdcp_auth_fail(it61620); [ ... ] > @@ -1067,10 +1067,10 @@ static void it61620_audio_update_hw_params(struct > it61620 *it61620, > case HDMI_I2S: > case HDMI_SPDIF: > break; > default: > return; > } > > is_lpcm = !(hparms->iec.status[0] & IEC958_AES0_NONAUDIO); > it61620_hdmi_audio_set_ncts(it61620, hparms->sample_rate); > > drm_dbg(drm, "sample rate %d", sample_rate_val); > drm_dbg(drm, "sample width %d", sample_width); [Severity: Medium] Are sample_rate_val and sample_width guaranteed to be initialized? The switch statements lack default cases, so if the ALSA/DRM core passes an unsupported audio parameter, these variables remain uninitialized when they are written to the hardware registers below. > regmap_update_bits(it61620->tx_regmap, TX_REG_AUD_FMT, 0x7f, > (sample_width << 5) | it61620->i2s_input_format); [ ... ] > @@ -1220,10 +1220,10 @@ static unsigned int it61620_parse_dt(struct it61620 > *it61620) > { > struct device *dev = it61620->dev; > struct device_node *np = it61620->dev->of_node; > int num_lanes; > > if (!dev->of_node) [Severity: Medium] Should this function be declared to return an int? It is currently defined to return an unsigned int, but it directly returns negative error codes like -EINVAL and the result of dev_err_probe(). > return -EINVAL; > > num_lanes = drm_of_get_data_lanes_count_ep(np, 0, -1, 1, 4); > if (num_lanes < 0) > num_lanes = 4; > it61620->mipirx_config.lane_num = num_lanes; > > it61620->next_bridge = devm_drm_of_get_bridge(dev, np, 1, -1); > if (IS_ERR(it61620->next_bridge)) > return dev_err_probe(dev, PTR_ERR(it61620->next_bridge), > "failed to get next bridge\n"); > > return 0; > } [ ... ] > @@ -1310,18 +1310,18 @@ static int it61620_poweroff(struct it61620 *it61620) > return 0; > } > > if (it61620->it61620_i2c->irq) { [Severity: High] The suspend path calls it61620_poweroff() which synchronously blocks on disable_irq() waiting for the threaded handler to finish. However, the threaded handler calls pm_runtime_get_sync(dev), which blocks waiting for suspend to finish. Could this create an AB-BA deadlock where both paths wait on each other? > disable_irq(it61620->it61620_i2c->irq); > drm_dbg(drm, "disable irq %d\n", > it61620->it61620_i2c->irq); > } > > gpiod_set_value_cansleep(it61620->gpiod_reset, 1); [Severity: Medium] Are the regulators being disabled in the correct order? They are enabled in the order ivdd, ovdd1833, then ovdd33, but they are disabled here in the order ovdd33, ivdd, then ovdd1833. Disabling ivdd before ovdd1833 violates the reverse sequence and might lead to hardware malfunction. > err = regulator_disable(it61620->ovdd33); > if (err) > dev_err(dev, "Failed to disable ovdd33: %d", err); > > err = regulator_disable(it61620->ivdd); > if (err) > dev_err(dev, "Failed to disable IVDD: %d", err); > > usleep_range(2000, 3000); > > err = regulator_disable(it61620->ovdd1833); > if (err) > dev_err(dev, "Failed to disable ovdd1833: %d", err); [ ... ] > @@ -1486,10 +1486,10 @@ static const struct drm_edid > *it61620_bridge_edid_read(struct drm_bridge *bridge > struct drm_connector > *connector) > { > struct it61620 *it61620 = bridge_to_it61620(bridge); > struct device *dev = it61620->dev; > const struct drm_edid *edid; [Severity: High] Does this path ensure the device is powered on before accessing the hardware? Unprivileged userspace can trigger EDID reads, and if the bridge is currently runtime-suspended and not awakened via pm_runtime_get_sync(), the subsequent I2C operations would fail. > edid = drm_edid_read_custom(connector, it61620_get_edid_block, it61620); > > if (!edid) > dev_dbg(dev, "failed to get edid!"); > > return edid; > } [ ... ] > @@ -1560,10 +1560,10 @@ static irqreturn_t it61620_int_threaded_handler(int > unused, void *data) > { > struct it61620 *it61620 = data; > struct device *dev = it61620->dev; [Severity: High] Can this cause an AB-BA deadlock with the suspend path? The threaded handler calls pm_runtime_get_sync(dev) below, which blocks waiting for any ongoing suspend to finish, while suspend waits for this handler via disable_irq(). > pm_runtime_get_sync(dev); > > it61620_mipi_irq_handler(it61620); > it61620_hdmi_irq(it61620); [ ... ] > @@ -1763,16 +1763,16 @@ static void it61620_remove(struct i2c_client *client) > { > struct it61620 *it61620 = i2c_get_clientdata(client); > struct device *dev = it61620->dev; > > disable_irq(client->irq); [Severity: Medium] Does this function ensure the device is synchronously powered off before disabling PM runtime? If the device was active when unbound, calling pm_runtime_disable() without a prior pm_runtime_put_sync() could leak the regulator and PM states. > pm_runtime_disable(dev); [Severity: High] Is it necessary to call it61620_detach_dsi() here? The DSI device is attached using devm_mipi_dsi_attach() during probe, which means the devres framework will automatically detach it. Doing it manually here might cause a double detach. [Severity: High] Should cancel_delayed_work_sync(&it61620->hdcp_work) be called here? If an HDCP authentication work item is queued when the driver is unbound, the pending work might execute after the devres-managed structure is freed, leading to a use-after-free. > it61620_detach_dsi(it61620); [Severity: High] Is it safe to destroy the ddc_lock mutex before the bridge is unregistered? The bridge is registered via devm_drm_bridge_add(), so it remains active until the devres cleanup phase, which runs after remove() completes. If userspace initiates an EDID read during this window, it would attempt to lock a destroyed mutex. > mutex_destroy(&it61620->ddc_lock); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
