Re: [PATCH v31 5/5] i2c: ast2600: Add target mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Target RX data is discarded when a target interrupt aborts a concurrent controller transfer due to premature zeroing of `AST2600_I2CC_BUFF_CTRL`.
- [High] Incorrect I2C slave event ordering and generation of spurious events.
- [High] Missing clamp on target RX buffer length causes out-of-bounds MMIO reads.
- [High] `target_active` is incorrectly set to `false` when a STOP condition and a new transaction start are coalesced, leading to shared buffer corruption.
- [High] Failing to explicitly W1C-clear error status bits permanently deadlocks the controller.
- [High] Use-After-Free (UAF) of i2c_bus->msgs due to premature re-enabling of master interrupts.
--

commit d29dcb5ac45f9953b386f06cfe1a4604787fdc18
Author: Ryan Chen <ryan_chen@aspeedtech.com>

i2c: ast2600: Add target mode support

This commit adds target mode support to the AST2600 I2C driver.
It implements target interrupt handling and address match and
response logic to enable dual-role capability.

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> --- a/drivers/i2c/busses/i2c-ast2600.c
> +++ b/drivers/i2c/busses/i2c-ast2600.c
[ ... ]
> @@ -346,6 +351,253 @@ static int ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
>  	return ret;
>  }
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void ast2600_i2c_target_packet_buff_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
> +{
> +	u8 value = 0;
> +	int target_rx_len = 0;
> +	u32 cmd = 0;
> +	int i;
> +
> +	/* due to controller target is common buffer, need force the master stop not issue */
> +	if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & GENMASK(15, 0)) {
> +		writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> +		writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> +		writel(readl(i2c_bus->reg_base + AST2600_I2CM_ISR),
> +		       i2c_bus->reg_base + AST2600_I2CM_ISR);
> +		i2c_bus->cmd_err = -EBUSY;
> +		writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);

[Severity: High]
Does zeroing AST2600_I2CC_BUFF_CTRL here discard the target's received RX
buffer length? 

When the handler subsequently reads this register to determine the target RX
length, it will get 0, potentially dropping any received target data.

> +		complete(&i2c_bus->cmd_complete);
> +		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +		       i2c_bus->reg_base + AST2600_I2CM_IER);

[Severity: High]
Could this cause a use-after-free of i2c_bus->msgs? 

Waking the master thread with complete() before re-enabling master interrupts
allows the thread to resume and clear msgs to NULL. If a hardware master
interrupt fires concurrently, the racing IRQ handler might access
i2c_bus->msgs before it is NULLed out.

> +	}
> +
> +	/* Handle i2c target timeout condition */
> +	if (sts & AST2600_I2CS_INACTIVE_TO) {
> +		/* Reset timeout counter */
> +		u32 ac_timing = readl(i2c_bus->reg_base + AST2600_I2CC_AC_TIMING) &
> +				AST2600_I2CC_AC_TIMING_MASK;
> +
> +		writel(ac_timing, i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
> +		ac_timing |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout);
> +		writel(ac_timing, i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
> +		writel(TARGET_TRIGGER_CMD, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);

[Severity: High]
Is it possible for the controller to permanently deadlock here?

The AST2600_I2CS_ISR register uses write-1-to-clear bits, but only
AST2600_I2CS_PKT_DONE is written. The AST2600_I2CS_INACTIVE_TO error bit
remains set, which might cause subsequent controller transfers to
unconditionally evaluate readl(I2CS_ISR) as true and return -EBUSY.

> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> +		WRITE_ONCE(i2c_bus->target_active, false);
> +		return;
> +	}
> +
> +	sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
> +
> +	if (sts & AST2600_I2CS_SLAVE_MATCH)
> +		WRITE_ONCE(i2c_bus->target_active, true);
> +
> +	switch (sts) {
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> +		fallthrough;

[Severity: High]
Will emitting I2C_SLAVE_STOP before falling through to emit
I2C_SLAVE_WRITE_REQUESTED and I2C_SLAVE_WRITE_RECEIVED break backend state
machines? 

Some backends, like i2c-slave-eeprom, rely on the STOP event to commit writes,
so the order here might cause them to mishandle the transaction.

> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH:
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		cmd = TARGET_TRIGGER_CMD;
> +		if (sts & AST2600_I2CS_RX_DONE) {
> +			target_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +						       AST2600_I2CC_BUFF_CTRL));
> +			for (i = 0; i < target_rx_len; i++) {
> +				value = readb(i2c_bus->buf_base + i2c_bus->buf_size + i);

[Severity: High]
Could this loop read out-of-bounds mapped I/O memory? 

The target_rx_len extracted from the hardware register is not clamped to the
software-defined i2c_bus->buf_size. If the hardware reports a length up to 63,
iterating up to target_rx_len directly could cause an out-of-bounds MMIO read
from i2c_bus->buf_base.

> +				i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED, &value);
> +			}
> +		}
[ ... ]
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_WAIT_TX_DMA:
> +		if (sts & AST2600_I2CS_SLAVE_MATCH)
> +			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_REQUESTED, &value);

[Severity: High]
Does emitting I2C_SLAVE_WRITE_REQUESTED for a read match (WAIT_TX_DMA) corrupt
backend state? 

This appears to generate a spurious write request before properly requesting
the read data.

> +
> +		if (sts & AST2600_I2CS_RX_DONE) {
[ ... ]
> +	if (cmd)
> +		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +
> +	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
> +
> +	if (sts & AST2600_I2CS_STOP)
> +		WRITE_ONCE(i2c_bus->target_active, false);

[Severity: High]
Could clearing target_active unconditionally on AST2600_I2CS_STOP lead to
shared buffer corruption?

If the hardware coalesces a STOP condition from a previous transaction with a
SLAVE_MATCH for a new transaction, target_active is set to false while the
target hardware is still transmitting. A concurrent controller transfer might
then see the bus as free and overwrite the shared I2C buffer.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-upstream_i2c-v31-0-ba7a02714f22@aspeedtech.com?part=5