Re: [PATCH v4 2/3] i2c: nt72676: Add Novatek NT72676 I2C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The driver unconditionally treats any 2-message transfer as a subaddress operation, breaking I2C protocol semantics and leaking memory.
- [High] The interrupt handler improperly handles flags and active transfers, causing unacknowledged hardware interrupts and infinite interrupt storms.
- [High] Incorrect teardown order in `nvt_i2c_remove()` disables the I2C controller hardware before unregistering the adapter.
- [High] The driver fails to clear previous address and operation bits in `I2C_REG_CTRL` across multiple messages, corrupting I2C target addresses and read/write commands.
- [High] A race condition between the transfer timeout path and the interrupt handler leads to a Use-After-Free (UAF) of the I2C message buffer.
- [Medium] The driver fails to disable hardware interrupts in the probe error path, risking unhandled interrupts on a shared line.
- [Medium] The driver unconditionally dereferences the result of `of_device_get_match_data()` without checking for `NULL`, leading to a kernel crash on non-Device Tree platforms.
--

--- Patch [2]: [PATCH v4 2/3] i2c: nt72676: Add Novatek NT72676 I2C controller driver ---
commit 24e2f090fd92d0b0a276a1df0dc4a6e78c1ccda9
Author: Ben Huang <Ben_Huang@novatek.com.tw>

i2c: nt72676: Add Novatek NT72676 I2C controller driver

This commit adds the I2C controller driver for the Novatek NT72676 SoC.
It supports Standard-mode and Fast-mode I2C operations as a bus master.

> diff --git a/drivers/i2c/busses/i2c-nt72676.c b/drivers/i2c/busses/i2c-nt72676.c
> new file mode 100644
> index 0000000000000..138b01dd98a7f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nt72676.c
> @@ -0,0 +1,683 @@
[ ... ]
> +static irqreturn_t nvt_i2c_isr(int irq, void *dev_id)
> +{
> +	struct nvt_i2c_bus *i2c = dev_id;
> +	struct i2c_msg *msg = i2c->current_msg;
> +	unsigned int status = nt72676_i2c_readl(i2c, I2C_REG_INTR);
> +	unsigned int clr = 0;
> +	int do_complete = 0;
> +
> +	if (!(status & I2C_IRQ_FLAG) || !i2c->current_msg)
> +		return IRQ_NONE;

[Severity: High]
Will this cause an interrupt storm?

If a spurious interrupt occurs or an interrupt fires after a timeout when
current_msg is NULL, the handler returns IRQ_NONE without clearing the
interrupt flags.

Since the interrupt is requested with IRQF_TRIGGER_HIGH, failing to clear the
hardware state will leave the interrupt line continuously asserted.

[ ... ]
> +static int nvt_i2c_write(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +	ctrl_mask = nt72676_i2c_readl(i2c, I2C_REG_CTRL);
> +	ctrl_mask |= (((msg->addr << 1) << 8) | I2C_ENABLE |
> +			I2C_CLOCK_DUTY_ENABLE | I2C_CLOCK_STRETCH_ENABLE |
> +			I2C_MASTER_CLK_STRETCH_ENABLE | I2C_TRIGGER);
> +	nt72676_i2c_writel(ctrl_mask, i2c, I2C_REG_CTRL);

[Severity: High]
Does this construct a valid control mask for subsequent messages?

The new address and flags are bitwise-ORed onto the existing I2C_REG_CTRL
value without clearing the previous bits.

In a multi-message transfer where subsequent messages have different addresses
or read/write directions, the accumulated bits might corrupt the target
address and operation commands.

> +
> +	ret = wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.timeout);
> +	if (ret == 0) {
> +		i2c->error_code = -ETIMEDOUT;
> +		nvt_i2c_reset(i2c);
> +	}

[Severity: High]
Is there a race condition between this timeout path and the interrupt handler?

If a transfer times out exactly as the hardware interrupt fires, the timeout
path resets the hardware and returns to the caller, which may then free the
message buffer.

Because this does not synchronize with the ISR (for instance, by disabling the
interrupt or taking a lock), the ISR can concurrently access i2c->current_msg
and dereference a freed buffer when handling the late interrupt.

[ ... ]
> +static int nvt_i2c_xfer(struct i2c_adapter *adap,
> +			struct i2c_msg msgs[],
> +			int num)
> +{
> +	struct nvt_i2c_bus *i2c = i2c_get_adapdata(adap);
> +	int ret = 0, i = 0;
> +	struct i2c_msg *msg = NULL;
> +
> +	nvt_i2c_ctrl_init(i2c);
> +
> +	if (num == 2) {
> +		nvt_i2c_set_subaddr(i2c, &msgs[0]);
> +		msg = &msgs[1];

[Severity: High]
Does this correctly handle all two-message transfers?

The driver unconditionally treats any 2-message transfer as a subaddress
write operation. It reads from msgs[0].buf to populate the subaddress
register and then drops msgs[0].

If a client initiates a standard two-message sequence (like a sequential
read), the first message buffer might be uninitialized, and the first message
will be dropped, breaking standard I2C protocol semantics.

[ ... ]
> +static int nvt_i2c_parse_dts(struct nvt_i2c_bus *i2c)
> +{
> +	int ret;
> +	struct device *dev = i2c->dev;
> +	struct device_node *np = dev->of_node;
> +
> +	i2c->comp_data = of_device_get_match_data(dev);
> +
> +	/* read DTS(novatek,stbc-syscon) for STBC I2C */
> +	if (i2c->comp_data->stbc_i2c) {

[Severity: Medium]
Can of_device_get_match_data() return NULL here?

If the driver is instantiated via sysfs bind or legacy platform matching
on a non-Device-Tree platform, this will return NULL.

Dereferencing i2c->comp_data immediately after will cause a NULL pointer
dereference.

[ ... ]
> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = i2c_add_adapter(&i2c->adapter);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add adapter\n");
> +		return ret;
> +	}

[Severity: Medium]
Should the hardware interrupts be disabled before returning here?

If i2c_add_adapter() fails, the probe function returns an error without
disabling the I2C interrupts in the hardware.

The devres cleanup will free the interrupt handler, but the hardware might
still be enabled and assert the shared IRQ line, resulting in an unhandled
interrupt storm.

[ ... ]
> +static void nvt_i2c_remove(struct platform_device *pdev)
> +{
> +	struct nvt_i2c_bus *i2c = platform_get_drvdata(pdev);
> +
> +	nt72676_i2c_writel(I2C_IRQ_DISABLE_SETTING, i2c, I2C_REG_INTR);
> +	nt72676_i2c_writel(nt72676_i2c_readl(i2c, I2C_REG_CTRL) & ~I2C_ENABLE,
> +			   i2c, I2C_REG_CTRL);
> +	i2c_del_adapter(&i2c->adapter);
> +}

[Severity: High]
Is this teardown order correct?

The hardware is disabled before i2c_del_adapter() unregisters the clients.

Many client drivers need to perform I2C transfers in their remove() functions
to gracefully power down devices. Disabling the controller hardware first
guarantees that these shutdown transfers will fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730072529.492398-1-nina_kuo@novatek.com.tw?part=2