Re: [PATCH RESEND v2 2/2] i2c: cadence: Add support for Axiado AX3000

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Enabling I2C_FUNC_SMBUS_QUICK advertises 0-length reads, which the Cadence IP does not natively support, leading to bus hangs.

Pre-existing issues:
- [Critical] A state confusion vulnerability in `cdns_i2c_master_isr` leads to a Use-After-Free read when a 0-length read follows a failed transmit transfer.
- [High] Atomic transfers completely ignore hardware errors (such as NACKs), leading to false success reports or unacceptable polling timeouts.
--

commit ff52e0b03cba3111fdadd1c1d340a29ac8a5b77f
Author: Swark Yang <syang@axiado.com>

i2c: cadence: Add support for Axiado AX3000

The Axiado AX3000 SoC integrates a Cadence I2C controller that supports SMBus
Quick commands. This patch introduces the "axiado,ax3000-i2c" compatible
string and a new quirk to enable this functionality.

> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index 0fb728ade92e4..8079c045f9365 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
[ ... ]
> @@ -1175,10 +1176,14 @@ static int cdns_i2c_master_xfer_atomic(struct i2c_adapter *adap, struct i2c_msg
>   */
>  static u32 cdns_i2c_func(struct i2c_adapter *adap)
>  {
> +	struct cdns_i2c *id = adap->algo_data;
>  	u32 func = I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
>  			(I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
>  			I2C_FUNC_SMBUS_BLOCK_DATA;
>  
> +	if (id->quirks & CDNS_I2C_QUIRKS_ENABLE_SMBUS_QUICK_CFG)
> +		func |= I2C_FUNC_SMBUS_QUICK;
> +

[Severity: High]
Does adding I2C_FUNC_SMBUS_QUICK allow the I2C core to send unsupported
0-length reads to the adapter?

The Cadence controller does not natively support 0-length reads. If a 0-length
read is processed, cdns_i2c_mrecv() writes 0 to the Transfer Size Register
(CDNS_I2C_XFER_SIZE_OFFSET). This unsupported hardware state can cause the I2C
bus to hang or trigger rogue transfers.

Should the adapter's quirks be populated with I2C_AQ_NO_ZERO_LEN_READ to safely
support Quick commands?

[Severity: High]
This isn't a bug introduced by this patch, but do atomic transfers completely
ignore hardware errors like NACKs?

In atomic mode, cdns_i2c_process_msg() polls for completion by waiting
specifically for the CDNS_I2C_IXR_COMP bit. It entirely ignores error bits such
as CDNS_I2C_IXR_NACK. If a NACK occurs without COMP being set, the polling macro
blocks for the full timeout instead of failing fast.

Additionally, cdns_i2c_master_common_xfer() checks id->err_status_atomic to
report atomic errors, but this variable is never assigned anywhere in the driver.

Could this lead to system stalls during operations like SMBus Quick scanning?

[Severity: Critical]
This is a pre-existing issue, but does exposing 0-length reads via SMBUS_QUICK
trigger a state confusion vulnerability in cdns_i2c_master_isr()?

When a master transmit transfer fails (e.g., due to a NACK), the cleanup routine
does not reset id->send_count or id->p_send_buf.

If a subsequent 0-length read is issued (where msg->buf is NULL),
cdns_i2c_mrecv() sets id->p_recv_buf = NULL but leaves id->send_count and
id->p_send_buf containing stale values.

Upon interrupt completion, the ISR cdns_i2c_master_isr() evaluates
!id->p_recv_buf as true, erroneously enters the transmit completion block, loops
over id->send_count, and dereferences the previously freed id->p_send_buf.

Can this write unmapped or freed kernel memory to the hardware FIFO?

>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	func |= I2C_FUNC_SLAVE;
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-axiado-ax3000-cadence-i2c-support-v2-0-cfdad0534afa@axiado.com?part=2