Re: [PATCH V3 09/14] i3c: master: Report wakeup events for IBIs

On 04/08/2026 18:10, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Unprotected dereference of dev->dev in i3c_master_queue_ibi() leading to NULL pointer dereference or Use-After-Free during teardown.

dev->dev is always valid.  It is use of the first 'dev' (struct i3c_dev_desc *)
that is questionable because of the oldev/newdev dance in
__i3c_master_add_i3c_dev_locked().  As it says there, olddev does not
exist on the bus, so it is unlikely there are any IBIs inflight from
when it did exist.  It would be strange if a newly DAA'ed device
started issuing IBIs, but most controller drivers do not accept IBIs
from newly DAA'ed devices anyway, so there ought not to be any IBIs
inflight for newdev either.

It is fragile, but it is not a new issue because struct i3c_dev_desc *dev
was/is being used anyway.

> 
> Pre-existing issues:
> - [High] Race condition between hardware IBI interrupts and IBI teardown leading to Use-After-Free of dev->ibi.
> --
> 
> --- Patch [9]: [PATCH V3 09/14] i3c: master: Report wakeup events for IBIs ---
> commit 044b30e8813c466ee0c955f282a081170370a564
> Author: Adrian Hunter <[email protected]>
> 
> i3c: master: Report wakeup events for IBIs
> 
> When an IBI is queued for processing, this commit records a wakeup event
> for the device if wakeup is enabled. It uses a 100 ms processing interval
> to give the I3C device driver time to process the IBI.
> 
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index e6b320da475e0..8c9e62e6f1469 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -3398,6 +3401,9 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot)
>>  	if (!dev->ibi || !slot)
>>  		return;
>>  
>> +	if (device_may_wakeup(&dev->dev->dev))
>> +		pm_wakeup_event(&dev->dev->dev, I3C_WAKEUP_PROCESSING_TIME_MS);
>> +
> 
> [Severity: High]
> Is it possible for an IBI interrupt to fire concurrently with device
> unregistration?
> 
> Looking at i3c_master_unregister_i3c_devs(), the teardown path sets
> dev->dev to NULL:
> 
> drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
>     ...
>     i3cdev->dev->desc = NULL;
>     put_device(&i3cdev->dev->dev);
>     i3cdev->dev = NULL;
>     ...
> }
> 
> If an interrupt occurs during or just after this unregistration, could
> dev->dev be NULL when device_may_wakeup() dereferences it here?
> 
>>  	atomic_inc(&dev->ibi->pending_ibis);
>>  	queue_work(dev->ibi->wq, &slot->work);
>>  }
> 
> [Severity: High]
> This is a pre-existing issue, but is there a race condition between hardware
> IBI interrupts and IBI teardown leading to a use-after-free of dev->ibi?
> 
> In i3c_dev_disable_ibi_locked(), the code checks the pending_ibis counter:
> 
> drivers/i3c/master.c:i3c_dev_disable_ibi_locked() {
>     ...
>     if (atomic_read(&dev->ibi->pending_ibis))
>         wait_for_completion(&dev->ibi->all_ibis_handled);
>     ...
> }
> 
> If a hardirq handler is already executing but has not yet called
> i3c_master_queue_ibi() to increment pending_ibis, the teardown path might
> assume no IBIs are pending and proceed to free dev->ibi in
> i3c_dev_free_ibi_locked().
> 
> When the interrupted CPU resumes, wouldn't it execute this atomic_inc() and
> queue_work() on the freed dev->ibi pointer since there is no
> synchronize_irq() to drain active hardirqs?
>