Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues: - [High] The migration to the I2C_XFER_V2 API silently removed the hardcoded 3-attempt retry loop for transfers, breaking the driver's resilience against transient bus errors. - [High] Removing `.xfer` from `i2c_algorithm` breaks downstream subsystems (like I2C multiplexers) that strictly check `.master_xfer`. - [High] Heap out-of-bounds write in `i2cdev_ioctl_rdwr_v2` when updating the length of a partially transferred message. - [High] Unconditional memory leak of I2C message buffers upon transfer failure in `i2cdev_ioctl_rdwr_v2`. - [Medium] Unprotected lockless read of `priv->cli.xfer_bytes` in process context races with the hardirq handler. - [Medium] Interrupt handler resets controller state before process context can read it, defeating the new byte-level reporting. Pre-existing issues: - [High] Missing return value check for pm_runtime_get_sync(), which can lead to a kernel crash if resume fails. - [High] Missing synchronize_irq() in the timeout path leads to a Use-After-Free of message buffers by the interrupt handler. -- commit 6e00e021c9abe420f0965de298c6d92c4d01241e Author: Dmitry Guzman <[email protected]> i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting I2C_XFER_V2 is a new API that allows I2C clients to get the detailed report in case of transmission failure. Previously, the only information returned by I2C bus controller was the error code, without exposing how many messages or bytes had been sent. This commit introduces support of this feature in the i2c-nomadik driver. > diff --git a/drivers/i2c/busses/i2c-nomadik.c > b/drivers/i2c/busses/i2c-nomadik.c > index c36e9961e0a39..8b0ab191f7c1f 100644 > --- a/drivers/i2c/busses/i2c-nomadik.c > +++ b/drivers/i2c/busses/i2c-nomadik.c [ ... ] > @@ -678,6 +683,16 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, > u16 flags) > status = priv->result; > } > > + if (flags & I2C_M_RD) { > + /* For READ messages, return the number of bytes read > from FIFO */ > + priv->bytes_cplt = priv->cli.xfer_bytes; [Severity: Medium] Is it safe to perform an unprotected lockless read of priv->cli.xfer_bytes here? If the interrupt handler updates priv->cli.xfer_bytes concurrently on an error or timeout path, could this regression cause the process context to read a stale or inconsistent value? > + } else { > + /* For WRITE messages, return the number of bytes sent > on bus */ > + priv->bytes_cplt = FIELD_GET(I2C_SR_LENGTH, i2c_sr); [Severity: Medium] When an I2C abort error occurs, the interrupt handler calls init_hw(), which resets the I2C_SR register before process context can read it. Does this race condition cause the newly introduced byte-level error reporting to silently fail by returning 0 for bytes_cplt and missing the abort cause printout? > + /* LENGTH value includes the last byte that has not > been sent or ACKed */ > + if (priv->bytes_cplt > 0) > + priv->bytes_cplt--; > + } > init_hw(priv); [Severity: High] This is a pre-existing issue and was not introduced by the patch under review, but missing synchronize_irq() in the timeout path leads to a use-after-free. If a timeout occurs during an I2C transfer, this code path frees the message buffer, but the interrupt handler could fire concurrently on another CPU: i2c_irq_handler() *priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR); Since init_hw() calls disable_all_interrupts() which only masks the IP block but does not wait for running handlers, can this cause memory corruption by writing to the concurrently freed buffer? > > status = status ? status : priv->result; [ ... ] > @@ -733,14 +749,16 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, > u16 flags) > * please use the i2c_smbus_read_i2c_block_data() > * or i2c_smbus_write_i2c_block_data() API > */ > -static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, > - struct i2c_msg msgs[], int num_msgs) > +static int nmk_i2c_xfer_v2(struct i2c_adapter *i2c_adap, > + struct i2c_msg msgs[], int num_msgs, > + struct i2c_transfer_report *report) > { [Severity: High] The migration to the I2C_XFER_V2 API silently removed the hardcoded 3-attempt retry loop for transfers. Does this regression break the driver's resilience against transient bus errors like lost arbitration or NACKs, causing failed I2C transfers for device drivers? > int status = 0; > int i; > struct nmk_i2c_dev *priv = i2c_get_adapdata(i2c_adap); > > pm_runtime_get_sync(&priv->adev->dev); [Severity: High] This is a pre-existing issue and was not introduced by the patch under review, but the return value of pm_runtime_get_sync() is unchecked here. If the power domain fails to resume and returns an error, execution continues to setup_i2c_controller(), which could lead to a kernel panic from accessing unclocked or unpowered registers. Should we check the return value before proceeding? > + priv->bytes_cplt = 0; > > /* setup the i2c controller */ [ ... ] > @@ -1014,7 +1039,7 @@ static unsigned int nmk_i2c_functionality(struct > i2c_adapter *adap) > } > > static const struct i2c_algorithm nmk_i2c_algo = { > - .xfer = nmk_i2c_xfer, > + .xfer_v2 = nmk_i2c_xfer_v2, [Severity: High] Does removing .xfer from i2c_algorithm break downstream subsystems like I2C multiplexers? When a multiplexer attached to this bus attempts to register its transfer methods, it strictly checks parent->algo->master_xfer. Since .xfer is removed, the .master_xfer field (which is aliased via a union) evaluates to NULL, causing multiplexed I2C transfers to consistently fail with -EOPNOTSUPP. > .functionality = nmk_i2c_functionality > }; Looking at i2cdev_ioctl_rdwr_v2() in drivers/i2c/i2c-dev.c introduced earlier in this series: drivers/i2c/i2c-dev.c:i2cdev_ioctl_rdwr_v2() { ... if (user_report) { res = i2c_transfer_v2(client->adapter, msgs, nmsgs, &report); i = report.msgs_cplt; } else { res = i2c_transfer(client->adapter, msgs, nmsgs); if (res < 0) i = 0; else i = nmsgs; } ... while (i-- > 0) { if (msgs[i].flags & I2C_M_RD) { if (copy_to_user(data_ptrs[i], msgs[i].buf, msgs[i].len)) res = -EFAULT; } kfree(msgs[i].buf); } ... } [Severity: High] Does this regression leak the un-transferred message buffers? When an I2C transfer involving multiple messages fails partway through, the loop index i is reassigned to the number of completed messages. This truncates the cleanup loop, skipping the kfree(msgs[i].buf) for all subsequent allocated message buffers. Further down in i2cdev_ioctl_rdwr_v2(): drivers/i2c/i2c-dev.c:i2cdev_ioctl_rdwr_v2() { ... /* Number of messages transferred completely or partially */ if (report.bytes_cplt > 0) { i++; msgs[i].len = report.bytes_cplt; } ... } [Severity: High] Can this regression corrupt memory with an out-of-bounds write? The code incorrectly increments i before writing report.bytes_cplt to msgs[i].len. If nmsgs == 1 and the only message faults, this writes entirely outside the msgs array, or otherwise corrupts the length of the next message. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
