Re: [PATCH RFC RESEND 2/7] i3c: master: Report wakeup events for IBIs

[Adrian Hunter <adrian.hunter@intel.com>]

On 01/07/2026 23:33, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Unconditional NULL pointer dereference of `dev->dev` in `i3c_master_queue_ibi` during device teardown
> 
> Pre-existing issues:
> - [High] Lockless TOCTOU race leading to Use-After-Free of `dev->ibi` during concurrent IBI teardown
> --
> 
> commit ffdd9b066a2bdadecaab205d187dab1a5ecff5ae
> Author: Adrian Hunter <adrian.hunter@intel.com>
> 
> 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.  Use 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 a47e2924bb701..a36f9b617cbf7 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
> 
> [ ... ]
> 
>> @@ -3007,6 +3010,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]
> Does this code introduce a NULL pointer dereference if an interrupt arrives
> during device teardown?
> 
> During I3C master controller unregistration, i3c_master_unregister_i3c_devs()
> clears the device description and sets i3cdev->dev to NULL:
> 
> 	i3cdev->dev->desc = NULL;
> 	if (device_is_registered(&i3cdev->dev->dev))
> 		device_unregister(&i3cdev->dev->dev);

I3C drivers are required to disable and free IBIs when they
are removed, and they do do that.  There is a corner case, if
the IBI disable fails, but that is out of scope for this patch
set.

> 	else
> 		put_device(&i3cdev->dev->dev);

dev->ibi will be NULL for devices that are not registered,
so i3c_master_queue_ibi() would never get to i3c_master_queue_ibi()
even if somehow the controller driver tried to queue an IBI.

> 	i3cdev->dev = NULL;
> 
> Since clearing dev->desc prevents the driver's remove callback from disabling
> the hardware IBI, the IBI may still fire. If i3c_master_queue_ibi() executes
> at this point, the newly added device_may_wakeup(&dev->dev->dev) will
> dereference the NULL dev->dev pointer.
> 
>>  	atomic_inc(&dev->ibi->pending_ibis);
> 
> [Severity: High]
> This is a pre-existing issue, but does this lockless check open a 
> use-after-free race window?
> 
> If a hardware IBI interrupt is processed concurrently on another CPU, it can
> evaluate if (!dev->ibi) as false, get preempted, and then access
> dev->ibi->pending_ibis and dev->ibi->wq after they have been freed by
> i3c_dev_free_ibi_locked():
> 
> 	kfree(dev->ibi);
> 	dev->ibi = NULL;
> 
> This is possible for hardware controllers like dw-i3c-master that do not
> implicitly synchronize with executing hardware IRQ handlers during their
> disable op.

Yes, some drivers have gaps.

For dw, an IBI would have to arrive immediately before the DISEC CCC
and end up being processed during the same invocation of
dw_i3c_master_irq_handler().  The DISEC would complete() first which
would potentially allow i3c_dev_disable_ibi_locked() to
atomic_read(&dev->ibi->pending_ibis) before the dw driver gets to
atomic_inc(&dev->ibi->pending_ibis) via i3c_master_queue_ibi().

Out of scope for this patch set.

> 
>>  	queue_work(dev->ibi->wq, &slot->work);
>>  }
>