Re: [PATCH RFC 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] Dangling pointer and double free on device_register() error path
- [High] Missing of_node_get() causes Use-After-Free of device tree node
--

commit ea80dbce0f5cdda7818e80b6dab51dc68f768d72
Author: Adrian Hunter <[email protected]>

i3c: master: Support IBI-based wakeup capability

Adds an ibi_wakeup flag to struct i3c_master_controller so controller
drivers can advertise support for IBI-based wakeup. If set, marks
IBI-capable I3C devices as wakeup capable when they are registered.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..a47e2924bb701 100644
> --- 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 need to
increment the reference count of the device tree node using of_node_get()?

Currently, the node is assigned directly from desc->boardinfo->of_node.
However, when the I3C device is later released (either via unregistration or
a device_register() failure), i3c_device_release() unconditionally calls
of_node_put(i3cdev->dev.of_node).

Could this consume the reference conceptually owned by boardinfo and lead to
a use-after-free of the device node on repeated hot-plug cycles or
registration failures?

> +		/*
> +		 * 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 is it possible for this error path to
leave a dangling pointer that causes a use-after-free later?

If device_register() fails, put_device() frees desc->dev via
i3c_device_release(). However, desc->dev and desc->dev->desc are not
cleared.

If i3c_master_unregister_i3c_devs() is later invoked during driver unbind,
it appears to find the non-NULL pointer and write to freed memory:

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

and then execute a double-free on the device object. Should this error path
clear desc->dev to prevent this?

>  	}
>  }

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