Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Returning early from `xfer_read()` on timeout or signal abandons the active hardware I2C transaction and leaves `i2c_dev->msg` as a dangling pointer, leading to a Use-After-Free and memory corruption. Pre-existing issues: - [High] `hdmi_i2c_read()` unconditionally writes 64 bytes to `msg->buf` without checking the requested transfer length (`msg->len`), allowing hardware to overflow the I2C message buffer. - [High] In the module teardown path, `i2c_dev` is freed before the shared IRQ handler is unregistered, causing a Use-After-Free if an interrupt fires. -- commit 605dc1b5dc9a1628f90b0022526c86a3c3636916 Author: Pengpeng Hou <[email protected]> drm/gma500: return errors from Oaktrail HDMI I2C reads This commit modifies xfer_read() to return -ETIMEDOUT or negative error codes on timeout or signal instead of ignoring them. It also updates oaktrail_hdmi_i2c_access() to propagate the first transfer error instead of reporting a successful message count. Link: https://patch.msgid.link/[email protected] > diff --git a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c > b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c > --- a/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c > +++ b/drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c > @@ -106,9 +107,14 @@ static int xfer_read(struct i2c_adapter *adap, struct > i2c_msg *pmsg) > HDMI_WRITE(HDMI_HI2CHCR, temp); > HDMI_READ(HDMI_HI2CHCR); > > - while (i2c_dev->status != I2C_TRANSACTION_DONE) > - wait_for_completion_interruptible_timeout(&i2c_dev->complete, > + while (i2c_dev->status != I2C_TRANSACTION_DONE) { > + ret = > wait_for_completion_interruptible_timeout(&i2c_dev->complete, > 10 * HZ); > + if (ret < 0) > + return ret; > + if (!ret) > + return -ETIMEDOUT; > + } [Severity: High] Does returning early here abandon the active hardware I2C transaction and leave i2c_dev->msg as a dangling pointer? If wait_for_completion_interruptible_timeout() returns early due to a timeout or signal, xfer_read() returns the error without clearing HI2C_ENABLE_TRANSACTION on the hardware or setting i2c_dev->msg to NULL. oaktrail_hdmi_i2c_access() disables the device IRQ and returns the error, freeing the user buffer. If the hardware finishes the transaction in the background, a subsequent I2C transfer calls hdmi_i2c_irq_enable(), unmasking the interrupt before xfer_read() updates i2c_dev->msg. The pending interrupt fires immediately, and hdmi_i2c_read() dereferences the stale i2c_dev->msg pointer, overwriting freed memory. [ ... ] While reviewing this code, I noticed a couple of pre-existing issues in drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c: [Severity: High] This is a pre-existing issue, but does hdmi_i2c_read() unconditionally write 64 bytes to msg->buf without checking the requested transfer length? drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c:hdmi_i2c_read() { ... for (i = 0; i < 0x10; i++) { temp = HDMI_READ(HDMI_HI2CRDB0 + (i * 4)); memcpy(buf + (offset + i * 4), &temp, 4); } ... } If a user issues an I2C_RDWR ioctl with a read message where msg->len is less than 64 (e.g., 1 byte), the I2C core allocates a smaller buffer for msg->buf. When the hardware fires the FULL interrupt, hdmi_i2c_read() reads 4 bytes per iteration and copies 64 bytes to msg->buf, which could overflow the allocated buffer bounds and corrupt adjacent heap memory. [Severity: High] This is a pre-existing issue, but is there a use-after-free race condition in the module teardown path? drivers/gpu/drm/gma500/oaktrail_hdmi_i2c.c:oaktrail_hdmi_i2c_exit() { ... i2c_dev = hdmi_dev->i2c_dev; kfree(i2c_dev); free_irq(dev->irq, hdmi_dev); ... } The teardown logic frees i2c_dev before unregistering the shared interrupt handler. Since the IRQ is registered with IRQF_SHARED, if another device sharing the IRQ line fires an interrupt before free_irq() runs, oaktrail_hdmi_i2c_handler() executes and dereferences i2c_dev->complete on the freed structure. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
