Re: [PATCH v5 4/5] i3c: add i3cdev module to expose i3c dev in /dev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] i3c: out-of-bounds shift due to unvalidated transfer mode
- [High] i3c: global DoS via userfaultfd stalling per-device xfer_lock
- [Medium] i3c: out-of-bounds read due to unverified actual_len in copy_to_user
- [Low] cleanup: mixing goto-based error handling with scope-based cleanup
--

commit b5d90eba8091c7ed2bb03166ab238062306fafa4
Author: Vitor Soares <vitor.soares@toradex.com>

i3c: add i3cdev module to expose i3c dev in /dev
    
Add userspace character device support for I3C transfers via /dev.
This allows userspace programs to interact directly with I3C targets
that do not have a kernel driver bound to them.

> diff --git a/drivers/i3c/i3cdev.c b/drivers/i3c/i3cdev.c
> new file mode 100644
> index 0000000000000..309ce81812096
> --- /dev/null
> +++ b/drivers/i3c/i3cdev.c
> @@ -0,0 +1,491 @@
> [ ... ]
> +static ssize_t
> +i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
> +{
> [ ... ]
> +	ret = i3c_device_do_xfers(i3c, &xfers, 1, I3C_SDR);
> +	if (ret)
> +		return ret;
> +
> +	if (copy_to_user(buf, tmp, xfers.actual_len))

[Severity: Medium]
Could this result in an out-of-bounds read to user space? If a buggy hardware
driver returns an actual_len that is greater than the requested transfer len
(or the allocated count), actual_len is used directly in copy_to_user()
without being clamped to the dynamically allocated buffer size, which might
leak kernel memory.

> +		return -EFAULT;
> +
> +	return xfers.actual_len;
> +}
> [ ... ]
> +static int
> +i3cdev_do_xfer(struct i3c_device *dev, struct i3c_ioc_xfer *xfers,
> +	       struct i3c_ioc_xfer __user *u_xfers, unsigned int nxfers)
> +{
> +	struct i3c_xfer *k_xfers __free(kfree) = NULL;

[Severity: Low]
Is it appropriate to mix scope-based and goto-based cleanup here? The
subsystem guidelines indicate that using goto-based error handling alongside
__free() in the same function creates confusing ownership semantics. In this
function, we have __free(kfree) here and goto err_free_mem below.

> +	enum i3c_xfer_mode mode = xfers[0].mode;

[Severity: High]
Can this user-provided mode cause an out-of-bounds shift?

If a mode value greater than or equal to 64 is passed via the ioctl, it is
extracted without validation and passed down:

i3cdev_do_xfer()
    i3c_device_do_xfers(dev, k_xfers, nxfers, mode)
        i3c_dev_do_xfers_locked()
            BIT(mode)

Since BIT(mode) does a bitwise shift, this would trigger undefined behavior.
Should this value be validated against valid I3C modes before proceeding?

> +	u8 **data_ptrs;
> +	int i, j, nalloc, ret = 0;
> [ ... ]
> +	ret = i3c_device_do_xfers(dev, k_xfers, nxfers, mode);
> +	if (ret)
> +		goto err_free_mem;
> +
> +	for (i = 0; i < nxfers; i++) {
> +		bool is_read = (mode == I3C_SDR) ?
> +			       (xfers[i].rnw == I3C_DEV_DIR_READ) :
> +			       (xfers[i].cmd & 0x80);
> +
> +		if (is_read) {
> +			if (copy_to_user(u64_to_user_ptr(xfers[i].data),
> +					 data_ptrs[i], k_xfers[i].actual_len) ||

[Severity: Medium]
Similar to i3cdev_read(), is actual_len verified here? An inflated
actual_len from the hardware driver could result in an out-of-bounds read
to user space without bounds clamping.

> +			    put_user(k_xfers[i].actual_len,
> +				     &u_xfers[i].actual_len)) {
> +				ret = -EFAULT;
> +				break;
> +			}
> +		}
> +	}
> +
> +err_free_mem:
> +	for (j = 0; j < nalloc; j++)
> +		kfree(data_ptrs[j]);
> +	return ret;
> +}
> [ ... ]
> +static long
> +i3cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c;
> +
> +	if (_IOC_TYPE(cmd) != I3C_DEV_IOC_MAGIC)
> +		return -ENOTTY;
> +
> +	/* Use the xfer_lock to prevent device detach during ioctl call */
> +	guard(mutex)(&i3cdev->xfer_lock);

[Severity: High]
Could holding this per-device xfer_lock throughout the ioctl create a global
DoS vulnerability?

The lock is held while calling sleepable user-memory access functions like
memdup_user() and copy_to_user() in i3cdev_do_xfer(). A userfaultfd stall
could freeze the thread here indefinitely.

Because i3cdev_detach() uses scoped_guard(mutex, &i3cdev->xfer_lock) while
also being called under the global i3cdev_attach_lock, any detach event
during this stall will block on xfer_lock, deadlocking the entire I3C
subsystem's hotplug and notification framework globally.

> +	i3c = i3cdev->i3c;
> +	if (!i3c || i3c->dev.driver)
> +		return -ENODEV;
> [ ... ]
> +static int i3cdev_attach(struct device *dev, void *dummy)
> +{
> +	struct i3cdev_data *i3cdev;
> +	struct i3c_device *i3c;
> +	int res;
> +
> +	if (dev->type == &i3c_masterdev_type)
> +		return 0;
> +
> +	i3c = dev_to_i3cdev(dev);
> +
> +	guard(mutex)(&i3cdev_attach_lock);

[Severity: Low]
Does this mix scope-based and goto-based cleanup? Like in i3cdev_do_xfer(),
this function uses guard() here but later uses goto error_put_dev for error
handling.

> +	if (dev->driver || i3cdev_get_by_i3c(i3c))
> +		return 0;
> [ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921230603.2518652-1-samagazaryan@google.com?part=4

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c