Re: [PATCH v5 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Dangling `i2c->msg` pointer and lack of IRQ synchronization causes Use-After-Free / Memory Corruption on timeout.
- [High] Incomplete target unregistration and lack of IRQ sync causes NULL pointer dereference panic and bus freeze.
- [High] Zero-message transfer triggers a START condition followed by a ZERO_SIZE_PTR dereference panic.
- [Medium] Missing `pm_runtime_mark_last_busy()` breaks PM autosuspend delay logic.
- [Medium] Unlocked Read-Modify-Write of `MA35_CTL0` in system suspend path races with the IRQ handler.
- [Medium] Incorrect handling of `I2C_M_IGNORE_NAK` aborts the transaction while reporting success.
- [Medium] A malformed or unusually large `clock-frequency` device tree property causes division by zero during probe.
- [Low] Comment incorrectly describes receiving data as transmitting data.
--

commit 469758a7eb28d07aab76dce6fd3f09b0e77b5e45
Author: Zi-Yu Chen <zychennvt@gmail.com>
Subject: i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

Add I2C support for Nuvoton MA35D1 SoC.
The controller supports standard, fast and fast-plus modes,
and provides controller/target functionality.

> diff --git a/drivers/i2c/busses/i2c-ma35d1.c b/drivers/i2c/busses/i2c-ma35d1.c
> new file mode 100644
> index 0000000000000..2044479b608ed
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ma35d1.c
[ ... ]
> @@ -345,13 +345,11 @@ static void ma35d1_i2c_irq_controller_trx(struct ma35d1_i2c *i2c,
>  	case MA35_M_TRAN_ADDR_NACK:
>  	case MA35_M_RECE_ADDR_NACK:
>  		/* Controller Transmit Address NACK */
>  		/* 0x20: SLA+W has been transmitted and NACK has been received */
>  		/* 0x48: SLA+R has been transmitted and NACK has been received */
>  		if (i2c->msg->flags & I2C_M_IGNORE_NAK) {
>  			ma35d1_i2c_stop(i2c, 0);
>  		} else {

[Severity: Medium]
Does handling I2C_M_IGNORE_NAK by stopping the transfer and returning 0
correctly implement the ignore flag?

Typically, ignoring a NACK implies treating it as an ACK and continuing to the
data phase rather than aborting the transaction entirely.

>  			dev_dbg(i2c->dev, "\n i2c: ack was not received\n");
>  			ma35d1_i2c_stop(i2c, -ENXIO);
>  		}
>  		break;
[ ... ]
>  	case MA35_M_RECE_DATA_ACK:
>  	case MA35_M_RECE_DATA_NACK:
>  		/* DATA has been transmitted and ACK has been received */

[Severity: Low]
Is this comment accurate? Since this is the receive data case, shouldn't it
state that data has been received instead of transmitted?

>  		byte = readb(i2c->regs + MA35_DAT);
>  		i2c->msg->buf[i2c->msg_ptr++] = byte;
[ ... ]
> @@ -415,22 +415,19 @@ static int ma35d1_i2c_doxfer(struct ma35d1_i2c *i2c, struct i2c_msg *msgs,
>  			     int num)
>  {
>  	unsigned long timeout;
>  	unsigned int val;
>  	int ret, err;
>  
>  	i2c->msg = msgs;
>  	i2c->msg_num = num;
>  	i2c->msg_ptr = 0;
>  	i2c->msg_idx = 0;
>  	i2c->err = 0;
>  
>  	ma35d1_i2c_enable_irq(i2c);
>  	ma35d1_i2c_write_ctl(i2c, MA35_CTL_STA_SI);
>  
>  	timeout = wait_event_timeout(i2c->wait, i2c->msg_num == 0, HZ * 5);

[Severity: High]
What happens if ma35d1_i2c_doxfer() is called with num == 0 (e.g., via the
I2C_RDWR ioctl)? The hardware is issued a START condition, but the
i2c->msg_num == 0 condition in wait_event_timeout() evaluates to true
instantly.

When the hardware START interrupt fires, will ma35d1_i2c_irq_controller_trx()
then call ma35d1_i2c_set_addr() and dereference a ZERO_SIZE_PTR for i2c->msg?

>  	ma35d1_i2c_disable_irq(i2c);
>  	ret = i2c->msg_idx;
>  
>  	if (timeout == 0) {
>  		dev_dbg(i2c->dev, "xfer timeout\n");
>  		ret = -ETIMEDOUT;
>  		goto reset;
>  	}

[Severity: High]
Does this timeout path leave a dangling pointer in i2c->msg? If
ma35d1_i2c_disable_irq() runs but an interrupt handler is already executing or
fires slightly late, the handler will bypass the unlikely(!i2c->msg) check and
operate on memory that has already been freed by the caller.

Could this cause memory corruption when the handler writes to
i2c->msg->buf[i2c->msg_ptr++], or a hardware external abort when
ma35d1_i2c_reset() runs concurrently?

>  
>  	err = readl_poll_timeout(i2c->regs + MA35_CTL0, val,
[ ... ]
> @@ -445,15 +442,13 @@ static int ma35d1_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  			   int num)
>  {
>  	struct ma35d1_i2c *i2c = i2c_get_adapdata(adap);
>  	int ret;
>  
>  	ret = pm_runtime_resume_and_get(i2c->dev);
>  	if (ret)
>  		return ret;
>  
>  	ret = ma35d1_i2c_doxfer(i2c, msgs, num);
>  
>  	if (i2c->target) {
>  		ma35d1_i2c_write_ctl(i2c, MA35_CTL_SI_AA);
>  		ma35d1_i2c_enable_irq(i2c);
>  	}
>  	pm_runtime_put_autosuspend(i2c->dev);

[Severity: Medium]
Does dropping the PM reference here without calling
pm_runtime_mark_last_busy() break the autosuspend delay logic?

Since the last busy timestamp isn't updated, won't subsequent transfers
instantly suspend the controller once the initial 5000ms delay elapses,
instead of keeping it active?

>  
>  	return ret;
>  }
[ ... ]
> @@ -490,12 +485,11 @@ static int ma35d1_unreg_target(struct i2c_client *target)
>  {
>  	struct ma35d1_i2c *i2c = i2c_get_adapdata(target->adapter);
>  	int ret;
>  
>  	if (!i2c->target)
>  		return -EINVAL;
>  
>  	/* Disable I2C interrupt */
>  	ma35d1_i2c_disable_irq(i2c);
>  
>  	i2c->target = NULL;
>  
>  	ret = pm_runtime_put_sync(i2c->dev);

[Severity: High]
Can setting i2c->target to NULL without calling synchronize_irq() lead to a
NULL pointer dereference in ma35d1_i2c_irq_target_trx() if a concurrent
target interrupt fires?

Additionally, since ma35d1_i2c_disable_irq() does not clear the hardware
target address (MA35_ADDR0) or the AA bit in MA35_CTL0, does the I2C
controller remain active on the bus? Would it continue to ACK its slave
address without generating interrupts, permanently holding SCL low and
freezing the bus until PM fully suspends it?

>  	if (ret < 0)
>  		dev_err(i2c->dev, "failed to suspend i2c controller");
>  
>  	return 0;
>  }
[ ... ]
> @@ -529,15 +523,12 @@ static int ma35d1_i2c_suspend(struct device *dev)
>  {
>  	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
>  	unsigned int val;
>  
>  	/* Prepare for wake-up from I2C events if target mode is active */
>  	if (i2c->target) {
>  		val = readl(i2c->regs + MA35_CTL0);
>  		val |= (MA35_CTL_SI | MA35_CTL_AA);
>  		writel(val, i2c->regs + MA35_CTL0);

[Severity: Medium]
Does this unlocked read-modify-write on MA35_CTL0 race with
ma35d1_i2c_write_ctl() in the interrupt handler?

Because the device is in target mode, it can still receive transactions and
generate interrupts concurrently. Can this race corrupt the control register
state, such as losing the MA35_CTL_SI flag or inadvertently clearing I2CEN?

>  
>  		/* Setup wake-up control */
>  		writel(0x1, i2c->regs + MA35_WKCTL);
[ ... ]
> @@ -642,12 +633,11 @@ static int ma35d1_i2c_probe(struct platform_device *pdev)
>  	/* Default to 100kHz if not specified in DT */
>  	busfreq = 100000;
>  	device_property_read_u32(dev, "clock-frequency", &busfreq);
>  
>  	/* Calculate divider based on the current peripheral clock rate */
>  	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(i2c->clk), busfreq * 4) - 1;

[Severity: Medium]
Can this division crash the kernel during boot if the device tree contains a
malformed clock-frequency?

If busfreq is 0, or if it is an excessively large value that causes
busfreq * 4 to overflow a 32-bit integer back to 0, does this result in a
division by zero macro expansion?

>  	if (clkdiv < 0 || clkdiv > 0xffff)
>  		return dev_err_probe(dev, -EINVAL, "invalid clkdiv value: %d\n",
>  				     clkdiv);
>  
>  	i2c->irq = platform_get_irq(pdev, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630012106.3616821-1-zychennvt@gmail.com?part=2