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

On Wed, Jun 17, 2026 at 08:22:58PM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

Yes, can you help fix it later?

Frank
>
> 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/[email protected]?part=1
>
> --
> linux-i3c mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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