Re: [PATCH V4 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] NULL pointer dereference in `i3c_master_queue_ibi()` when accessing `dev->dev`.

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);
>  }

-- 
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