Re: [PATCH AUTOSEL 6.18-5.10] drm/gma500: return errors from Oaktrail HDMI I2C reads

[sashiko-bot@kernel.org]

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 <pengpeng@iscas.ac.cn>

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/20260625003240.6923-1-pengpeng@iscas.ac.cn

> 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/20260831133314.4125787-219-sashal@kernel.org?part=1