Re: [PATCH RFC RESEND 1/7] i3c: master: Support IBI-based wakeup capability

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

Pre-existing issues:
- [High] Use-After-Free and double-free of I3C device due to dangling pointer on device_register() failure.
- [High] Reference count underflow of OF node due to missing of_node_get() during device registration.
--

commit 9efcfa1df1912edf83596e1ebd38559fc710aaca
Author: Adrian Hunter <[email protected]>

i3c: master: Support IBI-based wakeup capability

This patch adds an ibi_wakeup flag to the I3C master controller. It allows
IBI-capable I3C devices to be marked as wakeup capable during registration
if the controller supports waking the system via In-Band Interrupts.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -1930,6 +1931,13 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		if (desc->boardinfo)
>  			desc->dev->dev.of_node = desc->boardinfo->of_node;

[Severity: High]
This isn't a bug introduced by this patch, but does this assignment cause
an OF node reference count underflow?

The pointer is assigned without taking a new reference via of_node_get().
However, when the device is freed, i3c_device_release() unconditionally
calls of_node_put(i3cdev->dev.of_node).

If device_register() fails just below this, or when the device is eventually
unregistered, the release function will drop a reference that was never
acquired for the device itself.

>  
> +		/*
> +		 * In the case of IBI wakeup, any IBI-capable device can
> +		 * wakeup.
> +		 */
> +		if (master->ibi_wakeup && (desc->info.bcr & I3C_BCR_IBI_REQ_CAP))
> +			device_set_wakeup_capable(&desc->dev->dev, true);
> +
>  		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);

[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling
pointer that causes a use-after-free and double-free later?

If device_register() fails, put_device() is called which frees desc->dev
via i3c_device_release(), but desc->dev is never set to NULL.

Later, during controller teardown, i3c_master_unregister_i3c_devs()
iterates over the bus and checks if (!i3cdev->dev). Since the pointer
wasn't cleared, it proceeds to dereference it and free it again:

i3c_master_unregister_i3c_devs() {
    ...
    i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
        if (!i3cdev->dev)
            continue;

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

Should desc->dev be set to NULL after the put_device() call here?

>  		}

-- 
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