[PATCH] i3c: master: adi: initialize the lock before enabling interrupts

[sashiko-bot at kernel.org]

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

Pre-existing issues:
- [High] Missing hardware cleanup on `i3c_master_register()` failure.
- [High] Dynamic Address Assignment (DAA) repeatedly assigns the exact same address to all discovered I3C devices.
- [High] Premature IRQ unmasking in DAA combined with a missing bounds check causes an Out-of-Bounds read.
- [High] A single transfer timeout causes subsequently queued transfers to stall indefinitely.
- [High] Lockless array iteration leads to a Use-After-Free of the IBI pool.
- [High] Missing return statement causes the probe function to proceed with unsupported hardware.
- [Medium] Pending hardware interrupts are not cleared before unmasking, leading to spurious IRQs.
--

commit a3affb1ca97425b3ccb4b28da6e2b361f1b86e4e
Author: Runyu Xiao <runyu.xiao at seu.edu.cn>

i3c: master: adi: initialize the lock before enabling interrupts

This commit reorders the probe function to initialize the transfer queue lock
and IBI state before requesting and unmasking the IRQ. This prevents a pending
CMDR interrupt from attempting to acquire an uninitialized dynamic lock.

> diff --git a/drivers/i3c/master/adi-i3c-master.c b/drivers/i3c/master/adi-i3c-master.c
> index 047081c9f0643..e29aac2869577 100644
> --- a/drivers/i3c/master/adi-i3c-master.c
> +++ b/drivers/i3c/master/adi-i3c-master.c
> @@ -964,17 +964,9 @@ static int adi_i3c_master_probe(struct platform_device *pdev)
>  	writel(0x00, master->regs + REG_ENABLE);

[Severity: High]
This is a pre-existing issue, but is a return statement missing when checking
the peripheral version just prior to this line?

If ADI_AXI_PCORE_VER_MAJOR(version) != 1, dev_err_probe() logs an error but
doesn't abort execution. The probe function then proceeds to map registers
and initialize unsupported hardware, which could lead to unpredictable system
behavior.

>  	writel(0x00, master->regs + REG_IRQ_MASK);

[Severity: Medium]
This isn't a bug introduced by this patch, but could this lead to spurious
interrupts?

The driver initializes the interrupt mask to 0, but fails to clear
REG_IRQ_PENDING here. If a bootloader or warm reboot left stale pending
interrupts, they might fire immediately when unmasked later in the probe
function.

[ ... ]
> @@ -986,6 +978,13 @@ static int adi_i3c_master_probe(struct platform_device *pdev)
>  	spin_lock_init(&master->xferqueue.lock);
>  	INIT_LIST_HEAD(&master->xferqueue.list);
>  
> +	ret = devm_request_irq(&pdev->dev, irq, adi_i3c_master_irq, 0,
> +			       dev_name(&pdev->dev), master);
> +	if (ret)
> +		return ret;
> +
> +	writel(REG_IRQ_PENDING_CMDR, master->regs + REG_IRQ_MASK);
> +
>  	return i3c_master_register(&master->base, &pdev->dev,
>  				   &adi_i3c_master_ops, false);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but what happens to the hardware
state if i3c_master_register() fails?

The hardware interrupt is unmasked by writing to REG_IRQ_MASK, but if the
registration fails, the probe function returns the error directly without
masking the interrupts or disabling the hardware. Even though the devres
framework tears down the IRQ handler, the hardware remains active and can
continue asserting the interrupt line, potentially causing an interrupt
storm.


While reviewing this file, I found some other pre-existing issues:

In drivers/i3c/master/adi-i3c-master.c:adi_i3c_master_do_daa():

> 	for (u8 i = 0; i < ADI_MAX_DEVS; i++) {
> 		addr = i3c_master_get_free_addr(m, addr);
> 		if (addr < 0)
> 			return addr;
> 		master->daa.addrs[i] = addr;
> 	}

[Severity: High]
This is a pre-existing issue, but doesn't this loop repeatedly assign the
exact same address to all discovered I3C devices?

i3c_master_get_free_addr() returns the next free address greater than or
equal to addr. Because addr is not attached to a device until after the
loop completes, subsequent iterations will evaluate the same addr as free.
This populates master->daa.addrs with identical addresses.

Should the code increment addr to request the next available address?


Also in drivers/i3c/master/adi-i3c-master.c:adi_i3c_master_do_daa():

> 	writel(irq_mask | REG_IRQ_PENDING_DAA,
> 	       master->regs + REG_IRQ_MASK);
> 
> 	master->daa.index = 0;

And in adi_i3c_master_handle_da_req():

> 	adi_i3c_master_rd_from_rx_fifo(master, payload0, 6);
> 	addr = master->daa.addrs[master->daa.index++];
> 	addr = (addr << 1) | (parity8(addr) ? 0 : 1);

[Severity: High]
This isn't a bug introduced by this patch, but could this create a race
condition leading to an out-of-bounds read?

In adi_i3c_master_do_daa(), the DAA interrupt is unmasked before
master->daa.index is reset to 0. If a pending DAA interrupt triggers
instantly, adi_i3c_master_handle_da_req() will execute with a stale
master->daa.index value, which can be up to 16.

This would cause an out-of-bounds read past the 16-element array when
accessing master->daa.addrs.


In drivers/i3c/master/adi-i3c-master.c:adi_i3c_master_unqueue_xfer():

> 	guard(spinlock_irqsave)(&master->xferqueue.lock);
> 	if (master->xferqueue.cur == xfer)
> 		master->xferqueue.cur = NULL;
> 	else
> 		list_del_init(&xfer->node);

[Severity: High]
This is a pre-existing issue, but does dropping an active transfer here fail
to kick off the next transfer in the pending queue?

If a transfer times out, master->xferqueue.cur is set to NULL to clear it.
However, the driver doesn't seem to dequeue and promote the next pending
transfer from master->xferqueue.list.

Any transfers already queued could be stranded indefinitely, only executing
out-of-order if a completely new transfer is queued later.


In drivers/i3c/master/adi-i3c-master.c:adi_i3c_master_handle_ibi():

> 	for (id = 0; id < master->ibi.num_slots; id++) {
> 		if (master->ibi.slots[id] &&
> 		    master->ibi.slots[id]->info.dyn_addr == da)
> 			break;
> 	}
> 
> ...
> 
> 	guard(spinlock)(&master->ibi.lock);
> 	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);

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

The handler iterates over master->ibi.slots without holding
master->ibi.lock. Concurrently, adi_i3c_master_free_ibi() can acquire the
lock, clear the slot, and free data->ibi_pool.

If handle_ibi reads the device before it is cleared, but blocks on
acquiring master->ibi.lock until after free_ibi finishes, it will call
i3c_generic_ibi_get_free_slot() on the freed memory pool.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617150138.628578-1-runyu.xiao@seu.edu.cn?part=1