Re: [PATCH v2 01/12] i2c: core: add I2C_XFER_V2 - support for detailed transfer reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Message buffers allocated during initialization are leaked on error or partial transfer paths because the cleanup loop counter is incorrectly overwritten.
- [High] Incrementing the array index before writing `msgs[i].len` causes an out-of-bounds kernel heap write if the transfer fault occurs on the last message in the batch.
- [High] `dummy_report` is passed uninitialized to adapter drivers and its uninitialized contents dictate trace loop bounds, risking out-of-bounds memory accesses.
- [High] The `compat_i2cdev_ioctl()` handler is missing the `I2C_RDWR_V2` case, severely breaking 32-bit user-space compatibility on 64-bit kernels.
- [High] The `i2c_reply` tracepoint dynamically allocates and copies `msg->len` bytes instead of the actually transferred `data_len`.
- [Medium] The `fault_msg_idx` returned to userspace suffers from an off-by-one error during `I2C_M_RECV_LEN` validation.
- [Medium] Failure to copy `user_report` back to userspace when parameter validation fails in `i2cdev_ioctl_rdwr_v2`.
--

commit 57251745b527f28695f099c2ece7c79d4075938b
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: core: add I2C_XFER_V2 - support for detailed transfer reporting

This commit introduces the i2c_transfer_v2 API and I2C_RDWR_V2 ioctl to
provide detailed fault reporting during I2C transfers. It allows bus
controllers to specify which message failed and how many bytes were
transferred.

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index ddaacf876dada..a6515868f97ba 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
[ ... ]
> +int __i2c_transfer_v2(struct i2c_adapter *adap, struct i2c_msg *msgs, int num,
> +		   struct i2c_transfer_report *report)
>  {
> +	struct i2c_transfer_report dummy_report;
>  	unsigned long orig_jiffies;
>  	int ret, try;
[ ... ]
> +	/*
> +	 * If the controller only supports "v2" callback and the report is not requested,
> +	 * provide pointer to a dummy report.
> +	 */
> +	if (!(adap->algo->master_xfer) && (!report))
> +		report = &dummy_report;
[ ... ]
>  	if (static_branch_unlikely(&i2c_trace_msg_key)) {
> -		int i;
> -		for (i = 0; i < ret; i++)
> +		int n;
> +
> +		if (report)
> +			n = report->msgs_cplt;
> +		else
> +			n = ret;
> +		for (int i = 0; i < n; i++)

[Severity: High]
The dummy_report structure is declared on the stack but not initialized.

If the adapter only supports xfer_v2 and report is NULL, the code redirects
report to &dummy_report. If the transfer completes via the atomic fallback
or if xfer_v2 fails without populating the report, does this uninitialized
stack memory dictate the upper bound n in the tracing loop? This could
result in an out-of-bounds array read when tracing is enabled.

> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index ccaac5e29f906..90456e6c04b41 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
[ ... ]
> @@ -289,6 +297,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  					     I2C_SMBUS_BLOCK_MAX) {
>  				i++;
>  				res = -EINVAL;
> +				report.fault_msg_idx = i;
>  				break;
>  			}

[Severity: Medium]
Does this set the fault index to the incorrect message? The loop counter
is incremented before assigning it to report.fault_msg_idx. This means
userspace would be told the fault occurred on the message following the one
that actually failed the length validation.

[ ... ]
> @@ -303,9 +312,34 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		return res;
>  	}

[Severity: Medium]
If message validation fails (e.g., maximum length exceeded), the function
returns early with return res; here. Doesn't this skip the copy_to_user()
at the end of the function, discarding the fault_msg_idx that was recorded?

> -	res = i2c_transfer(client->adapter, msgs, nmsgs);
> +	if (user_report) {
> +		res = i2c_transfer_v2(client->adapter, msgs, nmsgs, &report);
> +		i = report.msgs_cplt;
> +	} else {
> +		res = i2c_transfer(client->adapter, msgs, nmsgs);
> +		if (res < 0)
> +			i = 0;
> +		else
> +			i = nmsgs;
> +	}

[Severity: High]
The loop counter is overwritten here with report.msgs_cplt or 0.
Since the cleanup loop uses while (i-- > 0), does this skip the
deallocation of the memory buffers from index i up to nmsgs - 1?
This could allow unprivileged users to leak kernel heap memory by
intentionally triggering partial transfers.

> +
> +	if (user_report && copy_to_user(user_report, &report, sizeof(report)))
> +		res = -EFAULT;
> +
> +	/* Number of messages transferred completely or partially */
> +	if (report.bytes_cplt > 0) {
> +		i++;
> +		msgs[i].len = report.bytes_cplt;
> +	}
> +

[Severity: High]
If the transfer fault occurs on the final message in the batch, the
overwritten loop counter will equal nmsgs - 1. If report.bytes_cplt > 0,
the counter is incremented to nmsgs before assigning msgs[i].len. Does this
result in an out-of-bounds kernel heap write to msgs[nmsgs]?

[ ... ]
> @@ -572,7 +627,7 @@ static long compat_i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned lo
>  			};
>  		}
>  
> -		res = i2cdev_ioctl_rdwr(client, rdwr_arg.nmsgs, rdwr_pa);
> +		res = i2cdev_ioctl_rdwr_v2(client, rdwr_arg.nmsgs, rdwr_pa, NULL);
>  		kfree(rdwr_pa);
>  		return res;
>  	}

[Severity: High]
The I2C_RDWR_V2 ioctl command is not added to the switch statement in
compat_i2cdev_ioctl(). Could this cause 32-bit processes calling this new
ioctl on 64-bit kernels to fall through to the native 64-bit i2cdev_ioctl(),
leading to struct layout parsing mismatches and memory corruption?

> diff --git a/include/trace/events/i2c.h b/include/trace/events/i2c.h
> index 142a23c6593c6..2ea8e9805edf5 100644
> --- a/include/trace/events/i2c.h
> +++ b/include/trace/events/i2c.h
[ ... ]
> @@ -102,7 +102,7 @@ TRACE_EVENT_FN(i2c_reply,
>  		       __entry->msg_nr = num;
>  		       __entry->addr = msg->addr;
>  		       __entry->flags = msg->flags;
> -		       __entry->len = msg->len;
> +		       __entry->len = data_len;
>  		       memcpy(__get_dynamic_array(buf), msg->buf, msg->len);
>  			      ),

[Severity: High]
While __entry->len is updated to use the provided data_len, the dynamic
array allocation (via __dynamic_array in TP_STRUCT__entry) and the
memcpy() call still use msg->len.

If data_len is greater than msg->len, could this cause TP_printk() to
read past the end of the trace ring buffer allocation? Conversely, if
data_len is smaller, does the memcpy() copy uninitialized kernel heap
memory into the trace ring buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-i2c-fault-reporting-v2-0-fedeb91792e6@mobileye.com?part=1