Re: [PATCH v14 2/8] i3c: master: Fix IBI request and free cleanup paths

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

New issues:
- [High] Bypassing the core cleanup paths when `dev->ibi` is cleared by a controller callback causes a permanent leak of the IBI workqueue (including its rescuer kthread) and potentially the `ibi` object itself.

Pre-existing issues:
- [High] Potential use-after-free of IBI pool during workqueue destruction if IBI disable fails.
--

--- Patch [2]: [PATCH v14 2/8] i3c: master: Fix IBI request and free cleanup paths ---
commit e0d1c57d2924f66898a40232f7e52ca0e1be0aea
Author: Lakshay Piplani <[email protected]>

i3c: master: Fix IBI request and free cleanup paths

This commit attempts to fix a workqueue leak in the IBI request failure path
by destroying the workqueue before freeing the IBI object. It also adds checks
to avoid touching dev->ibi after controller callbacks if they have already
cleared it, aiming to prevent NULL pointer dereferences.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 3b1c342be2bc5..ba37f71badac8 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -3702,8 +3702,18 @@ int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
>  
>  	dev->ibi = ibi;
>  	ret = master->ops->request_ibi(dev, req);
> -	if (ret) {
> -		kfree(ibi);
> +
> +	/*
> +	 * The controller callback may have already released and cleared dev->ibi
> +	 * when the request is forwarded by a virtual controller. Only clean up the
> +	 * IBI object if the callback left dev->ibi valid.
> +	 */
> +	if (ret && dev->ibi) {
> +		/* Avoid leaking the workqueue allocated for this IBI request. */
> +		if (dev->ibi->wq)
> +			destroy_workqueue(dev->ibi->wq);
> +
> +		kfree(dev->ibi);
>  		dev->ibi = NULL;
>  	}

[Severity: High]
Does this logic introduce a permanent leak of the IBI workqueue and the local
ibi object?

Since the core framework dynamically allocated ibi and ibi->wq earlier in
i3c_dev_request_ibi_locked(), if master->ops->request_ibi() fails and the
callback cleared dev->ibi, the check if (ret && dev->ibi) will be false.

Because the callback does not own these resources and cannot safely destroy
the framework's workqueue, the workqueue (including its rescuer kthread) and
the ibi object will be skipped during cleanup and leak permanently.

[ ... ]

> @@ -3743,6 +3753,13 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
>  
>  	master->ops->free_ibi(dev);

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a
use-after-free of the IBI pool memory if disabling the IBI fails?

Earlier in i3c_dev_free_ibi_locked(), if the IBI is still enabled, it tries
to disable it via i3c_dev_disable_ibi_locked(). If that disable fails
(e.g., due to a hardware timeout), it returns an error without waiting for
pending IBIs to finish, and we proceed directly to master->ops->free_ibi(dev).

This callback immediately frees the generic IBI pool. Subsequently,
destroy_workqueue() is called below, which synchronously drains
any executing IBI handler works. Since these pending work items reside inside
the newly-freed pool, won't the rescuer thread dereference freed memory as
the workqueue drains?

> +	/*
> +	 * The controller callback may have already released dev->ibi, for example
> +	 * when the request was forwarded by a virtual controller.
> +	 */
> +	if (!dev->ibi)
> +		return;

[Severity: High]
Similar to the request path above, does this early return bypass the
destruction of the workqueue and cause a resource leak?

If the controller callback cleared dev->ibi on the free path, we return
early and skip the destroy_workqueue(dev->ibi->wq) step, leaking the
workqueue and its rescuer thread.

> +
>  	if (dev->ibi->wq) {
>  		destroy_workqueue(dev->ibi->wq);
>  		dev->ibi->wq = NULL;

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

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