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 <[email protected]> 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/[email protected]?part=1
