Re: [PATCH 08/17] i3c: mipi-i3c-hci: Fix runtime PM violation in i3c_hci_free_ibi()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-after-free of IBI slot and payload buffer during IBI teardown due to premature memory pool freeing.
- [High] NULL pointer dereference when processing IBI requests on v2 HCI controllers.
--

commit 732d63594fc6d691b8839c600454610c6dcb62e2
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: mipi-i3c-hci: Fix runtime PM violation in i3c_hci_free_ibi()

This commit addresses a runtime PM issue where i3c_hci_free_ibi() calls
__i3c_hci_disable_ibi() without runtime PM being resumed. It fixes the issue
by replacing the hardware disable call with software removal of the device
from the ibi_devs array.

>     - runtime resume failed in i3c_dev_free_ibi_locked(), so it skips the
>       IBI disable but still proceeds to call ->free_ibi()
>     - the device has been re-enumerated under a new dynamic address in
>       __i3c_master_add_i3c_dev_locked(), which tears down the old device
>       descriptor and deliberately clears olddev->ibi->enabled (to avoid
>       sending a DISEC to the stale old address) before calling
>       i3c_dev_free_ibi_locked().  As a result ->free_ibi() is reached with
>       neither a hardware IBI disable nor runtime PM resumed.

[Severity: Critical]
This is a pre-existing issue, but does skipping the disable step in
i3c_dev_free_ibi_locked() lead to a use-after-free when there are pending IBIs?

When i3c_dev_free_ibi_locked() skips i3c_dev_disable_ibi_locked(), it proceeds
to call master->ops->free_ibi(dev) which frees the backing memory pool:

drivers/i3c/master.c:i3c_dev_free_ibi_locked() {
    ...
    master->ops->free_ibi(dev);

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

Because destroy_workqueue() flushes and executes pending works after the pool
is already freed, could the work handler access the freed slot data?

drivers/i3c/master.c:i3c_master_handle_ibi() {
    ...
    payload.data = slot->data;
    payload.len = slot->len;

    if (dev->dev)
        dev->ibi->handler(dev->dev, &payload);
    ...
}

> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index f6a34ce7a2955..b9b20797d0454 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -675,11 +675,13 @@ static void __i3c_hci_disable_ibi(struct i3c_hci *hci, struct i3c_dev_desc *dev)
>  
>  static void i3c_hci_free_ibi(struct i3c_dev_desc *dev)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does the __i3c_hci_disable_ibi()
function (and i3c_hci_request_ibi()) cause a NULL pointer dereference on v2
HCI controllers?

For v2 controllers (where hci->cmd == &mipi_i3c_hci_cmd_v2), i3c_hci_bus_init()
does not initialize the DAT array, leaving hci->DAT as NULL. However, the IBI
ops unconditionally invoke v1 DAT operations:

drivers/i3c/master/mipi-i3c-hci/core.c:__i3c_hci_disable_ibi() {
    ...
    mipi_i3c_hci_dat_v1.set_flags(hci, dev_data->dat_idx, DAT_0_SIR_REJECT, 0);
    ...
}

And similarly in i3c_hci_request_ibi():

drivers/i3c/master/mipi-i3c-hci/core.c:i3c_hci_request_ibi() {
    ...
    if (req->max_payload_len != 0)
        mipi_i3c_hci_dat_v1.set_flags(hci, dat_idx, DAT_0_IBI_PAYLOAD, 0);
    ...
}

Will these unconditionally dereference the NULL hci->DAT pointer via
dat_w0_read() when an IBI is requested or disabled on a v2 controller?

> +	struct i3c_hci_dev_data *dev_data = i3c_dev_get_master_data(dev);
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct i3c_hci *hci = to_i3c_hci(m);
>  
> -	/* Must ensure the IBI has been disabled */
> -	__i3c_hci_disable_ibi(hci, dev);
> +	/* Must ensure IBIs for this device will no longer be processed */
> +	scoped_guard(spinlock_irqsave, &hci->lock)
> +		hci->ibi_devs[dev_data->dat_idx] = NULL;
>  	hci->io->free_ibi(hci, dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914113003.183150-1-adrian.hunter@intel.com?part=8

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