Re: [PATCH 06/17] i3c: mipi-i3c-hci: Fix i3c_hci_enable_ibi() error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] On v2 hardware (CMD_SIZE=1 mode), enabling, disabling, or requesting In-Band Interrupts triggers a NULL pointer dereference because the driver unconditionally invokes v1 Device Address Table (DAT) methods which are not initialized for v2 hardware.
--

commit 088a038c298b55a08b26da9db716274664400c73
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: mipi-i3c-hci: Fix i3c_hci_enable_ibi() error path

i3c_hci_enable_ibi() performs controller-side setup before sending the
ENEC CCC to enable IBIs on the target. If the ENEC CCC fails, the
function returns an error but leaves the controller configured as if IBI
enabling had succeeded. This patch fixes the error path by undoing the
setup.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 8b3a87d8a8bb0..166d9f70f4358 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -703,11 +703,15 @@ static int i3c_hci_enable_ibi(struct i3c_dev_desc *dev)
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct i3c_hci *hci = to_i3c_hci(m);
>  	struct i3c_hci_dev_data *dev_data = i3c_dev_get_master_data(dev);
> +	int ret;
>  
>  	mipi_i3c_hci_dat_v1.clear_flags(hci, dev_data->dat_idx, DAT_0_SIR_REJECT, 0);

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause a NULL
pointer dereference on v2 hardware (CMD_SIZE=1 mode)?

It appears mipi_i3c_hci_dat_v1.clear_flags() is called unconditionally here,
even for v2 controllers. This can lead to a NULL pointer dereference in
hci_dat_w0_write() when accessing hci->DAT:

drivers/i3c/master/mipi-i3c-hci/dat_v1.c:hci_dat_w0_write() {
    ...
    hci->DAT[i].w0 = v;
    ...
}

since the v1 Device Address Table is not initialized or allocated for v2
hardware.

>  	scoped_guard(spinlock_irqsave, &hci->lock)
>  		hci->ibi_devs[dev_data->dat_idx] = dev;
> -	return i3c_master_enec_locked(m, dev->info.dyn_addr, I3C_CCC_EVENT_SIR);
> +	ret = i3c_master_enec_locked(m, dev->info.dyn_addr, I3C_CCC_EVENT_SIR);
> +	if (ret)
> +		__i3c_hci_disable_ibi(hci, dev);

[Severity: High]
This is part of the same pre-existing issue, but does this rollback path also
trigger a NULL pointer dereference on v2 hardware?

The __i3c_hci_disable_ibi() function unconditionally calls
mipi_i3c_hci_dat_v1.set_flags(), which will dereference the same uninitialized
structure under v2.

> +	return ret;
>  }
>  
>  static int i3c_hci_disable_ibi(struct i3c_dev_desc *dev)

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

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