Re: [PATCH v30 4/5] i2c: ast2600: Add controller driver for AST2600 new register set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing lifecycle validation of `i2c_bus->msgs` on transfer timeout leads to a Use-After-Free (UAF) and memory corruption.
- [Critical] The RX interrupt handler blindly trusts the hardware's RX buffer length, leading to a potential out-of-bounds memory write (buffer overflow).
- [High] Non-atomic read-modify-write of a shared global syscon register clobbers existing bits and introduces a TOCTOU race.
- [High] SMBus Block Read with a block length of 0 corrupts the length byte in the receive buffer.
- [High] Unaborted bus recovery operations on timeout lead to cross-transfer completion races.
--

commit 00d3297d874ae70d7ea1c3a5e963a09c214da736
Author: Ryan Chen <ryan_chen@aspeedtech.com>
Subject: i2c: ast2600: Add controller driver for AST2600 new register set

The AST2600 introduces a new I2C controller register layout, selectable at
runtime via global control registers. This patch adds an AST2600-specific I2C
controller driver implementing the new register layout, including support for
packet-based transfers.

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> new file mode 100644
> index 0000000000000..a06d0483865ed
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ast2600.c

[ ... ]

> +static int ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
> +{

[ ... ]

> +	/* Check SDA/SCL status in the status register. */
> +	state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> +	if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) {
> +		writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> +		r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
> +		if (r == 0) {
> +			dev_dbg(i2c_bus->dev, "recovery timed out\n");
> +			return -ETIMEDOUT;

[Severity: High]
Does returning immediately on timeout leave the hardware in a state that could
corrupt future transfers?

If the delayed hardware recovery operation from the previous attempt finishes
later, it will trigger the BUS_RECOVER interrupt. This could execute
complete(&i2c_bus->cmd_complete), prematurely completing the wait for a new,
unrelated transfer and tricking the state machine into treating an incomplete
transfer as successful.

> +		} else if (i2c_bus->cmd_err) {
> +			dev_dbg(i2c_bus->dev, "recovery error\n");
> +			ret = -EPROTO;
> +		}
> +	}

[ ... ]

> +static void ast2600_i2c_controller_packet_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
> +{

[ ... ]

> +	case AST2600_I2CM_RX_DONE:
> +	case AST2600_I2CM_RX_DONE | AST2600_I2CM_NORMAL_STOP:
> +		xfer_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +							     AST2600_I2CC_BUFF_CTRL));
> +		for (i = 0; i < xfer_len; i++)
> +			msg->buf[i2c_bus->controller_xfer_cnt + i] =
> +				readb(i2c_bus->buf_base + i2c_bus->buf_size + i);

[Severity: Critical]
Is there a missing bounds check on xfer_len here?

If the hardware malfunctions, misinterprets the bus state, or returns a larger
length than expected, this loop could write data past the end of the
dynamically allocated msg->buf. Could this cause a kernel buffer overflow?

> +
> +		if (msg->flags & I2C_M_RECV_LEN) {
> +			u8 recv_len = AST2600_I2CC_GET_RX_BUFF(readl(i2c_bus->reg_base
> +						       + AST2600_I2CC_STS_AND_BUFF));
> +			msg->len = min_t(unsigned int, recv_len, I2C_SMBUS_BLOCK_MAX);
> +			msg->len += ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
> +			msg->flags &= ~I2C_M_RECV_LEN;
> +			if (!recv_len)
> +				i2c_bus->controller_xfer_cnt = 0;

[Severity: High]
Could setting controller_xfer_cnt to 0 corrupt the block length byte?

If recv_len is 0, msg->len is updated to 1 to perform a 1-byte read.
When the hardware receives this next byte and triggers RX_DONE again, the loop
above copies the new byte into msg->buf[i2c_bus->controller_xfer_cnt + i].

Since controller_xfer_cnt was reset to 0, does this unconditionally overwrite
and corrupt the block length byte in msg->buf[0]?

> +			else
> +				i2c_bus->controller_xfer_cnt = 1;
> +		} else {

[ ... ]

> +static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{

[ ... ]

> +	i2c_bus->cmd_err = 0;
> +	i2c_bus->msgs = msgs;
> +	i2c_bus->msgs_index = 0;
> +	i2c_bus->msgs_count = num;
> +	reinit_completion(&i2c_bus->cmd_complete);
> +	ret = ast2600_i2c_do_start(i2c_bus);
> +	if (ret)
> +		goto controller_out;
> +	timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
> +	if (timeout == 0) {

[Severity: Critical]
Does this timeout path need to clear i2c_bus->msgs?

If wait_for_completion_timeout() times out, the function returns -ETIMEDOUT
and the I2C subsystem will free the msgs array. However, the driver never
clears the i2c_bus->msgs pointer.

If a delayed hardware response (such as a spurious PKT_DONE or RX_DONE)
later triggers an interrupt, the ast2600_i2c_controller_packet_irq()
handler will unconditionally dereference the dangling i2c_bus->msgs
pointer. Could this lead to a use-after-free or out-of-bounds memory
write causing kernel memory corruption?

> +		u32 ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);

[ ... ]

> +static int ast2600_i2c_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	i2c_bus->global_regs =
> +		syscon_regmap_lookup_by_phandle(dev_of_node(dev), "aspeed,global-regs");
> +	if (IS_ERR(i2c_bus->global_regs))
> +		return PTR_ERR(i2c_bus->global_regs);
> +
> +	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL, &global_ctrl);
> +	if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) {
> +		regmap_write(i2c_bus->global_regs, AST2600_I2CG_CTRL, AST2600_GLOBAL_INIT);

[Severity: High]
Does this non-atomic write clobber existing bits in the shared control register?

Because AST2600_I2CG_CTRL is shared across multiple I2C controllers,
using regmap_write() to unconditionally overwrite the register with
AST2600_GLOBAL_INIT destroys any other configuration bits that may have
been set by the bootloader or other subsystems.

Also, concurrent probing of multiple buses could introduce a Time-Of-Check-
To-Time-Of-Use race. Should this use regmap_update_bits() instead to
atomically modify only the relevant bits?

> +		regmap_write(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-upstream_i2c-v30-0-5d4f9adc3530@aspeedtech.com?part=4