Re: [PATCH RESEND v29 4/4] i2c: ast2600: Add target mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Aborting an active controller transfer from the target IRQ handler leads to Use-After-Free and Double-Complete.
- [High] The target interrupt handler fails to fully clear `AST2600_I2CS_ISR`, causing a permanent controller lockout and interrupt storm.
- [High] Missing buffer length bounds checks allow out-of-bounds MMIO reads and heap buffer overflows.
- [High] The `target_active` state is not reliably cleared, leading to a permanent controller deadlock.
- [High] Teardown ordering in `ast2600_i2c_unreg_target` introduces a Use-After-Free race and leaves interrupts unhandled.
- [High] Target hardware is enabled before the software target pointer is initialized, causing a race condition.
- [High] Invalid I2C slave event ordering and stalled target state machine on coalesced interrupts.
- [High] Target mode uses 8-bit `writeb()` on an MMIO buffer that strictly requires 32-bit dword writes.
- [Medium] The driver silently accepts 10-bit target addresses and corrupts adjacent hardware registers.
- [Medium] An uninitialized stack variable is passed to the target subsystem callback, risking a stack memory leak.
--

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

i2c: ast2600: Add target mode support
    
Add target mode support to the AST2600 I2C driver. Target mode features
implemented include target interrupt handling and address match and response
logic.

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> index 787ef6bd62442..137aaf7e26b51 100644
> --- a/drivers/i2c/busses/i2c-ast2600.c
> +++ b/drivers/i2c/busses/i2c-ast2600.c
[ ... ]
> @@ -337,6 +342,243 @@ 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)
> +{
> +	int target_rx_len = 0;
> +	u32 cmd = 0;
> +	u8 value;

Could this result in leaking uninitialized stack memory?

The local variable value is uninitialized, but it is passed by reference to
i2c_slave_event() below during events like I2C_SLAVE_STOP.

> +	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);
> +		i2c_bus->cmd_err = -EBUSY;
> +		writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		complete(&i2c_bus->cmd_complete);
> +	}

Is there a risk of a use-after-free and double-complete here?

If an ongoing controller transfer is aborted and complete() is called
immediately, the waiting thread in ast2600_i2c_controller_xfer() might exit
and free the i2c_bus->msgs buffers. 

The hardware could have already asserted the master interrupt, or it might
assert it as a result of the abort. When the controller IRQ handler eventually
processes this, it could use the freed i2c_bus->msgs pointer and call
complete() again.

[ ... ]
> +	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;
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:

Could this event ordering stall the target state machine? 

For combined interrupts, this emits I2C_SLAVE_STOP before falling through to
emit I2C_SLAVE_WRITE_REQUESTED and I2C_SLAVE_WRITE_RECEIVED. This appears to
violate the API contract and might cause the backend to terminate the
transaction prematurely.

[ ... ]
> +		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));

Should we bound check target_rx_len against i2c_bus->buf_size here?

It looks like up to 63 bytes can be read from the register without validation,
which might allow reading beyond the mapped MMIO SRAM region, potentially
causing a synchronous external abort.

Also, looking at ast2600_i2c_controller_packet_irq(), it trusts the xfer_len
from this same register without bounding it against msg->len. Since this IRQ
handler zeroes BUFF_CTRL during an abort, races could lead to corrupted
lengths and out-of-bounds writes to the kernel msg->buf allocation.

> +			for (i = 0; i < target_rx_len; i++) {
> +				value = readb(i2c_bus->buf_base + 0x10 + i);
> +				i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED, &value);
> +			}
> +		}
[ ... ]
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_RX_DONE |
> +	     AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_STOP:
> +		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 + 0x10 + i);
> +			i2c_slave_event(i2c_bus->target, I2C_SLAVE_WRITE_RECEIVED, &value);
> +		}
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_STOP, &value);
> +		i2c_slave_event(i2c_bus->target, I2C_SLAVE_READ_REQUESTED, &value);
> +		writeb(value, i2c_bus->buf_base);

Does this MMIO access require 32-bit dword writes? 

A comment in ast2600_i2c_setup_buff_tx() mentions that the controller's
buffer register supports dword writes only, but this code is using writeb().
Could this lead to ignored writes or bus faults?

> +		break;
[ ... ]
> +	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);

Will this fully clear the hardware status register?

AST2600_I2CS_ISR is typically a write-1-to-clear register. Since this only
writes AST2600_I2CS_PKT_DONE back, active bits like SLAVE_MATCH, STOP, or
RX_DONE remain uncleared in the hardware. This could leave the IRQ line
asserted, causing an interrupt storm, and potentially lock out future
controller transfers since ast2600_i2c_controller_xfer() checks for a non-zero
ISR.

> +
> +	if ((sts & AST2600_I2CS_STOP) && !(sts & AST2600_I2CS_SLAVE_PENDING))
> +		WRITE_ONCE(i2c_bus->target_active, false);
> +}

Is it possible for target_active to remain true indefinitely here? 

For standard coalesced interrupts, SLAVE_PENDING might be set alongside STOP,
meaning this condition fails. This would leave target_active true and
permanently block future master transfers.

[ ... ]
> +static int ast2600_i2c_reg_target(struct i2c_client *client)
> +{
> +	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
> +	u32 cmd = TARGET_TRIGGER_CMD;
> +
> +	if (i2c_bus->target)
> +		return -EINVAL;
> +
> +	dev_dbg(i2c_bus->dev, "target addr %x\n", client->addr);

Should we check if the client address uses the I2C_CLIENT_TEN flag?

Without validation, the upper bits of a 10-bit address could overflow into the
adjacent ADDR2 hardware configuration bits when written to
AST2600_I2CS_ADDR_CTRL.

> +
> +	writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +	writel(AST2600_I2CC_SLAVE_EN | readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL),
> +	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> +	writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +	i2c_bus->target = client;

Could there be a race condition here? 

Setting AST2600_I2CC_SLAVE_EN enables the target hardware before
i2c_bus->target is assigned the client pointer. If a transaction starts in
this window, the interrupt handler might fire while target is still NULL,
skipping processing and causing an interrupt storm.

> +	/* Set target addr. */
> +	writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
> +	       i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +
> +	return 0;
> +}
> +
> +static int ast2600_i2c_unreg_target(struct i2c_client *client)
> +{
> +	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
> +	u32 val;
> +
> +	/* Turn off target mode. */
> +	val = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +	writel(val & ~AST2600_I2CC_SLAVE_EN, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +	val = readl(i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +	writel(val & ~AST2600_I2CS_ADDR1_MASK, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +
> +	synchronize_irq(i2c_bus->irq);
> +	i2c_bus->target = NULL;

Does this teardown ordering introduce a use-after-free race?

If an interrupt fires just after SLAVE_EN is disabled but before
synchronize_irq completes, the IRQ handler might run concurrently with
unregistration and pass the freed target pointer to the subsystem callback.

Additionally, the AST2600_I2CS_IER target interrupt enable bit is never
cleared. If a spurious or pending target interrupt fires after unregistration,
the handler will skip target processing, failing to clear the interrupt and
causing an interrupt storm.

Also, does target_active need to be cleared to false here? If a target is
unregistered while active, it appears controller mode transfers might be
permanently blocked.

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-upstream_i2c-v29-0-fe9926964d55@aspeedtech.com?part=4