Re: [PATCH] i3c: master: Release I2C boardinfo node reference

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free and Double-Free on I3C device registration failure.
--

--- Patch [1]: [PATCH] i3c: master: Release I2C boardinfo node reference ---
commit fb94e3e139711c2723cb1a74b85c8a9c79efcfd3
Author: Ruoyu Wang <[email protected]>

i3c: master: Release I2C boardinfo node reference

The commit registers the board-info node reference as a managed resource
of the I3C master. This ensures the reference is properly retained for the
full lifetime of the board-info and released on both registration failure
and normal master teardown, preventing memory leaks.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..8063e2642cbda 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but while reviewing this file, I noticed a
potential use-after-free and double-free regression when handling I3C
device registration failures.

If device_register() fails in i3c_master_register_new_i3c_devs(), put_device()
is called to free desc->dev:

drivers/i3c/master.c:i3c_master_register_new_i3c_devs() {
    ...
		ret = device_register(&desc->dev->dev);
		if (ret) {
			dev_err(&master->dev,
				"Failed to add I3C device (err = %d)\n", ret);
			put_device(&desc->dev->dev);
		}
    ...
}

However, desc->dev is not set to NULL after being freed, leaving a dangling
pointer on the master's bus list.

If the master is later unregistered, i3c_master_unregister_i3c_devs() iterates
over the bus list and accesses this dangling pointer:

drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
    ...
	i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
		if (!i3cdev->dev)
			continue;

		i3cdev->dev->desc = NULL;
		if (device_is_registered(&i3cdev->dev->dev))
			device_unregister(&i3cdev->dev->dev);
		else
			put_device(&i3cdev->dev->dev);
		i3cdev->dev = NULL;
	}
    ...
}

Could this sequence lead to writing to freed memory when setting
i3cdev->dev->desc to NULL, followed by a double-free when put_device() is
called again?

Would it be safer to clear the desc->dev pointer in the error path of
i3c_master_register_new_i3c_devs() after the initial put_device() call?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c