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

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.

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9

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