Re: [PATCH RFC v3 09/14] i2c: core: implement I2C_RDWR_V2 ioctl - transfer with detailed report

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] i2c: dev: missing bytes_cplt bounds check in i2cdev_ioctl_rdwr_v2()
- [High] i2c: missing report reset and bounds checks in __i2c_transfer_v2()
- [Medium] i2c: dev: I2C_RDWR_V2 with NULL msgs and non-zero nmsgs returns 0 in i2cdev_ioctl()
- [Medium] i2c: dev: missing algo->xfer_v2 check in i2cdev_ioctl_rdwr_v2()
- [Low] i2c: dev: leftover fixme, sign-compare, and logging in i2cdev_ioctl_rdwr_v2()
--

commit 7d55aa9159adadd2b0feb48eabc72a0f050719a1
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: core: implement I2C_RDWR_V2 ioctl - transfer with detailed report

This commit adds the I2C_RDWR_V2 ioctl (and its compat counterpart) to
i2c-dev so userspace can receive an i2c_transfer_report describing completed
messages, bytes transferred, and the faulting message index when a transfer
fails.

> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index d219eb89f978b..14fe1dd7eecf4 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -240,11 +240,19 @@ static int i2cdev_check_addr(struct i2c_adapter *adapter, unsigned int addr)
>  	return result;
>  }
>  
> -static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
> -		unsigned nmsgs, struct i2c_msg *msgs)
> +static noinline int i2cdev_ioctl_rdwr_v2(struct i2c_client *client,
> +					 unsigned int nmsgs, struct i2c_msg *msgs,
> +					 struct i2c_transfer_report __user *user_report)
>  {
> +	struct i2c_transfer_report report;
>  	u8 __user **data_ptrs;
> -	int i, res;
> +	int i, res, msgs_done;
> +
> +	i2c_transfer_report_init(&report);
> +
> +	if (user_report)
> +		if (copy_to_user(user_report, &report, sizeof(report)))
> +			return -EFAULT;
>  
>  	/* Adapter must support I2C transfers */
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>  		return -EOPNOTSUPP;

[Severity: Medium]
When user_report is non-NULL (for I2C_RDWR_V2 with nmsgs > 0), should
i2cdev_ioctl_rdwr_v2() also verify client->adapter->algo->xfer_v2 before
allocating and validating the messages?

In drivers/i2c/i2c-core-base.c, __i2c_transfer_v2() initializes the report
fields to -EOPNOTSUPP and immediately returns -EOPNOTSUPP when
adap->algo->xfer_v2 is NULL:

drivers/i2c/i2c-core-base.c:__i2c_transfer_v2() {
	if (report) {
		i2c_transfer_report_init(report);

		if (!adap->algo->xfer_v2)
			return -EOPNOTSUPP;
	}
}

Because i2cdev_ioctl_rdwr_v2() only checks algo->xfer_v2 later via
i2c_transfer_v2(), calling I2C_RDWR_V2 on an adapter without xfer_v2 with a
message that fails validation (such as msgs[i].len > 8192, a memdup_user()
failure, or an invalid I2C_M_RECV_LEN buffer) will overwrite report via
i2c_transfer_report_bad_msg(&report, i) and copy that to user_report instead
of leaving the report fields set to -EOPNOTSUPP and returning -EOPNOTSUPP.
Even when all messages are valid, it allocates data_ptrs and duplicates every
message buffer with memdup_user() before __i2c_transfer_v2() rejects the
transfer.

> @@ -259,6 +267,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		/* Limit the size of the message to a sane amount */
>  		if (msgs[i].len > 8192) {
>  			res = -EINVAL;
> +			i2c_transfer_report_bad_msg(&report, i);
>  			break;
>  		}
>  
> @@ -266,6 +275,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		msgs[i].buf = memdup_user(data_ptrs[i], msgs[i].len);
>  		if (IS_ERR(msgs[i].buf)) {
>  			res = PTR_ERR(msgs[i].buf);
> +			i2c_transfer_report_bad_msg(&report, i);
>  			break;
>  		}
>  		/* memdup_user allocates with GFP_KERNEL, so DMA is ok */
> @@ -287,6 +297,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  			    msgs[i].len < 1 || msgs[i].buf[0] < 1 ||
>  			    msgs[i].len < msgs[i].buf[0] +
>  					     I2C_SMBUS_BLOCK_MAX) {
> +				i2c_transfer_report_bad_msg(&report, i);
>  				i++;
>  				res = -EINVAL;
>  				break;
> @@ -300,12 +311,44 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client,
>  		for (j = 0; j < i; ++j)
>  			kfree(msgs[j].buf);
>  		kfree(data_ptrs);
> +		if (user_report && copy_to_user(user_report, &report, sizeof(report)))
> +			res = -EFAULT;
>  		return res;
>  	}
>  
> -	res = i2c_transfer(client->adapter, msgs, nmsgs);
> +	if (user_report) {
> +		res = i2c_transfer_v2(client->adapter, msgs, nmsgs, &report);
> +		msgs_done = report.msgs_cplt;

[Severity: High]
Passing a non-NULL &report into i2c_transfer_v2() -> __i2c_transfer_v2()
activates the report paths in drivers/i2c/i2c-core-base.c:

drivers/i2c/i2c-core-base.c:__i2c_transfer_v2() {
	...
	/* Retry automatically on arbitration loss */
	orig_jiffies = jiffies;
	for (ret = 0, try = 0; try <= adap->retries; try++) {
		if (i2c_in_atomic_xfer_mode() && adap->algo->master_xfer_atomic) {
			ret = adap->algo->master_xfer_atomic(adap, msgs, num);
		} else {
			if (report)
				ret = adap->algo->xfer_v2(adap, msgs, num, report);
			else
				ret = adap->algo->master_xfer(adap, msgs, num);
		}

		if (ret != -EAGAIN)
			break;
		if (time_after(jiffies, orig_jiffies + adap->timeout))
			break;
	}

	if (static_branch_unlikely(&i2c_trace_msg_key)) {
		int i, n;

		if (report)
			n = report->msgs_cplt;
		else
			n = ret;
		for (i = 0; i < n; i++)
			if (msgs[i].flags & I2C_M_RD)
				trace_i2c_reply(adap, &msgs[i], msgs[i].len, i);
		if (report && report->bytes_cplt > 0 && msgs[n].flags & I2C_M_RD)
			trace_i2c_reply(adap, &msgs[n], report->bytes_cplt, n);
		trace_i2c_result(adap, num, ret);
	}
}

Could this cause out-of-bounds accesses on msgs[] or stale report data in
__i2c_transfer_v2()?

First, i2c_transfer_report_init(report) is called once before the retry loop
rather than before each retry attempt. If an attempt returns -EAGAIN on
arbitration loss after adap->algo->xfer_v2() has populated report, the next
retry starts with non-negative report->msgs_cplt and report->bytes_cplt from
the previous attempt (for example, in i2c_dw_fault_report() where
report->msgs_cplt < 0 guards initial assignment and report->msgs_cplt +=
msgs_in_prev_parts accumulates across parts).

Second, when i2c_trace_msg_key tracepoints are enabled, __i2c_transfer_v2()
uses n = report->msgs_cplt in the loop and indexes msgs[n] when
report->bytes_cplt > 0 without checking that 0 <= n < num or that
report->bytes_cplt <= msgs[n].len, whereas the bounds checks in
i2cdev_ioctl_rdwr_v2() only run after i2c_transfer_v2() returns.

Third, if i2c_in_atomic_xfer_mode() && adap->algo->master_xfer_atomic is
taken while report is non-NULL, master_xfer_atomic() does not update report,
leaving report->msgs_cplt set to -EOPNOTSUPP even on a successful transfer.

> +	} else {
> +		res = i2c_transfer(client->adapter, msgs, nmsgs);
> +		if (res < 0)
> +			msgs_done = 0;
> +		else
> +			msgs_done = nmsgs;
> +	}
> +
> +	if (user_report && copy_to_user(user_report, &report, sizeof(report)))
> +		res = -EFAULT;
> +
> +	if (msgs_done > (int)nmsgs) {
> +		//fixme - use dev_err?
> +		pr_err("Bad i2c_transfer_report: msgs_cplt = %i, bytes_cplt = %i, nmsgs = %i\n",
> +		       report.msgs_cplt, report.bytes_cplt, nmsgs);
> +		msgs_done = nmsgs;
> +	}
> +
> +	if (report.bytes_cplt > 0) {
> +		if (msgs_done < nmsgs && msgs_done >= 0) {
> +			msgs[msgs_done].len = report.bytes_cplt;
> +			msgs_done++;
> +		} else {
> +			pr_err("Bad i2c_transfer_report: msgs_cplt = %i, bytes_cplt = %i, nmsgs = %i\n",
> +				report.msgs_cplt, report.bytes_cplt, nmsgs);
> +		}
> +	}

[Severity: Low]
In i2cdev_ioctl_rdwr_v2(), there is a leftover "//fixme - use dev_err?"
comment above pr_err(). Should these pr_err() calls use
dev_err(&client->adapter->dev, ...) instead, and format unsigned int nmsgs
with %u?

Also, in the check:

	if (msgs_done < nmsgs && msgs_done >= 0)

msgs_done is a signed int while nmsgs is an unsigned int (unlike the earlier
check which casts nmsgs to int). Because of usual arithmetic conversions,
msgs_done is converted to unsigned int in msgs_done < nmsgs, which triggers a
-Wsign-compare warning under W=1 and makes msgs_done >= 0 redundant.

Finally, if a driver reports both report.msgs_cplt > nmsgs and
report.bytes_cplt > 0, the first block logs "Bad i2c_transfer_report" and
clamps msgs_done = nmsgs, which then causes msgs_done < nmsgs in the second
block to be false and logs the same error message a second time, all after
report was already copied to user_report.

> +
>  	while (i-- > 0) {
> -		if (res >= 0 && (msgs[i].flags & I2C_M_RD)) {
> +		if (msgs[i].flags & I2C_M_RD && i < msgs_done) {
>  			if (copy_to_user(data_ptrs[i], msgs[i].buf,
>  					 msgs[i].len))
>  				res = -EFAULT;

[Severity: High]
Can copy_to_user() read past the end of the allocated msgs[i].buf buffer in
i2cdev_ioctl_rdwr_v2()?

Earlier in i2cdev_ioctl_rdwr_v2(), each kernel buffer is allocated with the
original message length:

	msgs[i].buf = memdup_user(data_ptrs[i], msgs[i].len);

When report.bytes_cplt > 0 and 0 <= msgs_done < nmsgs, the code overwrites
msgs[msgs_done].len without checking that report.bytes_cplt does not exceed
the allocated msgs[msgs_done].len:

	if (report.bytes_cplt > 0) {
		if (msgs_done < nmsgs && msgs_done >= 0) {
			msgs[msgs_done].len = report.bytes_cplt;
			msgs_done++;
		}
	}

If a bus driver sets report.bytes_cplt to a value larger than
msgs[msgs_done].len (or if msgs[i].len was reduced to msgs[i].buf[0] for
I2C_M_RECV_LEN), copy_to_user(data_ptrs[i], msgs[i].buf, msgs[i].len) will
read out of bounds from the kernel heap buffer msgs[i].buf and copy past the
end of the userspace buffer. Should report.bytes_cplt be validated against
msgs[msgs_done].len before assigning it?

> @@ -439,18 +482,39 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		funcs = i2c_get_functionality(client->adapter);
>  		return put_user(funcs, (unsigned long __user *)arg);
>  
> -	case I2C_RDWR: {
> +	case I2C_RDWR:
> +	case I2C_RDWR_V2:
> +	{
> +		struct i2c_rdwr_ioctl_data __user *user_arg;
> +		struct i2c_transfer_report __user *user_rep;
>  		struct i2c_rdwr_ioctl_data rdwr_arg;
>  		struct i2c_msg *rdwr_pa;
>  		int res;
>  
> -		if (copy_from_user(&rdwr_arg,
> -				   (struct i2c_rdwr_ioctl_data __user *)arg,
> -				   sizeof(rdwr_arg)))
> +		if (cmd == I2C_RDWR_V2) {
> +			user_arg = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->rdwr_data;
> +			user_rep = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->report;
> +		} else {
> +			user_arg = (struct i2c_rdwr_ioctl_data __user *)arg;
> +			user_rep = NULL;
> +		}
> +
> +		if (copy_from_user(&rdwr_arg, user_arg, sizeof(rdwr_arg)))
>  			return -EFAULT;
>  
> -		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)
> -			return -EINVAL;
> +		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0) {
> +			/*
> +			 * I2C_RDWR_V2 ioctl with nmsgs == 0 is used for
> +			 * discovering of the controller capability to return
> +			 * detailed fault reports.
> +			 */
> +			if (cmd == I2C_RDWR)
> +				return -EINVAL;
> +			if (client->adapter->algo->xfer_v2)
> +				return 0;
> +			else
> +				return -EOPNOTSUPP;
> +		}

[Severity: Medium]
In both i2cdev_ioctl() and compat_i2cdev_ioctl(), what happens when a caller
invokes I2C_RDWR_V2 with rdwr_arg.nmsgs > 0 and rdwr_arg.msgs == NULL?

Because !rdwr_arg.msgs is combined with rdwr_arg.nmsgs == 0 in the same
condition, passing a NULL msgs pointer with a non-zero nmsgs enters the
capability discovery branch and returns 0 (when algo->xfer_v2 is present)
without transferring any messages or updating user_rep, instead of returning
-EINVAL. Should capability discovery only trigger when rdwr_arg.nmsgs == 0,
and return -EINVAL when rdwr_arg.msgs is NULL with rdwr_arg.nmsgs > 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-i2c-fault-reporting-v3-0-515ad74ac4c9@mobileye.com?part=9