Re: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART

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

Pre-existing issues:
- [High] Zero-length I2C reads cause a NULL pointer dereference and kernel panic in the interrupt handler due to integer underflow.
--

--- Patch [1]: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART ---
commit 600d2bffc8575545709ea03b9e6d37e2e2b2e812
Author: Vincent Jardin <[email protected]>

i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
    
This commit adds support for I2C_M_REV_DIR_ADDR and I2C_M_NOSTART flags in
the i2c-imx driver. It modifies the read/write paths and ISR to handle
messages that continue frames without re-addressing or invert the R/W bit.

> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
[ ... ]
> -static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
> +static inline void i2c_imx_setup_read(struct imx_i2c_struct *i2c_imx)
>  {
> -	int result;
>  	unsigned int temp;
>  
> -	result = i2c_imx_isr_acked(i2c_imx);
> -	if (result)
> -		return result;
> -
> -	/* setup bus to read data */
>  	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))
> @@ -1017,6 +1011,18 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
>  
>  	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>  	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
> +}

[Severity: High]
This is a pre-existing issue, but can a zero-length I2C read cause an integer
underflow here?

When an application uses the I2C_RDWR ioctl with a read message of length 0,
would (i2c_imx->msg->len - 1) underflow to a non-zero value and bypass the
intended logic to NACK the read?

If this underflows, the driver appears to clear I2CR_TXAK and issues a dummy
read to start data reception. Later, in i2c_imx_isr_read_continue(), the
underflowed length values bypass the loop termination conditions:

    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);

Since the I2C core sets msg->buf to NULL for zero-length messages, does this
result in the driver writing to a NULL pointer and causing a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-for-upstream-i2c-imx-lx2160-reverse-v1-1-f276ce8cb660@free.fr?part=1