Re: [PATCH] Fix i.MX I2C driver zero byte read kernel panic

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect message array indexing when checking `I2C_M_RECV_LEN` misidentifies SMBus block reads, potentially hanging DMA.
- [High] SMBus Block Read protocol violation in `i2c_imx_prepare_read` due to NACKing the length byte in atomic mode.
- [High] DMA mapping leak on timeout path in `i2c_imx_dma_read` and `i2c_imx_dma_write`.
- [High] Race condition in `i2c_imx_unreg_slave` allows `i2c_imx->slave` to be set to NULL while the ISR is accessing it.
- [High] ISR can restart `slave_timer` after `hrtimer_cancel()`, leading to a UAF when the timer fires after device removal.
--

commit f83c2cb1b7414914f0e2d7d9321edd1e70cbad6d
Author: Krzysztof Hałasa <khalasa@piap.pl>
Fix i.MX I2C driver zero byte read kernel panic

This commit fixes a kernel panic caused by zero-sized I2C read requests in
the i.MX driver. By explicitly checking for missing length in the message,
the driver now rejects invalid zero-length reads with an error.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 19ec056b00afe..3f39e6027a4fb 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1640,7 +1640,9 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>  		if (msgs[i].flags & I2C_M_RD) {
>  			int block_data = msgs->flags & I2C_M_RECV_LEN;

[Severity: High]
This is a pre-existing issue, but does evaluating the first message
(msgs->flags) instead of the current message (msgs[i].flags) break SMBus
block read DMA filtering?

For a typical multi-message SMBus block read, msgs[0] does not have
I2C_M_RECV_LEN set, so block_data evaluates to 0.

Does this erroneously route the dynamic-length block read to
i2c_imx_dma_read(), which could cause the DMA transfer to hang and time out
since DMA expects a fixed transfer size?

> -			if (atomic)
> +			if (!msgs[i].len) /* invalid, would panic */
> +				result = -EINVAL;
> +			else if (atomic)
>  				result = i2c_imx_atomic_read(i2c_imx, &msgs[i], is_lastmsg);
>  			else if (use_dma && !block_data)
>  				result = i2c_imx_dma_read(i2c_imx, &msgs[i], is_lastmsg);
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the following logic in
i2c_imx_prepare_read() properly handle SMBus block reads in atomic contexts?

drivers/i2c/busses/i2c-imx.c:i2c_imx_prepare_read() {
...
	if (msgs->len - 1)
		temp &= ~I2CR_TXAK;
...
}

When an SMBus block read is performed in atomic mode, the core passes a
message length initialized to 1. Since 1 - 1 evaluates to 0, I2CR_TXAK
remains set.

Could this cause the controller to improperly send a NACK immediately after
receiving the count byte, aborting the transfer prematurely?

[Severity: High]
This is a pre-existing issue, but does the timeout error path in
i2c_imx_dma_read() leak the DMA mapping?

drivers/i2c/busses/i2c-imx.c:i2c_imx_dma_read() {
...
	time_left = wait_for_completion_timeout(
				&i2c_imx->dma->cmd_complete,
				msecs_to_jiffies(DMA_TIMEOUT));
	if (time_left == 0) {
		dmaengine_terminate_sync(dma->chan_using);
		return -ETIMEDOUT;
	}
...
}

If the I2C transfer times out, the function returns without unmapping the
DMA buffer via dma_unmap_single().

Because dmaengine_terminate_sync() prevents the completion callback from
executing, are we leaking IOMMU translation entries or bounce buffers here?
The same pattern appears to exist in i2c_imx_dma_write().

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
when clearing the slave pointer?

drivers/i2c/busses/i2c-imx.c:i2c_imx_unreg_slave() {
...
	hrtimer_cancel(&i2c_imx->slave_timer);
	i2c_imx->slave = NULL;
...
}

The teardown path clears i2c_imx->slave without acquiring slave_lock or
calling synchronize_irq().

If the interrupt service routine i2c_imx_isr() checks i2c_imx->slave and
enters i2c_imx_slave_handle() just as this teardown code nullifies the
pointer, could this result in a NULL pointer dereference in the ISR?

[Severity: High]
This is a pre-existing issue, but can the timer be erroneously restarted
after cancellation?

drivers/i2c/busses/i2c-imx.c:i2c_imx_slave_handle() {
...
	hrtimer_restart(&i2c_imx->slave_timer);
...
}

Because i2c_imx_unreg_slave() calls hrtimer_cancel() without synchronizing
against the ISR, can a concurrently executing ISR on another CPU restart
the timer after the cancellation has finished?

If the driver is subsequently removed, would the re-armed timer accessing
the freed i2c_imx structure lead to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/m37bl2d28d.fsf@t19.piap.pl?part=1