Re: [PATCH 3/3] i3c: add i3cdev character device module for user-space access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Max Write Length (MWL) check is bypassed in `i3cdev_write` due to uninitialized length field.
- [Medium] Mixing goto-based error handling and `scoped_guard()` declarations in the same function violates the cleanup subsystem guidelines.
- [Medium] Memory and reference count leak on the `dev_set_name()` error path in `i3cdev_probe()`.
- [Medium] Un-ratelimited kernel logging in `print_i3c_err` allows unprivileged users to flood dmesg.
--

commit 691a30342eecac1b5bf88181bfab4ade443f2923
Author: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
Subject: i3c: add i3cdev character device module for user-space access

This commit introduces a character device driver for I3C devices. It allows
user-space applications to perform basic SDR reads and writes by binding
devices manually via sysfs driver_override.

> diff --git a/drivers/i3c/i3cdev.c b/drivers/i3c/i3cdev.c
> new file mode 100644
> index 0000000000000..da0e6910a4f72
> --- /dev/null
> +++ b/drivers/i3c/i3cdev.c

[ ... ]

> +static ssize_t
> +i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
> +{

[ ... ]

> +	scoped_guard(mutex, &i3cdev->lock) {
> +		/* .remove was called so don't mess with the device */
> +		if (!i3cdev->bound) {
> +			ret = -ENXIO;
> +			goto out_free_kbuf;
> +		}
> +
> +		ret = i3c_device_do_xfers(i3c, &xfer, 1, I3C_SDR);
> +		if (ret)
> +			goto out_free_kbuf;
> +	}

[Severity: Medium]
Does this mix goto-based error handling with scoped_guard() in the same
function?

The VFS/cleanup guidelines indicate that mixing goto-based error labels with
scope-based cleanups like scoped_guard() creates confusing ownership semantics
and increases the risk of double-free or resource leaks. Since the function
uses a goto label for freeing the memory buffer out_free_kbuf, could we
either convert the buffer to use a scope-based __free() or use traditional
mutex lock/unlock operations?

[ ... ]

> +static ssize_t
> +i3cdev_write(struct file *file, const char __user *buf, size_t count,
> +	     loff_t *f_pos)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c = i3cdev->i3c;
> +	struct i3c_xfer xfer = {
> +		.rnw = I3C_WRITE
> +	};
> +	size_t len;
> +	char *tmp;
> +	int ret;
> +
> +	if (!i3cdev->bound)
> +		return -ENXIO;
> +
> +	/* File system operation of 0 bytes is effectively a valid, no-op */
> +	if (!count)
> +		return 0;
> +
> +	if (exceeds_mwl(i3cdev, &xfer))
> +		return -ENXIO;
> +
> +	/* Clamp transfer length within driver limits */
> +	len = min(count, MAX_TOTAL_DATA_BYTES);
> +	xfer.len = len;

[Severity: Medium]
Is the Max Write Length (MWL) check bypassed here due to an uninitialized
length field?

At the point where exceeds_mwl() is called, xfer.len is 0 because only .rnw
was initialized in the struct declaration. The actual xfer.len is assigned
after the check, meaning the MWL validation will always pass regardless of the
requested size. This could allow hardware to receive writes exceeding its
supported maximum length. Should the length clamp and assignment happen before
the validation check?

[ ... ]

> +	scoped_guard(mutex, &i3cdev->lock) {
> +		/* .remove was called so don't mess with the device */
> +		if (!i3cdev->bound) {
> +			ret = -ENXIO;
> +			goto out_free_kbuf;
> +		}
> +
> +		ret = i3c_device_do_xfers(i3c, &xfer, 1, I3C_SDR);
> +		if (ret)
> +			goto out_free_kbuf;
> +	}

[Severity: Medium]
Similar to i3cdev_read(), does this mix goto-based error handling with
scoped_guard() in the write path?

[ ... ]

> +static void print_i3c_err(struct i3cdev_data *i3cdev,
> +			  struct i3cdev_xfers *metadata,
> +			  struct i3c_xfer *i3c_xfers)
> +{
> +	for (int i = 0; i < metadata->nxfers; i++) {
> +		/* Valid errors, e.g. M0 (now called C0) start at 1 */
> +		if (i3c_xfers[i].err > 0) {
> +			dev_warn(&i3cdev->dev,
> +				 "I3C error encountered: C%u\n",
> +				 i3c_xfers[i].err - 1);
> +			return;
> +		}
> +	}
> +}

[Severity: Medium]
Could this un-ratelimited kernel logging allow unprivileged users to flood
the dmesg log?

Since this error is triggered directly by user-supplied transfers via the
ioctl interface, a user-space application could repeatedly trigger hardware
errors to intentionally spam the kernel logs. Could we use a rate-limited
variant like dev_warn_ratelimited() here to prevent a potential local DoS?

[ ... ]

> +static int
> +i3cdev_ioctl_do_xfers(struct i3cdev_data *i3cdev,
> +		      struct i3cdev_xfers __user *uxfers)
> +{

[ ... ]

> +	scoped_guard(mutex, &i3cdev->lock) {
> +		/* .remove was called so don't mess with the device */
> +		if (!i3cdev->bound) {
> +			ret = -ENXIO;
> +			goto out_free_i3c_xfers;
> +		}
> +		ret = i3c_device_do_xfers(i3c, i3c_xfers,
> +					  metadata.nxfers, I3C_SDR);
> +		if (ret) {
> +			print_i3c_err(i3cdev, &metadata, i3c_xfers);
> +			goto out_free_i3c_xfers;
> +		}
> +	}

[Severity: Medium]
As in the read/write paths, does this also improperly mix scoped_guard()
with goto labels in the ioctl handler?

[ ... ]

> +static int i3cdev_probe(struct i3c_device *i3c)
> +{

[ ... ]

> +	i3cdev->dev.parent = i3c_device_dev;
> +	i3cdev->dev.devt = i3cdev->devt;
> +	i3cdev->dev.class = &i3cdev_class;
> +	i3cdev->dev.release = free_i3cdev_data;
> +	ret = dev_set_name(&i3cdev->dev, SYSFS_FORMAT, MINOR(i3cdev->devt));
> +	if (ret)
> +		goto error_free_ida;

[Severity: Medium]
Does this error path leak the i3cdev memory structure and leave an
unbalanced reference count on the underlying I3C device?

If dev_set_name() fails, execution jumps directly to error_free_ida, skipping
the release of i3cdev memory and the device reference acquired earlier in
probe. Should this instead call put_device(&i3cdev->dev) so that
free_i3cdev_data is executed to handle the cleanup properly?

[ ... ]

> +	scoped_guard(mutex, &i3cdev->lock) {
> +		ret = cdev_device_add(&i3cdev->cdev, &i3cdev->dev);
> +		if (ret)
> +			goto error_cleanup;
> +
> +		i3cdev->bound = true;
> +	}

[Severity: Medium]
Does this sequence mix scoped_guard() with a goto error cleanup label?

As with the other functions in this patch, this mixes scoped cleanup with
goto statements, which goes against the VFS/cleanup guidelines.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911210935.1353126-1-meaganlloyd@linux.microsoft.com?part=3

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