Re: [PATCH v4] i2c: imx: honour I2C_M_IGNORE_NAK

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] i2c: imx: out-of-bounds write on zero-length read in i2c_imx_isr_read()
--

commit 9dd0c3d38f0968bb16ae7d2a6e9822f507de880e
Author: Haobin Jiang <lemonoutput@foxmail.com>

i2c: imx: honour I2C_M_IGNORE_NAK

This commit honours I2C_M_IGNORE_NAK in the i.MX I2C controller driver by
removing I2C_M_IGNORE_NAK and I2C_M_STOP from the unsupported protocol
mangling flags and ignoring missing ACKs when I2C_M_IGNORE_NAK is set.
Messages with I2C_M_IGNORE_NAK are also forced to use the non-DMA transfer
path.

Link: https://patch.msgid.link/20261023-for-upstream-i2c-imx-lx2160-reverse-v3-1-a07b249df381@free.fr # [1]
Link: https://lore.kernel.org/linux-i2c/tencent_342A87BFCECA70C9982E68A79AAD878B6909@qq.com/ # [2]

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index ccf08cd63151f..8e422b45dc2bb 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c

[ ... ]

> @@ -988,7 +995,7 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
>  {
>  	int result;
>  
> -	result = i2c_imx_isr_acked(i2c_imx);
> +	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>  	if (result)
>  		return result;
>  
> @@ -1017,7 +1024,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
>  {
>  	int result;
>  
> -	result = i2c_imx_isr_acked(i2c_imx);
> +	result = i2c_imx_isr_acked(i2c_imx, i2c_imx->msg->flags & I2C_M_IGNORE_NAK);
>  	if (result)
>  		return result;
>  

[Severity: High]
What happens here if a zero-length read message (msg->len == 0 with
I2C_M_RD) is transferred, particularly now that I2C_M_IGNORE_NAK allows
i2c_imx_isr_acked() to succeed even when no device ACKs the address byte?

In i2c_imx_check_msgs(), zero-length messages are only rejected when
I2C_M_NOSTART is set:

drivers/i2c/busses/i2c-imx.c:i2c_imx_check_msgs() {
	...
		if (msgs[i].flags & I2C_M_NOSTART) {
			if (i == 0 || msgs[i].len == 0)
				return -EOPNOTSUPP;
	...
}

Unlike i2c_imx_isr_write(), which checks whether i2c_imx->msg->len ==
i2c_imx->msg_buf_idx after i2c_imx_isr_acked(), i2c_imx_isr_read()
unconditionally calls i2c_imx_setup_read() and transitions the state machine
in i2c_imx_master_isr() to IMX_I2C_STATE_READ_CONTINUE:

drivers/i2c/busses/i2c-imx.c:i2c_imx_setup_read() {
	...
	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
	temp &= ~I2CR_MTX;
	if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags & I2C_M_RECV_LEN))
		temp &= ~I2CR_TXAK;

	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
}

When msg->len is 0, (i2c_imx->msg->len - 1) is non-zero, so
i2c_imx_setup_read() clears I2CR_MTX and I2CR_TXAK and performs a dummy read
of IMX_I2C_I2DR, causing the controller to clock in a data byte. When that
byte completes, i2c_imx_master_isr() -> i2c_imx_isr_read_continue() runs:

drivers/i2c/busses/i2c-imx.c:i2c_imx_isr_read_continue() {
	...
	if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
		...
	} else if (i2c_imx->msg_buf_idx == (i2c_imx->msg->len - 2)) {
		...
	}

	i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
	return next_state;
}

Because msg->len is 0, neither (msg->len - 1) nor (msg->len - 2) matches
msg_buf_idx, so i2c_imx_isr_read_continue() writes out of bounds into
i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] on every interrupt (dereferencing
ZERO_SIZE_PTR when called via I2C_RDWR ioctl with len == 0, or overflowing
msgbuf0 on the stack during an emulated SMBus Quick Read). Similarly, in
i2c_imx_atomic_read() -> i2c_imx_prepare_read(), the controller is switched
to receive mode and performs a dummy read even when msgs->len is 0.

Should zero-length reads be handled before calling i2c_imx_setup_read(), or
rejected via adapter quirks / i2c_imx_check_msgs()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_877C99956190215D644EB959DF1663542A0A@qq.com?part=1