Re: [PATCH v4 3/3] i3c: add i3cdev module to expose i3c dev in /dev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Silent integer truncation and unbounded memory allocation in `i3cdev_read` and `i3cdev_write` due to unvalidated `size_t count`.
- [High] Race condition during initialization leads to double attachment, resulting in a memory leak and Use-After-Free on device removal.
- [Medium] Missing handling of `BUS_NOTIFY_DRIVER_NOT_BOUND` permanently leaves the device inaccessible if the kernel driver probe fails.
- [Medium] Bus notifier callback incorrectly returns negative error codes instead of NOTIFY_* constants, breaking the notifier chain.
- [Medium] `ida_alloc` lacks an upper bound check, which can lead to invalid minor device numbers.
--

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

i3c: add i3cdev module to expose i3c dev in /dev
    
This patch adds userspace character device support for I3C SDR private
transfers via /dev.

> diff --git a/drivers/i3c/i3cdev.c b/drivers/i3c/i3cdev.c
> new file mode 100644
> index 0000000000000..904ebfd487696
> --- /dev/null
> +++ b/drivers/i3c/i3cdev.c
[ ... ]
> +static struct i3cdev_data *get_free_i3cdev(struct i3c_device *i3c)
> +{
> +	struct i3cdev_data *i3cdev;
> +	int id;
> +
> +	id = ida_alloc(&i3cdev_ida, GFP_KERNEL);

[Severity: Medium]
Since this allows IDs up to INT_MAX, could an artificially inflated IDA
index exceed MINORMASK?

If the returned id exceeds MINORMASK, the macro MKDEV(MAJOR, id) will
overflow the id into the major number bits. Should this use
ida_alloc_max(..., MINORMASK, ...) or similar to prevent character device
number corruption?

[ ... ]
> +static ssize_t
> +i3cdev_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
> +{
> +	struct i3cdev_data *i3cdev = file->private_data;
> +	struct i3c_device *i3c;
> +	struct i3c_xfer xfers = {
> +		.rnw = true,
> +		.len = count,
> +	};

[Severity: High]
The count variable is a size_t, but xfers.len is a u16. Could this silently
truncate read requests larger than 65535 bytes?

> +	int ret = -ENODEV;
> +	char *tmp;
> +
> +	mutex_lock(&i3cdev->xfer_lock);
> +	i3c = i3cdev->i3c;
> +	if (!i3c || i3c->dev.driver)
> +		goto err_out;
> +
> +	tmp = kzalloc(count, GFP_KERNEL);

[Severity: High]
Since count is derived directly from userspace without bounds checking here,
could an extremely large read() request trigger unbounded kernel allocations?

This might result in page allocation failure warnings and dmesg spam. Would
it make sense to enforce a size limit or use __GFP_NOWARN?

[ ... ]
> +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;
> +	struct i3c_xfer xfers = {
> +		.rnw = false,
> +		.len = count,
> +	};

[Severity: High]
Similar to the read path, could the size_t count be silently truncated to
u16 here?

[ ... ]
> +	dev_dbg(&i3c->dev, "Writing %zu bytes.\n", count);
> +
> +	ret = i3c_device_do_xfers(i3c, &xfers, 1, I3C_SDR);
> +	kfree(tmp);
> +
> +err_out:
> +	mutex_unlock(&i3cdev->xfer_lock);
> +	return (!ret) ? count : ret;

[Severity: High]
If count was truncated when passed to xfers.len, does returning the original
un-truncated count mislead userspace into believing all bytes were written
when only up to 65535 bytes were actually sent?

[ ... ]
> +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 || dev->driver)
> +		return 0;
> +
> +	i3c = dev_to_i3cdev(dev);
> +
> +	/* Get a device */
> +	i3cdev = get_free_i3cdev(i3c);

[Severity: High]
In i3cdev_init(), bus_register_notifier() is called before
i3c_for_each_dev(). If a device is hotplugged concurrently, could
i3cdev_attach() be called twice for the same device?

Since get_free_i3cdev() unconditionally allocates a new i3cdev_data and
calls i3cdev_set_drvdata(), would the second call overwrite the first
allocation's data, permanently leaking the first i3cdev_data and its cdev?

> +	if (IS_ERR(i3cdev))
> +		return PTR_ERR(i3cdev);
[ ... ]
> +	res = cdev_device_add(&i3cdev->cdev, &i3cdev->dev);
> +	if (res)
> +		goto error_put_dev;
> +
> +	pr_debug("i3cdev: I3C device [%s] registered as minor %d\n",
> +		 dev_name(&i3c->dev), i3cdev->id);
> +	return 0;
> +
> +error_put_dev:
> +	i3cdev_set_drvdata(i3c, NULL);

[Severity: High]
If a second attach attempt from the race condition fails here (for example,
due to a sysfs name collision in cdev_device_add()), it sets drvdata to NULL.

Would this leave the successfully registered first cdev untracked?

> +	put_device(&i3cdev->dev);
> +	return res;
> +}
> +
> +static int i3cdev_detach(struct device *dev, void *dummy)
> +{
> +	struct i3cdev_data *i3cdev;
> +	struct i3c_device *i3c;
> +
> +	if (dev->type == &i3c_masterdev_type)
> +		return 0;
> +
> +	i3c = dev_to_i3cdev(dev);
> +
> +	i3cdev = i3cdev_get_drvdata(i3c);
> +	if (!i3cdev)
> +		return 0;

[Severity: High]
If the device was left with a NULL drvdata but an active cdev due to the
initialization race condition, would returning early here skip
cdev_device_del()?

Could this leave the orphaned cdev accessible and trigger a Use-After-Free
when userspace accesses the underlying i3c_device after it has been freed?

[ ... ]
> +static int i3cdev_notifier_call(struct notifier_block *nb,
> +				unsigned long action,
> +				void *data)
> +{
> +	struct device *dev = data;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		return i3cdev_attach(dev, NULL);

[Severity: Medium]
If i3cdev_attach() fails, it returns a negative error code (like -ENOMEM).
Since negative values have their high bit set, does returning them here
accidentally set the NOTIFY_STOP_MASK (0x8000) expected by the
blocking_notifier_call_chain API?

Could this prematurely terminate the bus notifier chain and break other
subsystems relying on I3C bus events?

> +	case BUS_NOTIFY_DEL_DEVICE:
> +	case BUS_NOTIFY_REMOVED_DEVICE:
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		return i3cdev_detach(dev, NULL);

[Severity: Medium]
If a kernel driver probe fails, the core emits BUS_NOTIFY_DRIVER_NOT_BOUND.
Since this action is missing from the switch block, does the device get
permanently stuck without a character device interface if it was detached
during BUS_NOTIFY_BIND_DRIVER?

> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int __init i3cdev_init(void)
> +{
[ ... ]
> +	/* Keep track of busses which have devices to add or remove later */
> +	res = bus_register_notifier(&i3c_bus_type, &i3cdev_notifier);
> +	if (res)
> +		goto out_unreg_class;
> +
> +	/* Bind to already existing device without driver right away */
> +	i3c_for_each_dev(NULL, i3cdev_attach);

[Severity: High]
As mentioned earlier, does this initialization order open a race window for
concurrent device additions, potentially leading to the double attachment
and UAF issue?

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

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