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

On 06/08/2026 16:49, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] NULL pointer dereference in `i3c_master_queue_ibi()` when accessing `dev->dev`.

Refer:

	https://lore.kernel.org/all/[email protected]/

> 
> Pre-existing issues:
> - [High] Race condition in `i3c_master_queue_ibi()` leading to Use-After-Free of `dev->ibi`.
> --
> 
> --- Patch [9]: [PATCH V4 09/14] i3c: master: Report wakeup events for IBIs ---
> commit 461a49ac6bedf9467539aa8fac5583bf77ad5045
> Author: Adrian Hunter <[email protected]>
> 
> i3c: master: Report wakeup events for IBIs
> 
> An I3C device configured as a wakeup source can wake the system by
> generating an In-Band Interrupt (IBI). When an IBI is queued for
> processing, record a wakeup event for the device if wakeup is enabled.
> 
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index 2616354dbe545..6c53414919445 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
> 
> [ ... ]
> 
>> @@ -3397,6 +3400,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]
> Can dev->dev be NULL here?
> 
> Looking at i3c_master_unregister_i3c_devs(), it sets i3cdev->dev to NULL
> during device teardown:
> 
> drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
>     ...
> 		i3cdev->dev->desc = NULL;
> 		put_device(&i3cdev->dev->dev);
> 		i3cdev->dev = NULL;
> 	}
> }
> 
> If a hardware IBI is received during or shortly after this teardown,
> wouldn't accessing &dev->dev->dev cause a NULL pointer dereference when
> evaluated inside device_may_wakeup()?
> 
>>  	atomic_inc(&dev->ibi->pending_ibis);
> 
> [Severity: High]
> This is a pre-existing issue, but does the lack of synchronization here allow
> a use-after-free of dev->ibi?
> 
> The teardown path in i3c_dev_disable_ibi_locked() checks pending_ibis
> locklessly and bypasses waiting if it reads 0:
> 
> 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 an ISR is executing concurrently and passes the check for !dev->ibi at
> the top of i3c_master_queue_ibi(), but hasn't yet reached the atomic_inc()
> here, the teardown thread could see pending_ibis == 0 and proceed to free
> dev->ibi in i3c_dev_free_ibi_locked():
> 
> drivers/i3c/master.c:i3c_dev_free_ibi_locked() {
>     ...
> 	kfree(dev->ibi);
> 	dev->ibi = NULL;
>     ...
> }
> 
> When the ISR resumes, wouldn't the subsequent accesses to
> dev->ibi->pending_ibis and dev->ibi->wq cause a use-after-free? Adding
> the wakeup event logic slightly widens this existing race window.
> 
>>  	queue_work(dev->ibi->wq, &slot->work);
>>  }
>